Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WiP] PR: Add a menu entry to select between available color schemes #8426

Closed
wants to merge 1 commit into from

Conversation

chends888
Copy link

@chends888 chends888 commented Dec 12, 2018

Description of Changes

I added two options in the 'Source menu' for alternating between two schemes, Spyder and Spyder Dark. This is a bit hardcoded, I could improve (like making the two opting merge into one, making spyder check the current scheme and applying the oposite color) in case reviewers get interested :D

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot (if affecting the UI)

spyderissue8317

Issue(s) Resolved

Fixes #8317

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: Chends888

@pep8speaks
Copy link

Hello @chends888! Thanks for submitting the PR.

Line 1086:39: E128 continuation line under-indented for visual indent
Line 1088:39: E128 continuation line under-indented for visual indent
Line 1092:39: E128 continuation line under-indented for visual indent

@jnsebgosselin
Copy link
Member

jnsebgosselin commented Dec 13, 2018

Thank you very much for your PR!

However, I think @ccordoba12's idea was to "add an entry to the Source menu with all color schemes available so users can easily select from them", not to add an option for switching between a dark or light scheme.

@ccordoba12
Copy link
Member

However, I think @ccordoba12's idea was to "add an entry to the Source menu with all color schemes available so users can easily select from them", not to add an option for switching between a dark or light scheme.

Yes, that was my suggestion because there's no single dark or light color scheme we could go to.

@chends888
Copy link
Author

Nice, thanks for the feedback, I can work on that the next few days if you're interested on seeing a list of current schemes on the Source Menu. Just let me know if it'll really be useful for Spyder

@CAM-Gerlach CAM-Gerlach changed the title Fixes issue#8317 [WiP] PR: Add a menu entry to select between available color schemes Dec 13, 2018
@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Dec 13, 2018

As others have noted, realistically the way to implement this is a submenu in the Source menu showing all available color schemes. However, I'm not sure if it'd really be worth doing, in terms of addressing the use case requested by @abudden , which was a quick way to switch between light and dark syntax themes.

First, the user would still have to open the menu, navigate to the submenu, and scroll through the (soon to be much longer, thanks to #8381 ) list of schemes to find the one they want, and do the same thing to switch back; meanwhile, tapping ``Ctrl-Alt-Shift-P`` to summon preferences, selecting the scheme from a menu and hitting apply doesn't take all that much longer.

This is particularly true in light of the (at least on my machine) 20-30 second delay needed to apply the scheme, so the at most a few seconds it would save are inconsequential, to say nothing of the full restart required to actually switch from the light to the dark mode and back (which would presumably be the case if the user switched from a light to a dark theme as the original reporter wanted to with the UI Theme set to Automatic, unless this has special logic to override that (which could even cause user confusion later). Therefore, unless the huge time lag can be resolved (even cutting it by 1/2 would save more time than a submenu vs. preferences), I'm not sure I understand how this really adds value for the stated use case.

@jnsebgosselin
Copy link
Member

Nice, thanks for the feedback, I can work on that the next few days if you're interested on seeing a list of current schemes on the Source Menu. Just let me know if it'll really be useful for Spyder

Just to be sure, what we are talking about is adding a single subemenu named Color schemes to the Source menu, not adding the complete list directly to the Source menu.

I think this is worth it and would be a nice addition to Spyder if you have the time to do it.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Dec 13, 2018

adding a single subemenu named Color schemes to the Source menu

That was my impression as well; adding all the syntax highlighting schemes (especially after #8381 ) would might not even fit on the screen (at least for 720p). Also, if we do it, it should be named Syntax highlighting scheme for clarity, consistency and idiomatic correctness.

I guess my objection is I don't see the point of a feature that saves negligible time relative to the switching overhead and adds edge cases and therefore potential bugs (needs to handle Color Scheme Automatic and switching from light to dark, stay in sync with prefs changes, etc) , when e.g. optimizing the scheme switching routine to not take 20-30 seconds is likely going to save far more time for users than a few seconds opening preferences. However, there isn't *really* a harm in it if implemented correctly, other than adding one more item in an already very messy Source menu.

@jnsebgosselin
Copy link
Member

@CAM-Gerlach The relevance of this feature was kind of already established in the associated Issue. Your arguments are admissible though, so maybe they could be synthesized a little and added to the Issue, so that we can try to keep the PR focused more on the implementation details instead.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Dec 14, 2018

The relevance of this feature was kind of already established in the associated Issue.

My argument is that this specific implementation (submenu for all color schemes in the source menu, no shortcuts, list pruning or apply speed improvements) as it stands doesn't really solve the issue discussed in #8381 , wanting to be able to more quickly toggle between dark and light syntax themes.

It is essentially no faster than the current alternative, particularly relative to the massive delay (~10 seconds on a clean prefs file, 20-30s on a dirty one, if the results on my i7/4core/8thread/24GB/SSD/Windows machine are representative) needed to apply a theme.

In my testing, opening the preferences with the existing shortcut, selecting the theme, and hitting enter/okay took essentially the same time as navigating to the Source menu, moving to the submenu, finding the theme in the (soon to be much longer) list (I simulated it with an appropriate length warnings/errors list), and clicking it both took between 5-6 seconds per attempt, averaged over 3 attempts each with one practice run (excluding the compute time to apply the syntax theme), with the Source menu approach taking within a second of the preferences one. Therefore, I assert that it seems this implementation as it stands now (without shortcuts, pruning the displayed theme list to the ~5 most recently used themes, and/or speeding up the time needed to apply a theme) does not seem to fulfill the quite valid use case outlined in the issue, and @ mentioned the requester of the feature so they could comment for themselves as to whether it does.

@jnsebgosselin
Copy link
Member

I quite got your point the first time you posted thank you. Repeating the same argument over and over again with that level of unnecessary details is a waste of my time and yours and is quite irritating.

The very idea you are objecting to was proposed by @ccordoba12 in Issue #8317 as a valid solution to solve the problem and no one has objected to that over there... So this PR should have been very straightforward, with maybe a suggestion or two to improve it, like your idea of list pruning. The only reason I posted here in the first place was because the implementation was not respecting what it was agreed and discussed in the issue.

@gepcel
Copy link
Contributor

gepcel commented Dec 14, 2018

I'm not a developer, but just a normal user. From my using scenario, it'll be helpful to have a submenu to switch color schemes (without considering the implementation or delay detail), but it won't hurt without it. I can change that via preferences, and an even better thing is I can preview the effect via preferences.

What will be very helpful is a convenient way to switch between dark mode and light mode by user base on the time or the environment. Maybe one can select one color scheme as the default light mode and one as the default dark mode.

@ccordoba12
Copy link
Member

What will be very helpful is a convenient way to switch between dark mode and light mode by user base on the time or the environment. Maybe one can select one color scheme as the default light mode and one as the default dark mode.

The idea is kind of interesting but I don't know how to implement it graphically and how other people (who doesn't know the context or the purpose of that) would understand the meaning of default light and dark themes.


In all fairness (and with my sincere apologies to @chends888), I think we should stop here and not implement this feature (at least with my initial suggestion) given @CAM-Gerlach's and @gepcel's remarks.

Besides, @gepcel's initial remark was that this would be useful to do presentations. So, unless you give presentations like crazy, I think it's simpler and more intuitive to simply go to Preferences and select your scheme there.

@jnsebgosselin
Copy link
Member

I think we should stop here and not implement this feature (at least with my initial suggestion) given @CAM-Gerlach's and @gepcel's remarks.

I agree with this. I think Issue #8317 is still a valid one though and we should all go back brainstorming over there. Maybe we can find a better solution.

@CAM-Gerlach
Copy link
Member

In all fairness (and with my sincere apologies to @chends888), I think we should stop here and not implement this feature (at least with my initial suggestion) given @CAM-Gerlach's and @gepcel's remarks.

I agree with this. I think Issue #8317 is still a valid one though and we should all go back brainstorming over there. Maybe we can find a better solution.

Agreed on all points with you both.

Sorry for the excessive detail and repetitiveness. I wasn't around for the issue discussion and thus I was reviewing this PR on its own merits as to whether it actually fixed the issue it was created to resolve. I'll remove my comments here and post a more concise version over there on that thread, and look forward to discussing it further.

@jnsebgosselin
Copy link
Member

Thank you @CAM-Gerlach, it is much appreciated!

@ccordoba12
Copy link
Member

Sorry for making you waste your time @chends888.

@ccordoba12 ccordoba12 closed this Dec 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants