-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PR: Initial support for Spyder's dark theme #8020
Conversation
Hello @dalthviz! Thanks for updating the PR.
Comment last updated on October 14, 2018 at 21:50 Hours UTC |
@dalthviz, please merge with master to fix our tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really, really good!! Thanks @dalthviz!
Now my comments:
- Your first screenshot makes obvious that we need to select a dark theme for the Editor when users select the dark theme for the entire application.
- This means the new option in
Preferences > Global > Appearance
is not really necessary, so please remove it. - What we need to do is to directly select the dark application theme when users select a dark Editor theme in
Preferences > Syntax coloring
(and the same for light themes). This way we'll make both go hand in hand. - A Spyder restart will be required if users change the syntax scheme from light to dark or dark to light. If we go from a dark (light) scheme to another dark (light) scheme, we don't need to inform users that a restart is needed.
- This is the way RStudio works, and I think it's very intuitive.
spyder/utils/icon_manager.py
Outdated
@@ -236,6 +236,220 @@ | |||
'dependency_error': [('fa.warning',), {'color': 'darkred'}], | |||
} | |||
|
|||
_qtaargsdark = { | |||
'log': [('fa.file-text-o',), {'color': 'white'}], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please parametrize color
here, like this:
'log': [('fa.file-text-o',), {'color': MAIN_LIGHT_COLOR}],
and the same for all other keys in this dict, so we can check the new scheme with different shades of white.
Please also define MAIN_LIGHT_COLOR = 'white'
at the top of this file for this.
Fantastic change, of course, and it already looks very good and makes Spyder appear much more modern. If its ready, I can test it more thoroughly on Windows. In fact, I would favor making the dark theme the default on first run (as IIRC Rstudio, Atom and VSCode do) as it makes a better first impression of Spyder being a modern, clean IDE to new users, and also distinctly new and fresh look vs. Spyder 3 (not to mention, @ccordoba12 can't argue I can't make the dark version our hero screenshot on the website this time, as those three editors have on theirs :) ). IMO, inexorably linking the overall UI color and the syntax highlighting background color is not the best choice, particularly for Spyder's specific situation vs. Rstudio. TL;DR: Spyder requires a full restart with all its negative repurcussions unlike Rstudio, vastly increasing the penalty of this approach; Spyder users a more conventional and less minimal UI that looks much less odd when in a different color than the code background; Spyder has user-configurable themes and colors which lead to ambiguity, edge cases and loss of user control if the UI theme is determined automatically; and the contrasting color of the core Editor and Console backgrounds help draw visual focus to what's most important, the code, which some users prefer. (See below for full reasoning). What I suggest to easily obtain the best of both worlds is to have three radio buttons in the unused space below the "scheme" buttons to the right of the syntax highlighting preview, under the heading "UI Theme" or similar, with the options As another minor suggestion, but perhaps not within scope of this PR, it would be really nice to have an icon or other indicator next to each syntax theme indicating whether its light or dark by default, which would be a useful indicator to the user for both picking an appropriate theme they like (light or dark), as well as allowing them to easily determine whether selecting it would require a restart before doing so. Full reasoning for this does not work so well for Spyder, particularly vs. Rstudio (feel free to skip):
|
This is a very good suggestion, but not using radio buttons but a dropdown menu. This is called RStudio theme in RStudio and this is how it looks: @dalthviz, please implement this idea too. |
If we have no plans to add more UI themes anytime soon, or too many options there, radio buttons are quicker and more obvious to interact with than a dropdown. However, if we do plan to add more UI themes beyond light and dark, or other options to the side, a dropdown makes sense.
Yeah, I saw that although it was rather confusing there since I didn't notice any difference between "Modern" and "Sky" and it wasn't clear exactly what it was supposed to change (unlike the wording above). Working off that, we could take that a step further, call that preference pane "Themes", and move the font and window theme/icon theme choices there as well like Rstudio has, since they're also directly theme-related. |
@ccordoba12 @CAM-Gerlach a new preview: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it on Windows 8.1 with a clean prefs file.
Regarding the theme itself:
Many comboboxes with backgrounds (path bar, pylint, profiler, find/replace) had the text cut off like so:Apparently QDarkStyle
-
The background color of the Editor and Console pane, as well as the syntax color scheme preview in preferences, does not change as it should with the "Background" color of the syntax highlighting theme, but rather only the background surrounding the actual text characters. This works properly with the light UI theme. The issue reporting dialog editor has the same problem, except that it always uses the light theme so it looks even worse.Separate issue -
The "Greedy completion" section in IPython console > Advanced Settings was cut offSeparate issue -
Greyed-out text should be lighter, since its almost impossible to even read now; also, links could be a little lighter as well.QDarkStyle -
Places that need to be "darkified":Future PR- Help pane (obviously)
- Pylint/Profiler
Output
window - Dependencies dialog
- Github authentication dialog
- Interactive tour
- Online help and interactive tours submenus (of Help menu)
-
Places that still have black/dark text:Future PR- Find in files results
- File switcher file list
- Symbol finder results
- Keyboard shortcuts
names
column - LSP Manager
Command to Execute
column - Pylint/Profiler date and rating text
Icons that need to be white/light grey:Future PR- "Gear"/pane options icon (completely invisible)
- Regex icon (find in files, find/replace)
Regarding the preference behavior:
-
The "Color theme" dropdown was on
Light
by default, notAutomatic
(as it presumably should be if we take @ccordoba12 's suggestion to behave as Rstudio does at face value). -
"Color theme" should read "UI Theme", as "Color theme" is rather confusing while "UI theme" is more clear as to what it affects".
-
It appears that in terms of asking for a restart, "Dark" is behaving as "Automatic", "Light" is behaving as "Dark", "Automatic" is correct, and there is no setting that behaves as "Light" should. However, their actual effects (once restarted) are correct.
Full behavior description:
The behavior was correct when I switched "Color theme" to "Automatic"; I was prompted to restart when switching to a dark syntax highlighting scheme when the current UI theme was light, while I was not prompted when switching to a light-BG scheme.
However, when "color theme" was set to "Light", the behavior was wrong. When I changed the theme from "Spyder" to "Spyder Dark", I was still presented with a dialog saying that Spyder needed to restart, even though "Color theme" was set to "Light" (and the current UI theme was light). Furthermore, I was even prompted to restart when switching to a light-BG scheme, despite the dropdown, my current UI theme, the selected "Scheme" setting, and the and current syntax highlighting scheme all being light.
Conversely, when set to "Dark", I was not prompted to restart when changing the "Color theme" for the current syntax highlighting scheme, nor when then changing the "Scheme" to a light-BG one, while I was prompted to restart when changing to a dark BG-one.
spyder/preferences/configdialog.py
Outdated
@@ -1171,6 +1174,16 @@ def setup_page(self): | |||
'selected') | |||
self.schemes_combobox = schemes_combobox_widget.combobox | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra linebreak here
spyder/preferences/configdialog.py
Outdated
self.main.editor.apply_plugin_settings(['color_scheme_name']) | ||
if self.main.ipyconsole is not None: | ||
self.main.ipyconsole.apply_plugin_settings( | ||
['color_scheme_name']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indent looks offf; if its hanging it should be 4 spaces to the right of the previous line.
spyder/preferences/configdialog.py
Outdated
['color_scheme_name']) | ||
if self.main.historylog is not None: | ||
self.main.historylog.apply_plugin_settings( | ||
['color_scheme_name']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above indent comment.
spyder/utils/icon_manager.py
Outdated
import qtawesome as qta | ||
|
||
# Main color for a dark theme | ||
MAIN_LIGHT_COLOR = 'white' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be described more clearly, since Main color for a dark theme == Main Light Color is confusing. I suggest describing it as a "Foreground color for a dark theme" instead, and consider renaming MAIN_LIGHT_COLOR
something like DARK_THEME_FG_COLOR
(or LIGHT COLOR
) instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the description I putted is missliding. I will delete it. In that way ´MAIN_LIGHT_COLOR´ is more general concept and maybe could be thought as a property to make the icons look clear or 'light' (in the sens that they contrast with a dark background).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use this variable to define a single icons dictionary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that way ´MAIN_LIGHT_COLOR´ is more general concept and maybe could be thought as a property to make the icons look clear or 'light' (in the sens that they contrast with a dark background).
Okay, sure—that works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use this variable to define a single icons dictionary?
It could be possible, then the validations in the icon function will be in the top when defining the variable which then should be called something like MAIN_COLOR
.
@dalthviz I tested the above before your latest commit of the font color refactor, but just to confirm, a quick retest indicates that all of the above comments/issues still apply, FYI. Also, just curious, how come you use periods in your commit messages? I thought standard convention dictated they were to never be used, and the first line should be kept at or under 70-74 characters (Github cuts them off at 74)? |
Thanks @CAM-Gerlach for your review. My answers:
I think this needs to be solved in QDarkStyle, so please report it there.
I was expecting this. Please open an issue about it.
Open a new issue about it with a screenshot because I don't understand what you mean.
Open an issue in QDarkStyle about it.
Sure, that's why the title says Initial support not Full support. Please remove those places from your comment and open a new issue about them.
Please remove those from your comment and add them to the same issue as the one above.
Please do the same as with the two previous two comments.
@dalthviz, please fix that in this PR.
UI is a term only understood by us, computer geeks. @dalthviz, please rename it to User interface theme.
@dalthviz, pleas review how you implemented this. The logic is the following:
|
@CAM-Gerlach, so the purpose of this PR is to add QDarkStyle as a new dep and implement the logic of switching between light and dark UI themes. All the remaining stuff will be covered later. |
@ccordoba12 Okay, thanks. From the title, I inferred it would be preliminary, but I didn't see anywhere that it actually stated what was in scope and what wasn't for this round, so I figured I'd mention everything in one central place and then separate off what wouldn't be covered here, per your request. Should we create an Epic or something to keep track of all of this? I made the stuff checkboxes so it would be easy to cut and paste into something like that. |
Sure, please create one and:
Thanks! |
Will do! That's all in work now. |
That's something I talked to @dalthviz privately, sorry. |
@CAM-Gerlach I'm used to put periods at the end of what I'm typing (although sometimes I don't use them XD). But overall my commit messages could be improved of course. As an example, my last commit hahaha |
@ccordoba12 @dalthviz What do you think about my preview suggestion, based on your own affinity for Rstudio's system (which this in turn is based off):
This would put everything related to UI and syntax themeing in one place, since we've already moved the UI theme there. Also, a bug related to the preferences pane under direct consideration in this PR: A missing portion of the bounding box where the title used to be: |
It also looks like a QDarkStyle issue. |
That's not work for this PR, so please open a new issue to discuss about it. |
Do you know why its just on that section, and not any other one? All of the sections have left-aligned text in the standard theme that is moved to centered in the dark theme, but there's nothing obvious different about that one that would make it behave like that while none of the others do. What should I tell the QDarkStyle people?
Okay, sure. |
I thought it was only on all sections. @dalthviz, please try to figure that one out. |
Directly related to this issue, |
e53d719
to
c9f26fd
Compare
@ccordoba12 @CAM-Gerlach I think the base funtionality is complete. About the naming of the preferences it would be nice to have a final decision. For me it could be something like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dalthviz, this is looking really good!
My comments:
- Please change
color_theme
forui_theme
wherever necessary. - Please change
MAIN_COLOR
forMAIN_FG_COLOR
(per @CAM-Gerlach's suggestion).
spyder/config/main.py
Outdated
@@ -465,6 +465,7 @@ | |||
}), | |||
('color_schemes', | |||
{ | |||
'color_theme': 'automatic', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this option name to ui_theme
spyder/app/mainwindow.py
Outdated
@@ -553,6 +556,16 @@ def create_toolbar(self, title, object_name, iconsize=24): | |||
def setup(self): | |||
"""Setup main window""" | |||
self.debug_print("*** Start of MainWindow setup ***") | |||
self.debug_print(" ..theme configuration") | |||
color_theme = CONF.get('color_schemes', 'color_theme') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
color_theme
-> ui_theme
spyder/preferences/configdialog.py
Outdated
@@ -1171,17 +1171,29 @@ def setup_page(self): | |||
'selected') | |||
self.schemes_combobox = schemes_combobox_widget.combobox | |||
|
|||
# Layouts | |||
vlayout = QVBoxLayout() | |||
color_themes = ['Automatic', 'Light', 'Dark'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
color_themes
-> ui_themes
spyder/utils/icon_manager.py
Outdated
Get main color for the icons based on the color theme | ||
and color scheme currently selected. | ||
""" | ||
color_theme = CONF.get('color_schemes', 'color_theme') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
color_theme
-> ui_theme
spyder/utils/icon_manager.py
Outdated
return main_color | ||
|
||
|
||
MAIN_COLOR = get_main_color() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MAIN_COLOR
-> MAIN_FG_COLOR
spyder/utils/icon_manager.py
Outdated
import qtawesome as qta | ||
|
||
|
||
def get_main_color(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_main_color
-> get_foreground_color
spyder/utils/icon_manager.py
Outdated
color_theme = CONF.get('color_schemes', 'color_theme') | ||
color_scheme = CONF.get('color_schemes', 'selected') | ||
if color_theme == 'dark': | ||
main_color = 'white' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main_color
-> foreground_color
spyder/preferences/configdialog.py
Outdated
# Layouts | ||
vlayout = QVBoxLayout() | ||
color_themes = ['Automatic', 'Light', 'Dark'] | ||
color_theme_choices = list(zip(color_themes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
color_theme_choices
-> ui_theme_choices
spyder/preferences/configdialog.py
Outdated
color_theme_choices = list(zip(color_themes, | ||
[color_theme.lower() | ||
for color_theme in color_themes])) | ||
color_theme_combo = self.create_combobox(_('User interface theme:'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
color_theme_combo
-> ui_theme_combo
spyder/preferences/configdialog.py
Outdated
self.update_combobox() | ||
self.update_preview() | ||
color_scheme = self.get_option('selected') | ||
color_theme = self.get_option('color_theme') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
color_theme
-> ui_theme
I really like that, has a nice ring to it :) Only problem is that
Too long, IMO, and that's coming from me [laughs in English] :) In #8071 , based off my earlier comments here, I suggested renaming it "Themes" after moving the other theme-related settings (fonts, Qt window theme, icon theme) to this pane as well, but "Colors and Themes" could be a more descriptive alternative. |
21178bf
to
9ce1c0a
Compare
@CAM-Gerlach, please add more comments to the Epic issue and not here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @dalthviz!! This is really great work!
We'll close issue #2350 when we solve all issues referenced by @CAM-Gerlach in the epic he opened. |
Pull Request Checklist
Developer Certificate of Origin Affirmation
By submitting this Pull Request or typing my name below, I affirm the
I certify the above statement is true and correct: dalthvizDeveloper Certificate of Origin
with respect to both the content of the contribution itself and this post,
and understand I am releasing it under Spyder's MIT (Expat) license.
Description of Changes
Add initial support for Spyder's dark theme settings
Preview using
Monokai
color schemeIssue(s) Resolved