[CLOSED] Set the font size before opening the documents

[core-ai-bot]

Issue by TomMalbran
Thursday Mar 13, 2014 at 02:28 GMT
Originally opened as adobe/brackets#7185

---

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.

---

TomMalbran included the following code: https://github.com/adobe/brackets/pull/7185/commits

[core-ai-bot]

Comment by RaymondLim
Friday Mar 14, 2014 at 02:13 GMT

---

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.

[core-ai-bot]

Comment by TomMalbran
Friday Mar 14, 2014 at 02:33 GMT

---

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.

[core-ai-bot]

Comment by RaymondLim
Friday Mar 14, 2014 at 02:50 GMT

---

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

[core-ai-bot]

Comment by TomMalbran
Friday Mar 14, 2014 at 03:14 GMT

---

@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.

[core-ai-bot]

Comment by TomMalbran
Friday Mar 14, 2014 at 06:46 GMT

---

@RaymondLim Changes pushed.

[core-ai-bot]

Comment by RaymondLim
Friday Mar 14, 2014 at 07:21 GMT

---

@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?

[core-ai-bot]

Comment by TomMalbran
Friday Mar 14, 2014 at 07:59 GMT

---

@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.

[core-ai-bot]

Comment by RaymondLim
Friday Mar 14, 2014 at 16:58 GMT

---

@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.

[core-ai-bot]

Comment by MiguelCastillo
Friday Mar 14, 2014 at 17:22 GMT

---

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?

[core-ai-bot]

Comment by TomMalbran
Friday Mar 14, 2014 at 18:33 GMT

---

@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.

[core-ai-bot]

Comment by MiguelCastillo
Friday Mar 14, 2014 at 18:45 GMT

---

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

[core-ai-bot]

Comment by TomMalbran
Friday Mar 14, 2014 at 18:53 GMT

---

@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.

[core-ai-bot]

Comment by MiguelCastillo
Friday Mar 14, 2014 at 18:57 GMT

---

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.

[core-ai-bot]

Comment by RaymondLim
Friday Mar 14, 2014 at 19:00 GMT

---

@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?

[core-ai-bot]

Comment by TomMalbran
Friday Mar 14, 2014 at 19:11 GMT

---

@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).

[core-ai-bot]

Comment by MiguelCastillo
Friday Mar 14, 2014 at 19:18 GMT

---

@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?

[core-ai-bot]

Comment by TomMalbran
Friday Mar 14, 2014 at 19:24 GMT

---

@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).

[core-ai-bot]

Comment by TomMalbran
Friday Mar 14, 2014 at 19:29 GMT

---

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

[core-ai-bot]

Comment by MiguelCastillo
Friday Mar 14, 2014 at 19:35 GMT

---

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

[core-ai-bot]

Comment by TomMalbran
Friday Mar 14, 2014 at 19:36 GMT

---

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

[core-ai-bot]

Comment by MiguelCastillo
Friday Mar 14, 2014 at 19:45 GMT

---

@TomMalbran I will do a PR. Thanks dude!

[core-ai-bot]

Comment by RaymondLim
Friday Mar 14, 2014 at 20:11 GMT

---

@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.

[core-ai-bot]

Comment by TomMalbran
Friday Mar 14, 2014 at 20:13 GMT

---

@RaymondLim Yes, I can do it soon.

[core-ai-bot]

Comment by RaymondLim
Friday Mar 14, 2014 at 20:39 GMT

---

@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);
        }
    }

[core-ai-bot]

Comment by TomMalbran
Friday Mar 14, 2014 at 21:29 GMT

---

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

[core-ai-bot]

Comment by dangoor
Friday Mar 14, 2014 at 21:41 GMT

---

@RaymondLim that preference conversion seems fine to me.

[core-ai-bot]

Comment by RaymondLim
Friday Mar 14, 2014 at 21:58 GMT

---

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

[core-ai-bot]

Comment by TomMalbran
Friday Mar 14, 2014 at 22:14 GMT

---

@RaymondLim Fixes pushed

[core-ai-bot]

Comment by TomMalbran
Friday Mar 14, 2014 at 23:04 GMT

---

@RaymondLim done

[core-ai-bot]

Comment by RaymondLim
Friday Mar 14, 2014 at 23:07 GMT

---

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