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

Set the font size before opening the documents #7185

Merged
merged 7 commits into from
Mar 14, 2014
Merged

Set the font size before opening the documents #7185

Show file tree
Hide file tree
Changes from 2 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: 53 additions & 45 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,46 @@ 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();

function _addDynamicFontSize(fontSizeStyle, lineHeightStyle) {
// 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: " + 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
Contributor

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
Contributor Author

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
Contributor Author

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
Contributor

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
Contributor Author

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
Contributor

Choose a reason for hiding this comment

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

Ok.

console.log(line);
Copy link
Contributor

Choose a reason for hiding this comment

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

You forgot to remove two console.log in this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops. I forgot to recheck the code before pushing.

if (fontSizeStyle && lineHeightStyle) {
_addDynamicFontSize(fontSizeStyle, lineHeightStyle);
} else {
_removeDynamicFontSize();
Copy link
Contributor

Choose a reason for hiding this comment

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

You can move _removeDynamicFontSize(); before the if statement since it is common for both if and else cases. And don't forget to remove it from _addDynamicFontSize after moving it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion. _addDynamicFontSize shouldn't remove it anyway.

}
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
Contributor

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
Contributor Author

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
Contributor

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");
console.log(scrollPosY);
editor.setScrollPos(scrollPosX, scrollPosY);
}

/**
Expand Down Expand Up @@ -158,6 +159,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 +179,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", undefined);
PreferencesManager.setViewState("lineHeightStyle", undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you're explicitly using undefined as the second argument to clear the view states. My personal preference is to omit the unneeded optional second arguments to remove the view states.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, using PreferencesManager.setViewState("fontSizeStyle") already passes the second argument as undefined.

}


Expand All @@ -215,14 +217,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 +225,18 @@ define(function (require, exports, module) {
}
}

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

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
Contributor

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
Contributor Author

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
Contributor

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.

_addDynamicFontSize(fsStr, lhStr);
}
}



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

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

exports.restoreFontSize = restoreFontSize;
});