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 1 commit
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
79 changes: 44 additions & 35 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,44 @@ 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 a the size unit
Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Mar 13, 2014

Choose a reason for hiding this comment

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

Typo: a the size unit. should remove a.

*/
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 a the size unit
Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Mar 13, 2014

Choose a reason for hiding this comment

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

Old typo: a the size unit. should remove a.

*/
function _setSizeAndRestoreScroll(fontSizeStyle, lineHeightStyle) {
var editor = EditorManager.getCurrentFullEditor(),
oldWidth = editor._codeMirror.defaultCharWidth(),
oldHeight = editor.getTextHeight(),
scrollPos = editor.getScrollPos();

_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));
// 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)),
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 = scrollPos.y + Math.round(deltaY * (newHeight - oldHeight));

editor.setScrollPos(scrollPosX, scrollPosY);
}
editor.setScrollPos(scrollPosX, scrollPosY);
}

/**
Expand Down Expand Up @@ -158,6 +157,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,6 +177,9 @@ define(function (require, exports, module) {

_setSizeAndRestoreScroll(fsStr, lhStr);

PreferencesManager.setViewState("fontSizeString", fsStr);
PreferencesManager.setViewState("lineHeightString", lhStr);

$(exports).triggerHandler("fontSizeChange", [adjustment, fsStr, lhStr]);
return true;
}
Expand Down Expand Up @@ -215,14 +218,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 +226,18 @@ 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("fontSizeString"),
lhStr = PreferencesManager.getViewState("lineHeightString");
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.

I think we may not need fontSizeAdjustment pref any more with these two new view states. The only place where we still need it is for migration of the old pref from sprint 36 or older sprints.


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.

_addDynamicFontSize(fsStr, lhStr);
}
}



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

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

exports.restoreFontSize = restoreFontSize;
});
⚡