Persisting font size between Brackets Sessions

[lkcampbell]

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.

[WebsiteDeveloper]

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

[lkcampbell]

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.

[peterflynn]

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

[lkcampbell]

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

[RaymondLim]

I had some difficulties in understanding this line. At first, I thought you have "+ 1" by mistake (thinking that you copied this line of code from somewhere else and forgot to remove this part). Later, your comment above this line helped me to realize that your are reverting the exact action done by the caller. So instead of having to know what the caller has done and having to revert the same action here, why don't we make the following changes?

• Return a boolean in this function to indicate whether we actually adjust the font size or not.
• Update the caller code to check the return value of this function call and update the preference only on success.
• Also add an upper bound check here to fix issues Crash - [Win Only]:Continuely Increase Font Size will get APP crashed - few users will encounter #2640 and Increase text size should have a reasonable maximum. #994.
• Finally, check the font size value read from the preference. We can't be 100% sure that we always get the correct font size value from the preference. Update the preference with default font size if restoring font size from preference is failing.

[lkcampbell]

Sounds good to me. Did we ever agree what that upper bound is going to be or do you want me to pick an arbitrary value and we can tweak it from there?

[RaymondLim]

I believe no one has suggested the upper bound value. So you have the power to make that decision.

[lkcampbell]

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.

[lkcampbell]

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

[lkcampbell]

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

[redmunds]

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?

[TomMalbran]

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.

[lkcampbell]

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.

[lkcampbell]

Sorry hit the wrong button :(

[lkcampbell]

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.

[redmunds]

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.

[RaymondLim]

Looks good to me. Merging.