Fixing the adjust font restore scroll and adding unit tests

src/view/ViewCommandHandlers.js

@@ -76,24 +76,61 @@ define(function (require, exports, module) {
      * @type {boolean}
      */
     var _fontSizePrefsLoaded = false;
-
+    
 
     /**
      * @private
-     * Removes the styles used to update the font size and updates the editor if refresh is true
-     * @param {boolean} refresh - True to refresh the current full editor
+     * Removes the styles used to update the font size
      */
-    function _removeDynamicFontSize(refresh) {
+    function _removeDynamicFontSize() {
         $("#" + DYNAMIC_FONT_STYLE_ID).remove();
-        if (refresh) {
-            EditorManager.getCurrentFullEditor().refreshAll();
+    }
+
+    /**
+     * @private
+     * Sets the font size and restores the scroll position as best as possible
+     * @param {string} fsStr A string with the font size and the size unit
+     * @param {string} lhStr A string with the line height and a the size unit
+     */
+    function _setSizeAndRestoreScroll(fsStr, lhStr) {
+        var editor     = EditorManager.getCurrentFullEditor(),
+            oldWidth   = editor._codeMirror.defaultCharWidth(),
+            oldHeight  = editor.getTextHeight();
+
+        // Since the height isn't updated after the font-size is changed, we just consider the rendered lines
+        // to calculate the new scroll position
+        var scrollPos   = editor.getScrollPos(),
+            viewportTop = $(".CodeMirror-lines", editor.getRootElement()).parent().position().top,
+            scrollTop   = scrollPos.y - viewportTop;
+
+        // It's necessary to inject a new rule to address all editors.
+        _removeDynamicFontSize();
+        var style = $("<style type='text/css'></style>").attr("id", DYNAMIC_FONT_STYLE_ID);
+        style.html(".CodeMirror {" +
+                   "font-size: "   + fsStr + " !important;" +
+                   "line-height: " + lhStr + " !important;}");
+        $("head").append(style);
+
+        editor.refreshAll();
+
+        // Calculate the new scroll based on the old font sizes and scroll position
+        var newWidth    = editor._codeMirror.defaultCharWidth(),
+            newHeight   = editor.getTextHeight(),
+            deltaX      = scrollPos.x / oldWidth,
+            deltaY      = scrollTop / oldHeight,
+            scrollPosX  = scrollPos.x + Math.round(deltaX * (newWidth - oldWidth)),
+            scrollPosY  = scrollPos.y + Math.round(deltaY * (newHeight - oldHeight));
+
+        // Scroll the document back to its original position, but not on the first load
+        if (_fontSizePrefsLoaded) {
+            editor.setScrollPos(scrollPosX, scrollPosY);
         }
     }
 
     /**
      * @private
      * Increases or decreases the editor's font size.
-     * @param {number} negative number to make the font smaller; positive number to make it bigger.
+     * @param {number} adjustment Negative number to make the font smaller; positive number to make it bigger
      * @return {boolean} true if adjustment occurred, false if it did not occur 
      */
     function _adjustFontSize(adjustment) {
@@ -136,48 +173,28 @@ define(function (require, exports, module) {
             return false;
         }
 
-        // It's necessary to inject a new rule to address all editors.
-        _removeDynamicFontSize(false);
-        var style = $("<style type='text/css'></style>").attr("id", DYNAMIC_FONT_STYLE_ID);
-        style.html(".CodeMirror {" +
-                   "font-size: "   + fsStr + " !important;" +
-                   "line-height: " + lhStr + " !important;}");
-        $("head").append(style);
-
-        var editor = EditorManager.getCurrentFullEditor();
-        editor.refreshAll();
-
-        // Scroll the document back to its original position. This can only happen
-        // if the font size is specified in pixels (which it currently is).
-        if (fsUnits === "px") {
-            var scrollPos = editor.getScrollPos();
-            var scrollDeltaX = Math.round(scrollPos.x / lhOld);
-            var scrollDeltaY = Math.round(scrollPos.y / lhOld);
-
-            scrollDeltaX = (adjustment >= 0 ? scrollDeltaX : -scrollDeltaX);
-            scrollDeltaY = (adjustment >= 0 ? scrollDeltaY : -scrollDeltaY);
-
-            editor.setScrollPos((scrollPos.x + scrollDeltaX),
-                                (scrollPos.y + scrollDeltaY));
-        }
+        _setSizeAndRestoreScroll(fsStr, lhStr);
 
         return true;
     }
 
+    /** Increases the font size by 1 */
     function _handleIncreaseFontSize() {
         if (_adjustFontSize(1)) {
             _prefs.setValue("fontSizeAdjustment", _prefs.getValue("fontSizeAdjustment") + 1);
         }
     }
-
+
+    /** Decreases the font size by 1 */
     function _handleDecreaseFontSize() {
         if (_adjustFontSize(-1)) {
             _prefs.setValue("fontSizeAdjustment", _prefs.getValue("fontSizeAdjustment") - 1);
         }
     }
 
+    /** Restores the font size to the original size */
     function _handleRestoreFontSize() {
-        _removeDynamicFontSize(true);
+        _adjustFontSize(-_prefs.getValue("fontSizeAdjustment"));
         _prefs.setValue("fontSizeAdjustment", 0);
     }
 
@@ -198,7 +215,7 @@ define(function (require, exports, module) {
 
             // Font Size preferences only need to be loaded one time
             if (!_fontSizePrefsLoaded) {
-                _removeDynamicFontSize(false);
+                _removeDynamicFontSize();
                 _adjustFontSize(_prefs.getValue("fontSizeAdjustment"));
                 _fontSizePrefsLoaded = true;
             }
@@ -216,10 +233,10 @@ define(function (require, exports, module) {
     /**
      * @private
      * Calculates the first and last visible lines of the focused editor
-     * @param {!number} textHeight
-     * @param {!number} scrollTop
-     * @param {!number} editorHeight
-     * @param {!number} viewportFrom
+     * @param {number} textHeight
+     * @param {number} scrollTop
+     * @param {number} editorHeight
+     * @param {number} viewportFrom
      * @return {{first: number, last: number}}
      */
     function _getLinesInView(textHeight, scrollTop, editorHeight, viewportFrom) {
@@ -237,7 +254,7 @@ define(function (require, exports, module) {
     /**
      * @private
      * Scroll the viewport one line up or down.
-     * @param {number} -1 to scroll one line up; 1 to scroll one line down.
+     * @param {number} direction -1 to scroll one line up; 1 to scroll one line down.
      */
     function _scrollLine(direction) {
         var editor        = EditorManager.getCurrentFullEditor(),
@@ -306,10 +323,12 @@ define(function (require, exports, module) {
         }
     }
 
+    /** Scrolls one line up */
     function _handleScrollLineUp() {
         _scrollLine(-1);
     }
 
+    /** Scrolls one line down */
     function _handleScrollLineDown() {
         _scrollLine(1);
     }
@@ -326,7 +345,7 @@ define(function (require, exports, module) {
     KeyBindingManager.addBinding(Commands.VIEW_SCROLL_LINE_UP);
     KeyBindingManager.addBinding(Commands.VIEW_SCROLL_LINE_DOWN);
 
-    // Init PreferenceStorage
+    // Initialize the PreferenceStorage
     _prefs = PreferencesManager.getPreferenceStorage(module, _defaultPrefs);
 
     // Update UI when opening or closing a document

test/UnitTestSuite.js

@@ -24,7 +24,7 @@
 /*jslint vars: true, plusplus: true, devel: true, browser: true, nomen: true, indent: 4, maxerr: 50 */
 /*global define */
 define(function (require, exports, module) {
-    'use strict';
+    "use strict";
 
     require("spec/CodeHint-test");
     require("spec/CodeHintUtils-test");
@@ -57,6 +57,7 @@ define(function (require, exports, module) {
     require("spec/QuickOpen-test");
     require("spec/StringMatch-test");
     require("spec/UpdateNotification-test");
+    require("spec/ViewCommandHandlers-test");
     require("spec/ViewUtils-test");
     require("spec/WorkingSetView-test");
 });

test/spec/ViewCommandHandlers-test-files/test.css

@@ -0,0 +1,3 @@
+.testClass {
+  color: red;
+}

test/spec/ViewCommandHandlers-test-files/test.html

@@ -0,0 +1,11 @@
+<!doctype html>
+<html>
+<head>
+<title>Simple Test</title>
+<link rel="stylesheet" href="test.css">
+</head>
+
+<body>
+<p class="testClass">Brackets is awesome!</p>
+</body>
+</html>

test/spec/ViewCommandHandlers-test.js

@@ -0,0 +1,156 @@
+/*
+ * Copyright (c) 2013 Adobe Systems Incorporated. All rights reserved.
+ *  
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"), 
+ * to deal in the Software without restriction, including without limitation 
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense, 
+ * and/or sell copies of the Software, and to permit persons to whom the 
+ * Software is furnished to do so, subject to the following conditions:
+ *  
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *  
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, 
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER 
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
+ * DEALINGS IN THE SOFTWARE.
+ * 
+ */
+
+/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
+/*global define, describe, beforeEach, it, runs, expect, waitsForDone */
+
+define(function (require, exports, module) {
+    "use strict";
+
+    var CommandManager,      // loaded from brackets.test
+        Commands,            // loaded from brackets.test
+        EditorManager,       // loaded from brackets.test
+        FileViewController,
+        SpecRunnerUtils     = require("spec/SpecRunnerUtils");
+
+    describe("ViewCommandHandlers", function () {
+        this.category = "integration";
+
+        var testPath = SpecRunnerUtils.getTestPath("/spec/ViewCommandHandlers-test-files"),
+            testWindow;
+
+        var CSS_FILE  = testPath + "/test.css",
+            HTML_FILE = testPath + "/test.html";
+
+        beforeEach(function () {
+            var promise;
+
+            // Create a new window that will be shared by ALL tests in this spec.
+            if (!testWindow) {
+                SpecRunnerUtils.createTestWindowAndRun(this, function (w) {
+                    testWindow = w;
+
+                    // Load module instances from brackets.test
+                    CommandManager      = testWindow.brackets.test.CommandManager;
+                    Commands            = testWindow.brackets.test.Commands;
+                    EditorManager       = testWindow.brackets.test.EditorManager;
+                    FileViewController  = testWindow.brackets.test.FileViewController;
+
+                    SpecRunnerUtils.loadProjectInTestWindow(testPath);
+                });
+
+                runs(function () {
+                    promise = CommandManager.execute(Commands.FILE_ADD_TO_WORKING_SET, {fullPath: HTML_FILE});
+                    waitsForDone(promise, "Open into working set");
+                });
+
+                runs(function () {
+                    // Open inline editor onto test.css's ".testClass" rule
+                    promise = SpecRunnerUtils.toggleQuickEditAtOffset(EditorManager.getCurrentFullEditor(), {line: 8, ch: 11});
+                    waitsForDone(promise, "Open inline editor");
+                });
+            }
+        });
+
+        function getEditors() {
+            var editor = EditorManager.getCurrentFullEditor();
+            return {
+                editor: editor,
+                inline: editor.getInlineWidgets()[0].editors[0]
+            };
+        }
+
+
+        describe("Adjust the Font Size", function () {
+            it("should increase the font size in both editor and inline editor", function () {
+                runs(function () {
+                    var editors      = getEditors();
+                    var expectedSize = editors.editor.getTextHeight() + 2;
+
+                    CommandManager.execute(Commands.VIEW_INCREASE_FONT_SIZE);
+                    CommandManager.execute(Commands.VIEW_INCREASE_FONT_SIZE);
+
+                    expect(editors.editor.getTextHeight()).toBe(expectedSize);
+                    expect(editors.inline.getTextHeight()).toBe(expectedSize);
+                });
+            });
+
+            it("should decrease the font size in both editor and inline editor", function () {
+                runs(function () {
+                    var editors      = getEditors();
+                    var expectedSize = editors.editor.getTextHeight() - 2;
+
+                    CommandManager.execute(Commands.VIEW_DECREASE_FONT_SIZE);
+                    CommandManager.execute(Commands.VIEW_DECREASE_FONT_SIZE);
+
+                    expect(editors.editor.getTextHeight()).toBe(expectedSize);
+                    expect(editors.inline.getTextHeight()).toBe(expectedSize);
+                });
+            });
+
+            it("should restore the font size in both editor and inline editor", function () {
+                runs(function () {
+                    var editors      = getEditors();
+                    var expectedSize = editors.editor.getTextHeight();
+
+                    CommandManager.execute(Commands.VIEW_INCREASE_FONT_SIZE);
+                    CommandManager.execute(Commands.VIEW_INCREASE_FONT_SIZE);
+                    CommandManager.execute(Commands.VIEW_RESTORE_FONT_SIZE);
+
+                    expect(editors.editor.getTextHeight()).toBe(expectedSize);
+                    expect(editors.inline.getTextHeight()).toBe(expectedSize);
+                });
+            });
+
+            it("should keep the same font size when opening another document", function () {
+                var promise, expectedSize, editor;
+
+                runs(function () {
+                    editor       = EditorManager.getCurrentFullEditor();
+                    expectedSize = editor.getTextHeight() + 1;
+
+                    promise = CommandManager.execute(Commands.VIEW_INCREASE_FONT_SIZE);
+                    waitsForDone(promise, "Increase font size");
+                });
+
+                runs(function () {
+                    // Open another document and bring it to the front
+                    waitsForDone(FileViewController.openAndSelectDocument(CSS_FILE, FileViewController.PROJECT_MANAGER),
+                                 "FILE_OPEN on file timeout", 1000);
+                });
+
+                runs(function () {
+                    editor = EditorManager.getCurrentFullEditor();
+                    expect(editor.getTextHeight()).toBe(expectedSize);
+                });
+
+                // This must be in the last spec in the suite.
+                runs(function () {
+                    this.after(function () {
+                        SpecRunnerUtils.closeTestWindow();
+                    });
+                });
+            });
+        });
+    });
+});