[Themes] Revert theme settings when cancelling out of the themes dialog #8405
Conversation
|
Are all the other settings in the dialog already reverted when you hit Cancel? (That is, bug #8390 only affects the Theme dropdown, and the rest of the dialog was already working correctly?) |
|
All settings, except for the selected theme are only committed to the preferences when pressing done. So there is nothing to revert, with the exception of the selected theme, which I commit upon selection so that users can see what the theme looks like without having to dismiss the dialog. |
|
@peterflynn Short answer yes... Sorry I started with the long answer. I do things backwards sometimes :) |
|
Makes sense, thanks! (Though in the future, for consistency I think we should consider making all the controls in this dialog update in real time -- that would be useful for picking out a good font size, etc. too) |
|
Well, that would fairly easy to do and I think that would really useful too... I will make another PR for that. I didn't do it for the other settings because their result is generally known whereas picking a theme seems to be a bit more trial and error. But I think making all the setting take effect right away would be smoother I think. |
|
@MiguelCastillo @peterflynn Unfortunately, it's probably not that easy, 'cause those changes can be unexpectedly small or big. Let's say one wants to edit 15px to 16px - he'll first delete the trailing 5, so it's only 1px for a short period, which isn't human-readable any more. And I guess these font size updates aren't the right thing to do, as it can affect other states like the scroll position... |
|
@SAplayer You are correct there are implication if you are just arbitrarily changing the values at a rapid pace... That can be mitigated by adding a bit of a throttle. So, we'll just come up with something that makes sense. I was referring to the changes in the themes settings themselves being pretty simple though. Let's stabilize themes first, land this guy #8305 and then worry about saving font settings real time. |
|
You can use onchange for those cases, so that the font updates after blur. But another issue is that the modal dialog is covering most of the screen and makes it hard to see the changes and really play with them. Maybe we could sue a bottom panel instead of a dialog? |
| @@ -121,6 +121,10 @@ define(function (require, exports, module) { | |||
| prefs.set(setting, newSettings[setting]); | |||
| }); | |||
| } | |||
| else if (id === "cancel") { | |||
There was a problem hiding this comment.
This should be: } else if (id === "cancel") {
|
Man, Crock's jslint is going to be the end of me! :D Will change it shortly. |
|
Just use it in Brackets. You'll get used to it and found any other styling weird and realize that jslint does a good job. Having |
|
Yeah, that's just personal preference... I prefer my elses and ifs in their own line. So, I do use the built in linter specifically for Brackets commits. But my team and I all use interactive linter, which uses jshint by default and I forget to switch context when working between my day job and Brackets. :-) |
|
I am only guessing that Crock is really OCD... :D |
MiguelCastillo
changed the title
|
@SAplayer I don't view it s a huge problem if the user briefly sees silly intermediate values shown as they're typing. But as @MiguelCastillo mentioned, it could easily be solved via a small debouncing delay. I'd prefer that over only updating on blur ( @TomMalbran Good point that the dialog makes it tricky to see the editor below it. I wonder if making our dialogs draggable would be a nice solution to this? Probably not too hard, and might benefit other cases too (e.g. unsaved changes dialog -- you might want to look at part of the text below the dialog before deciding, etc.). At any rate -- I agree making those other fields update in real time is not needed for 0.42 :-) |
|
@peterflynn A draggable dialog will not really solve any issue. The panel is still big and you would need to drag it so that part of it is not visible to be able to see most of the screen, which most don't allow it. You also have the black overlay that is not the best for showing the changes. And finally being able to switch documents while checking the preferences would be great, which is not possible with the dialogs. The best solution is to have the preferences as something similar to a custom viewer as an old design made for the extensions manager. |
|
@MiguelCastillo Re linters: Once something like #7362 lands, you'll be able to set project-specific preferences for which linter(s) are enabled :-) |
|
@peterflynn yeah that's awesome. I also have plans to add a way to set which linter Interactive Linter runs for you on a language basis... I have jscs, jslint, and jshint without any good way to configure which you run. It would be even cooler if I could do it per project/directory. :D |
|
Works fine. If we do change to make the other settings update live for preview purposes, we'll need to restore them as well, but this does the job here. Thanks! |
[Themes] Revert theme settings when cancelling out of the themes dialog
|
@dangoor yeah for sure. I just want to make sure we get through show stoppers. But I will make another PR in the next day or two to make the settings live update. Gotta get the integration happen first |
|
@MiguelCastillo No worries about live updating settings. It's just a nice to have, not something required to ship this great feature! |
#8390