Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Set the font size before opening the documents #7185

Merged
RaymondLim merged 7 commits into from
Mar 14, 2014
+103 −90
Merged

Set the font size before opening the documents #7185

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
79ccbe9
Set the font size before openning the documents
TomMalbran Mar 13, 2014
a5a6394
Fixes after Review
TomMalbran Mar 14, 2014
ebb6b7a
More changes after review
TomMalbran Mar 14, 2014
8d0abbd
Migrate "fontSizeAdjustment" to new view states.
RaymondLim Mar 14, 2014
25ee232
Move the line height to the CSS
TomMalbran Mar 14, 2014
bb3e75d
Fixes after last review
TomMalbran Mar 14, 2014
3959dd0
convert to new view state (with no s)
TomMalbran Mar 14, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/brackets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ define(function (require, exports, module) {
ColorUtils = require("utils/ColorUtils"),
CodeInspection = require("language/CodeInspection"),
NativeApp = require("utils/NativeApp"),
ViewCommandHandlers = require("view/ViewCommandHandlers"),
_ = require("thirdparty/lodash");

// Load modules that self-register and just need to get included in the main project
Expand All @@ -107,7 +108,6 @@ define(function (require, exports, module) {
require("editor/EditorStatusBar");
require("editor/EditorCommandHandlers");
require("editor/EditorOptionHandlers");
require("view/ViewCommandHandlers");
require("help/HelpCommandHandlers");
require("search/FindInFiles");
require("search/FindReplace");
Expand Down Expand Up @@ -220,6 +220,7 @@ define(function (require, exports, module) {
// Load the initial project after extensions have loaded
extensionLoaderPromise.always(function () {
// Finish UI initialization
ViewCommandHandlers.restoreFontSize();
var initialProjectPath = ProjectManager.getInitialProjectPath();
ProjectManager.openProject(initialProjectPath).always(function () {
_initTest();
Expand Down
98 changes: 52 additions & 46 deletions src/view/ViewCommandHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,6 @@ define(function (require, exports, module) {
*/
var LINE_HEIGHT = 1.25;

/**
* @private
* @type {boolean}
*/
var _fontSizePrefsLoaded = false;


/**
* @private
Expand All @@ -95,39 +89,43 @@ define(function (require, exports, module) {

/**
* @private
* Sets the font size and restores the scroll position as best as possible.
* @param {string} fontSizeStyle A string with the font size and the size unit
* @param {string} lineHeightStyle A string with the line height and a the size unit
* Add the styles used to update the font size
* @param {string} fontSizeStyle A string with the font size and the size unit
* @param {string} lineHeightStyle A string with the line height and the size unit
*/
function _setSizeAndRestoreScroll(fontSizeStyle, lineHeightStyle) {
var editor = EditorManager.getCurrentFullEditor(),
oldWidth = editor._codeMirror.defaultCharWidth(),
oldHeight = editor.getTextHeight(),
scrollPos = editor.getScrollPos();

// It's necessary to inject a new rule to address all editors.
_removeDynamicFontSize();
function _addDynamicFontSize(fontSizeStyle, lineHeightStyle) {
var style = $("<style type='text/css'></style>").attr("id", DYNAMIC_FONT_STYLE_ID);
style.html(".CodeMirror {" +
"font-size: " + fontSizeStyle + " !important;" +
"line-height: " + lineHeightStyle + " !important;}");
$("head").append(style);
}

/**
* @private
* Sets the font size and restores the scroll position as best as possible.
* @param {string=} fontSizeStyle A string with the font size and the size unit
* @param {string=} lineHeightStyle A string with the line height and the size unit
*/
function _setSizeAndRestoreScroll(fontSizeStyle, lineHeightStyle) {
var editor = EditorManager.getCurrentFullEditor(),
oldWidth = editor._codeMirror.defaultCharWidth(),
scrollPos = editor.getScrollPos(),
line = editor._codeMirror.lineAtHeight(scrollPos.y, "local");
Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmv4 is already landed in master and I wonder lineAtHeight and heightAtLine are still the same in cmv4. Can you double check or can you merge master to your pull request?

Copy link
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure will try, hopefully it should behave the same way.

Copy link
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I made this PR after cmv4 landed.

Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really, but I notice that CM submodule is different.

Copy link
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I can use multiple cursors in this branch.

Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.


_removeDynamicFontSize();
if (fontSizeStyle && lineHeightStyle) {
_addDynamicFontSize(fontSizeStyle, lineHeightStyle);
}
editor.refreshAll();

// Scroll the document back to its original position, but not on the first load since the position
// was saved with the new height and already been restored.
if (_fontSizePrefsLoaded) {
// 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 = scrollPos.y / oldHeight,
scrollPosX = scrollPos.x + Math.round(deltaX * (newWidth - oldWidth)),
scrollPosY = scrollPos.y + Math.round(deltaY * (newHeight - oldHeight));

editor.setScrollPos(scrollPosX, scrollPosY);
}
// Calculate the new scroll based on the old font sizes and scroll position
var newWidth = editor._codeMirror.defaultCharWidth(),
deltaX = scrollPos.x / oldWidth,
scrollPosX = scrollPos.x + Math.round(deltaX * (newWidth - oldWidth)),
Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not related to the two issues you're fixing, but I wonder this calculation is always correct for a long line that can wrap the horizontal scroll position to a new line with Word Wrap on.

Copy link
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if Word Wrap is on, you can't scroll horizontally, so even trying to make ti scroll horizontally, it will always stay at 0.

Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're right!

scrollPosY = editor._codeMirror.heightAtLine(line, "local");

editor.setScrollPos(scrollPosX, scrollPosY);
}

/**
Expand Down Expand Up @@ -158,6 +156,7 @@ define(function (require, exports, module) {

var fsNew = fsOld + (delta * adjustment);
var lhNew = lhOld;

if (fsUnits === lhUnits) {
lhNew = fsNew * LINE_HEIGHT;
if (lhUnits === "px") {
Expand All @@ -177,28 +176,28 @@ define(function (require, exports, module) {

_setSizeAndRestoreScroll(fsStr, lhStr);

PreferencesManager.setViewState("fontSizeStyle", fsStr);
PreferencesManager.setViewState("lineHeightStyle", lhStr);

$(exports).triggerHandler("fontSizeChange", [adjustment, fsStr, lhStr]);
return true;
}

/** Increases the font size by 1 */
function _handleIncreaseFontSize() {
if (_adjustFontSize(1)) {
PreferencesManager.setViewState("fontSizeAdjustment", PreferencesManager.getViewState("fontSizeAdjustment") + 1);
}
_adjustFontSize(1);
}

/** Decreases the font size by 1 */
function _handleDecreaseFontSize() {
if (_adjustFontSize(-1)) {
PreferencesManager.setViewState("fontSizeAdjustment", PreferencesManager.getViewState("fontSizeAdjustment") - 1);
}
_adjustFontSize(-1);
}

/** Restores the font size to the original size */
function _handleRestoreFontSize() {
_adjustFontSize(-PreferencesManager.getViewState("fontSizeAdjustment"));
PreferencesManager.setViewState("fontSizeAdjustment", 0);
_setSizeAndRestoreScroll();
PreferencesManager.setViewState("fontSizeStyle");
PreferencesManager.setViewState("lineHeightStyle");
}


Expand All @@ -215,14 +214,6 @@ define(function (require, exports, module) {
CommandManager.get(Commands.VIEW_DECREASE_FONT_SIZE).setEnabled(true);
CommandManager.get(Commands.VIEW_RESTORE_FONT_SIZE).setEnabled(true);
}

// Font Size preferences only need to be loaded one time
if (!_fontSizePrefsLoaded) {
_removeDynamicFontSize();
_adjustFontSize(PreferencesManager.getViewState("fontSizeAdjustment"));
_fontSizePrefsLoaded = true;
}

} else {
// No current document so disable all of the Font Size commands
CommandManager.get(Commands.VIEW_INCREASE_FONT_SIZE).setEnabled(false);
Expand All @@ -231,6 +222,19 @@ define(function (require, exports, module) {
}
}

/**
* Restores the Font Size and Line Height using the saved strings
Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We can take out Line Height here.

*/
function restoreFontSize() {
var fsStr = PreferencesManager.getViewState("fontSizeStyle"),
lhStr = PreferencesManager.getViewState("lineHeightStyle");

if (fsStr && lhStr) {
Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With these new view states fontSizeAdjustment migration is totally broken. If you can provide me the formula to convert fontSizeAdjustment to the string values for these two new view states, then I can add code to handle the migration in this pull request.

Update: Maybe we can refactor the code in _adjustFontSize to a separate function to return fontSizeStyle and lineHeightStyle, but I wonder $(".CodeMirror").css("font-size") is already available at migration time.

Copy link
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original font size is 12px and we can assume that we always used px, then:

fontSize = 12 + PreferencesManager.getViewState("fontSizeAdjustment");
fontSizeStyle = fontSize + "px";
lineHeightStyle = Math.ceil(fontSize * 1.25) + "px";

Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'll use LINE_HEIGHT in place of 1.25. Let me know when you're done with your changes and I'll push my changes to your branch.

_removeDynamicFontSize();
Copy link
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left this remove here, even though we don't need it on the initial load, but it is required if this method is ever called after appReady.

Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Mar 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfectly fine to call it here to ensure that we don't miss any.

_addDynamicFontSize(fsStr, lhStr);
}
}



/**
Expand Down Expand Up @@ -336,4 +340,6 @@ define(function (require, exports, module) {

// Update UI when Brackets finishes loading
AppInit.appReady(_updateUI);

exports.restoreFontSize = restoreFontSize;
});
⚡