[CLOSED] Persisting font size between Brackets Sessions

[core-ai-bot]

Issue by lkcampbell
Sunday Mar 03, 2013 at 07:37 GMT
Originally opened as adobe/brackets#3027

---

Here is the code to persist font size between Brackets Sessions per this discussion:

https://groups.google.com/forum/#!topic/brackets-dev/rYDR13sbhnk

In addition, I noticed a bug where the Increase, Decrease, and Restore Font Size commands remain enabled when there is no current document open in the editor. When I executed the commands, it threw errors into the Dev Tools console.

I fixed the bug but there is still a small problem I can't figure out. When the commands are disabled, I am still able to use the shortcut key for Decrease Font Size (Ctrl--) and throw a different error into the Dev Tools console. I don't know why it is still firing off after I disabled the command. Doesn't happen with the other two disabled commands. Any assistance or insight on this problem would be appreciated.

-- Lance.

---

lkcampbell included the following code: https://github.com/adobe/brackets/pull/3027/commits

[core-ai-bot]

Comment by WebsiteDeveloper
Sunday Mar 03, 2013 at 08:16 GMT

---

i tested this and get an error like this Uncaught TypeError: Cannot call method 'state' of undefined when no document is open
i investigated this a bit an found a fix. It seems that the CommandManager execute method returns undefined if the Command is disabled, you can fix that by returning a already rejected promise like this:

return (new $.Deferred()).reject().promise()

as to the persistent font size, it works great so thank you for that feature

[core-ai-bot]

Comment by lkcampbell
Sunday Mar 03, 2013 at 16:33 GMT

---

Thanks for finding the source of the CommandManager problem.

It is odd that the behavior is different with one of the shortcuts but not the other two. I would expect the CommandManager to deal with all disabled commands without throwing an error. I will put together a repro and report a bug on this behavior.

[core-ai-bot]

Comment by peterflynn
Friday Mar 08, 2013 at 18:32 GMT

---

@lkcampbell were you planning to back out the trailing whitespace diffs here too?

[core-ai-bot]

Comment by lkcampbell
Saturday Mar 09, 2013 at 02:48 GMT

---

@peterflynn I added the "empty line" whitespace back in. I think there was only one or two I couldn't get to behave quite right but the diff is much cleaner now.

The only other white space cleanups I kept are on the license. Those are legitimate cleanups.

[core-ai-bot]

Comment by lkcampbell
Saturday Mar 09, 2013 at 19:21 GMT

---

I have at least one more commit I want to do. I will let you all know when it is ready to look at again.

[core-ai-bot]

Comment by lkcampbell
Saturday Mar 09, 2013 at 20:42 GMT

---

@RaymondLim

I selected font size 72 as the max. It's ridiculously large and also the font size limit on Windows Notepad.

I addressed every bullet you mentioned directly and I believe I addressed the last bullet indirectly. The preference value fontSizeAdjustment has a default of zero already, so even if it isn't defined it will be zero. If the value is really large or really small, the min and max font size will take care of that situation. I believe that takes care of all of the possible values but let me know if there is a value I missed.

[core-ai-bot]

Comment by lkcampbell
Saturday Mar 09, 2013 at 20:50 GMT

---

Okay, it's ready to check again, thanks.

[core-ai-bot]

Comment by redmunds
Saturday Mar 09, 2013 at 21:27 GMT

---

Why do we need a max font size?

If we do, I think a max font size of 72 (px) is not large enough. It might seem ridiculously large on your device, but 4K resolution monitors are starting to come out, so it won't seem so large on those. Maybe max font size is the height of the Brackets window?

[core-ai-bot]

Comment by TomMalbran
Saturday Mar 09, 2013 at 21:32 GMT

---

I believe this #2640 is the main reason for a max font size. So it could be good to investigate at which size brackets crashes and use a size smaller than that, and maybe check if other things affect this too.

[core-ai-bot]

Comment by lkcampbell
Saturday Mar 09, 2013 at 21:33 GMT

---

Ah, the slippery slope of selecting arbitrary values :).

The max font size is to address the following issues:

Also add an upper bound check here to fix issues #2640 and #994.

[core-ai-bot]

Comment by lkcampbell
Saturday Mar 09, 2013 at 21:34 GMT

---

Sorry hit the wrong button :(

[core-ai-bot]

Comment by lkcampbell
Saturday Mar 09, 2013 at 21:53 GMT

---

Perhaps we are running into a bit of scope creep on this pull request.

If you like, I can roll back the max size code and just submit the font size persistence part.

Then, after the merge, I can look into the max font size problem.

[core-ai-bot]

Comment by redmunds
Saturday Mar 09, 2013 at 22:01 GMT

---

OK, I guess 72 is a reasonable starting point. Ultimately, it needs to be defined in a json file, but we don't yet have default user preferences setup in a json file.

[core-ai-bot]

Comment by RaymondLim
Monday Mar 11, 2013 at 03:01 GMT

---

Looks good to me. Merging.