Set the font size before opening the documents #7185

TomMalbran · 2014-03-13T02:28:15Z

This is a fix for issue #7093. The idea is to add the styles with the new font-size and line-height before opening any document. To do this, when changing the font-size, we now save the strings that we use to set the styles, so we can use them to set the styles without having any editor open.

RaymondLim · 2014-03-13T23:49:00Z

src/view/ViewCommandHandlers.js

+     * @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


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

RaymondLim · 2014-03-14T02:13:20Z

I verified that #7093 is fixed for any scroll positions only if Word Wrap is off. We may need to fix #7168 sooner since changes in this pull request make the scroll position off by the number of wrapped lines.

Done with review and will merge after you fix the JSDoc nits.

TomMalbran · 2014-03-14T02:33:07Z

This does fix all the reload issues, with or without word wrap on. And I checked with the steps in issue #7168 and is fixed too.

The issue that this doesn't fix yet is increasing the font-size with Word Wrap on, but the issue there is in how we restore the scroll position. I need to check later if I can fix that using some of the new CM apis.

RaymondLim · 2014-03-14T02:50:51Z

Yes, I meant #7168 with some font-size adjustment. I didn't remember that I filed #7168 without any font-size adjustment.

TomMalbran · 2014-03-14T03:14:46Z

@RaymondLim I fixed the comments and removed the fontSizeAdjustment view state since we don't really need it anymore. I also added a fix for the word wrap issue, but it does't work when an inline editor is partially visible at the top of the screen, but might not be that bad of an issue. Let me know if the fix works for you.

RaymondLim · 2014-03-14T05:08:48Z

src/view/ViewCommandHandlers.js

+        var editor    = EditorManager.getCurrentFullEditor(),
+            oldWidth  = editor._codeMirror.defaultCharWidth(),
+            scrollPos = editor.getScrollPos(),
+            line      = editor._codeMirror.lineAtHeight(scrollPos.y, "local");


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?

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

Actually I made this PR after cmv4 landed.

Really, but I notice that CM submodule is different.

Yes, I can use multiple cursors in this branch.

TomMalbran · 2014-03-14T06:46:07Z

@RaymondLim Changes pushed.

TomMalbran · 2014-03-14T06:47:25Z

src/view/ViewCommandHandlers.js

+            lhStr = PreferencesManager.getViewState("lineHeightStyle");
+
+        if (fsStr && lhStr) {
+            _removeDynamicFontSize();


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.

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

RaymondLim · 2014-03-14T07:21:05Z

@TomMalbran My changes are pushed. I'm done reviewing your changes and it's your turn to review mine. @dangoor Can you also review my changes in PreferenceStorage.js?

TomMalbran · 2014-03-14T07:59:52Z

@RaymondLim The ViewCommands code looks good. I am not that familiar with the new Preferences code to know if your changes are good or not.

Edit: nvm the last comment, we only convert from the local storage.

RaymondLim · 2014-03-14T16:58:25Z

@TomMalbran Yes, we're migrating only from the local storage. But your point is valid since this pull request is not going to release 37 and we may have a gap in migrating "fontSizeAdjustment" from release 37. Maybe we should stick with "fontSizeAdjustment" and just recalculate font size and line height in your new restoreFontSize function.

MiguelCastillo · 2014-03-14T17:22:48Z

This may or may not be the proper place for this question. But would it be possible to expose the font sizing functionality as an API? I have added the ability to change the font size and type in Themes and it would be really smooth if I could leverage Brackets API instead of Themes completely taking over it. Any thoughts?

TomMalbran · 2014-03-14T18:33:14Z

@RaymondLim That can be an option, but we would need to add the default font size as a new constant and remember to change it if we ever change it in the CSS. Anyway we already have that problem with the line height. Another option would be to do a 1 time conversion from the fontSizeAdjustment state view?

@MiguelCastillo To increase or decrease the font size, you could always call the commands. Or maybe even change the view states.

MiguelCastillo · 2014-03-14T18:45:22Z

@TomMalbran Yeah I saw I could do that. I was looking for a direct way to set a specific font size.

TomMalbran · 2014-03-14T18:53:32Z

@MiguelCastillo Wouldn't you just set the font size in the CSS, and have a function to increase it? This code should work for themes with other font-sizes.

@RaymondLim I just realized that it will be a bad idea to have the default font-size here, since we would have problems with themes that set the font-size to a different value in the CSS. We can get rid of the line height part by just setting the line height to 1.25 directly in the CSS, and other themes can set it to other unitless-values if required.

MiguelCastillo · 2014-03-14T18:57:17Z

Ok, so here is how themes is working. I have an input field where you can set a particular font size you want to use. There is no button to increase/decrease the size. So, is there a way to tell bracket to set the font size to a specific number/unit? If not, that's no problem. I will just continue to override the style that brackets puts in the document.

RaymondLim · 2014-03-14T19:00:27Z

@TomMalbran I like the new view states for saving us from recalculating font size and line height, but I don't like the fact that they are two independent view states. Instead of "fontSizeStyle" and "lineHeightStyle", we should group them just like we have with "linting" preferences. I know that linting preferences have some issues that @dangoor is looking at. @dangoor Can you suggest which solution we should follow?

TomMalbran · 2014-03-14T19:11:12Z

@RaymondLim I don't like the way linting was done, since we end up using a . for both extension preferences and default ones. We have lots of preferences without adding a . (like all the editor ones) and some with it. We aren't consistent with it.

Anyway, what about just removing the whole line height part and place it into the CSS? if we just set it to 1.25, then we wouldn't need to update it everytime we update the font-size.

@MiguelCastillo No we don't have that function. If _adjustFontSize was exported, you could use it by passing the difference between the theme's default font-size and the new value. WE would need to change the function for it to accept a font-size and calculate the adjustment. Anyway, ins't it easier to just use the increase/decrease commands? Or maybe you could make a PR that would let you change the value (a menu item that makes a dialog appear, where you set the font-size).

MiguelCastillo · 2014-03-14T19:18:04Z

@TomMalbran Yeap, the dialog to set the specific font size is what I have added in Themes as a configurable setting. So, if brackets provided that then there is no need for that setting in themes. Are the values that are stored by brackets in the preference font sizes or font increments?

TomMalbran · 2014-03-14T19:24:58Z

@MiguelCastillo The current stored values are font increments, but this PR will change them to font sizes which include the units, so it would save something like 15px. Even with that, we never assume a default font-size, so we calculate the font-size using an increment. Anyway we can get the default font-size in the same was as it is done in _adjustFontSize (once an editor is open).

TomMalbran · 2014-03-14T19:29:17Z

@MiguelCastillo Or maybe even easier would be to use _setSizeAndRestoreScroll(fsStr, lhStr); passing the parsed font-size and line-height.

MiguelCastillo · 2014-03-14T19:35:08Z

@TomMalbran we are now thinking the same thing. :) Would you like me to open a PR?

TomMalbran · 2014-03-14T19:36:57Z

@MiguelCastillo If you want to. But might be easier to do after we merge this one.

MiguelCastillo · 2014-03-14T19:45:52Z

@TomMalbran I will do a PR. Thanks dude!

RaymondLim · 2014-03-14T20:11:32Z

@TomMalbran I like the idea of removing the whole line height part and place it into the CSS. Can you make that change? I'll add the code to do the one-time conversion.

TomMalbran · 2014-03-14T20:13:46Z

@RaymondLim Yes, I can do it soon.

RaymondLim · 2014-03-14T20:39:29Z

@TomMalbran Actually, can you put the following change in your commit as it also depends on your changes?

    function restoreFontSize() {
        var fsStr = PreferencesManager.getViewState("fontSizeStyle"),
            fsAdjustment = PreferencesManager.getViewState("fontSizeAdjustment");

        if (fsAdjustment) {
            // Always remove the old view state even if we also have the new view state.
            PreferencesManager.setViewState("fontSizeAdjustment");

            if (!fsStr) {
                // Migrate the old view state to the new one.
                fsStr = (12 + fsAdjustment) + "px";
                PreferencesManager.setViewState("fontSizeStyle", fsStr);
            }
        }

        if (fsStr) {
            _removeDynamicFontSize();
            _addDynamicFontSize(fsStr);
        }
    }

TomMalbran · 2014-03-14T21:29:40Z

@RaymondLim I removed the line height and added your code.

TomMalbran · 2014-03-14T21:31:29Z

src/styles/brackets_theme_default.less

@@ -144,10 +144,8 @@
 */
 .code-font() {
    color: @content-color;
-    // line-height must be specified in px not em because the code font and line number font sizes are different.
-    // Sizing via em will cause the code and line numbers to misalign
-    line-height: 15px;


This comment is no longer valid, since the line numbers are now in the same div as the rest of the line. Also if we wanted to decrease the font-size value of the line numbers, we can easily increase the line-height value.

dangoor · 2014-03-14T21:41:30Z

@RaymondLim that preference conversion seems fine to me.

RaymondLim · 2014-03-14T21:53:25Z

src/view/ViewCommandHandlers.js

@@ -231,6 +195,30 @@ define(function (require, exports, module) {
        }
    }

+    /**
+     * Restores the Font Size and Line Height using the saved strings


Nit: We can take out Line Height here.

RaymondLim · 2014-03-14T21:58:05Z

@TomMalbran Done with re-review. ready to merge after you fix the minor nit and the magic number.

TomMalbran · 2014-03-14T22:14:41Z

@RaymondLim Fixes pushed

RaymondLim · 2014-03-14T22:35:19Z

src/view/ViewCommandHandlers.js

+     * @return {Object} JSON object for the new view state equivalent to
+     *      the old "fontSizeAdjustment" preference.
+     */
+    function _convertToNewViewStates(key, value) {


Nice job in cleaning. Just one more minor nit since we only have one new view state, can you also rename this to singular _convertToNewViewState and update the callback argument in convertPreferences call below?

TomMalbran · 2014-03-14T23:04:44Z

@RaymondLim done

RaymondLim · 2014-03-14T23:07:11Z

Nice job! I really like the changes to new view state. Merging now.

Set the font size before opening the documents

Set the font size before openning the documents

79ccbe9

dangoor assigned RaymondLim Mar 13, 2014

RaymondLim reviewed Mar 13, 2014
View reviewed changes

Fixes after Review

a5a6394

RaymondLim reviewed Mar 14, 2014
View reviewed changes

More changes after review

ebb6b7a

TomMalbran reviewed Mar 14, 2014
View reviewed changes

Migrate "fontSizeAdjustment" to new view states.

8d0abbd

Move the line height to the CSS

25ee232

TomMalbran reviewed Mar 14, 2014
View reviewed changes

RaymondLim reviewed Mar 14, 2014
View reviewed changes

Fixes after last review

bb3e75d

RaymondLim reviewed Mar 14, 2014
View reviewed changes

convert to new view state (with no s)

3959dd0

RaymondLim added a commit that referenced this pull request Mar 14, 2014

Merge pull request #7185 from adobe/tom/issue-7093

8971aed

Set the font size before opening the documents

RaymondLim merged commit 8971aed into master Mar 14, 2014

RaymondLim deleted the tom/issue-7093 branch March 14, 2014 23:07

MiguelCastillo mentioned this pull request May 8, 2014

Themes in Brackets core #7616

Closed

8 tasks

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Set the font size before opening the documents brackets-archive/bracketsIssues#6496

Open

Set the font size before opening the documents #7185

Set the font size before opening the documents #7185

Conversation

TomMalbran commented Mar 13, 2014

Choose a reason for hiding this comment

RaymondLim commented Mar 14, 2014

TomMalbran commented Mar 14, 2014

RaymondLim commented Mar 14, 2014

TomMalbran commented Mar 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomMalbran commented Mar 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RaymondLim commented Mar 14, 2014

TomMalbran commented Mar 14, 2014

RaymondLim commented Mar 14, 2014

MiguelCastillo commented Mar 14, 2014

TomMalbran commented Mar 14, 2014

MiguelCastillo commented Mar 14, 2014

TomMalbran commented Mar 14, 2014

MiguelCastillo commented Mar 14, 2014

RaymondLim commented Mar 14, 2014

TomMalbran commented Mar 14, 2014

MiguelCastillo commented Mar 14, 2014

TomMalbran commented Mar 14, 2014

TomMalbran commented Mar 14, 2014

MiguelCastillo commented Mar 14, 2014

TomMalbran commented Mar 14, 2014

MiguelCastillo commented Mar 14, 2014

RaymondLim commented Mar 14, 2014

TomMalbran commented Mar 14, 2014

RaymondLim commented Mar 14, 2014

TomMalbran commented Mar 14, 2014

Choose a reason for hiding this comment

dangoor commented Mar 14, 2014

Choose a reason for hiding this comment

RaymondLim commented Mar 14, 2014

TomMalbran commented Mar 14, 2014

Choose a reason for hiding this comment

TomMalbran commented Mar 14, 2014

RaymondLim commented Mar 14, 2014