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

Fix fontSizeChange event for Restore Font Size command #7443

Merged
merged 3 commits into from
Apr 8, 2014
Merged

Fix fontSizeChange event for Restore Font Size command #7443

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34dbe00
Fix fontSizeChange event for Restore Font Size command
lkcampbell Apr 8, 2014
479ba7b
Stop event from firing twice. Make code work with em units.
lkcampbell Apr 8, 2014
1547c8b
Only fire event when there is an adjustment in font size
lkcampbell Apr 8, 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
15 changes: 11 additions & 4 deletions src/view/ViewCommandHandlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,17 +102,24 @@ define(function (require, exports, module) {
* @param {string=} fontSizeStyle A string with the font size and the size unit
*/
function _setSizeAndRestoreScroll(fontSizeStyle) {
var editor = EditorManager.getCurrentFullEditor(),
oldWidth = editor._codeMirror.defaultCharWidth(),
scrollPos = editor.getScrollPos(),
line = editor._codeMirror.lineAtHeight(scrollPos.y, "local");
var editor = EditorManager.getCurrentFullEditor(),
oldWidth = editor._codeMirror.defaultCharWidth(),
newFontSize = "",
oldFontSize = $(".CodeMirror").css("font-size"),
fontSizeChange = 0,
scrollPos = editor.getScrollPos(),
line = editor._codeMirror.lineAtHeight(scrollPos.y, "local");

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

newFontSize = $(".CodeMirror").css("font-size");
fontSizeChange = parseInt(newFontSize, 10) - parseInt(oldFontSize, 10);
$(exports).triggerHandler("fontSizeChange", [fontSizeChange, newFontSize]);
Copy link
Contributor

Choose a reason for hiding this comment

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

This event is now triggered twice. We should remove the line 163.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lkcampbell If you want to trigger the event here, then you need to remove the one in _adjustFontSize. Otherwise, we're triggering twice for increase/decrease font size scenarios.

Also, you are passing the delta in font size whereas @TomMalbran was passing the actual font size in his changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

fontSizeChange always sends the adjustment and the new font size string. This works fine for font sizes in px, but would fail with font sizes in ems.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lkcampbell and @TomMalbran
I was expecting to have one line addition to _handleRestoreFontSize to trigger event with the following line.

$(exports).triggerHandler("fontSizeChange", [adjustment, DEFAULT_FONT_SIZE + "px"]);

Copy link
Contributor

Choose a reason for hiding this comment

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

That won't work because we don't have the adjustment defined in _handleRestoreFontSize so we need to calculate it in the same way as @lkcampbell did. And since we need to calculate it, using the css style as the font size string is better since we then don't depend on knowing the default font-size or the units and the code will just work if we decide to change any of those.

Copy link
Contributor

Choose a reason for hiding this comment

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

@TomMalbran is saying your current implementation is better, you just need to remove the other trigger event.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TomMalbran, just because I am curious. Has there ever been a situation where we are using a font size that uses ems as its units? I mean, I know that code has been in there for a while but does it ever actually get used?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want it to work with ems too? The rest of the code works with ems too.

Copy link
Contributor

Choose a reason for hiding this comment

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

The first one who implemented this code did it like that. But yes, the default font-size never actually changed. Anyway, maybe in the future it could change or a theme could use a different font-size it or it could be edited by a preference.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should work for both px and ems

delta = /em$/.test(oldFontSize) ? 10 : 1;
adjustment = parseInt((parseFloat(newFontSize) - parseFloat(oldFontSize)) * delta, 10);


// Calculate the new scroll based on the old font sizes and scroll position
var newWidth = editor._codeMirror.defaultCharWidth(),
deltaX = scrollPos.x / oldWidth,
Expand Down