PR: Modify color scheme preferences #2861

goanpeca · 2015-12-08T03:31:48Z

Related to #820

Description

This PR aims to simplify the user experience for editing the color schemes.
It also incorporates the creation/deletion of an unlimited number of custom schemes.

This PR however does not (yet) add the custom schemes to the options in the other plugins (editor etc...) as the idea is to unify the different color schemes into a single global one, located in this updated preferences page. This will be accomplished in a subsequent PR.

The custom name is generated automatically (custom-N) but are modifiable. For default schemes colors are modifiable, but the scheme name is not.

Final Screenshots

goanpeca · 2015-12-08T03:43:24Z

Initial screenshots

@Nodd, @SylvainCorlay, @blink1073 comments are welcome

goanpeca · 2015-12-08T04:21:08Z

spyderlib/config/gui.py

@@ -162,6 +162,3 @@ def set_default_color_scheme(name, replace=True):

 for _name in sh.COLOR_SCHEME_NAMES:
    set_default_color_scheme(_name, replace=False)
-CUSTOM_COLOR_SCHEME_NAME = "Custom"
-set_color_scheme(CUSTOM_COLOR_SCHEME_NAME, sh.get_color_scheme("Spyder"),
-                 replace=False)


There is no point in having a custom scheme now

Ok, no problem with that :-)

blink1073 · 2015-12-08T15:12:02Z

I love the comments on your own PR, very schizophrenic. I am +100 on this change.

goanpeca · 2015-12-08T16:03:40Z

Hehe, I wanted to save some time, and I was almost sure someone had something to say in that respect,

Would you mind testing it @blink1073 ?

ccordoba12 · 2015-12-08T16:24:51Z

A couple of quick suggestions:

I don't like the Create new scheme button to expand the two columns. I'd like to see it at the bottom but only on the left column (i.e. the buttons column :-)
In the dropdown menu please change Scheme to Scheme:

goanpeca · 2015-12-08T16:30:08Z

👍

Nodd · 2015-12-08T19:18:42Z

The first thing I thought when viewing the screenshots is that the preview is really a great addition. Just the line numbers are missing.

About the edition of the colors, the entries should be grouped differently, ideally with many group widgets. The minimum would be to group the text values together so that the checkboxes are not all over the place. One group for "text" and one for "background" for example. Or "text", "text highlighting" and "background".

goanpeca · 2015-12-09T08:47:32Z

Look update

@ccordoba12, and for @Nodd ... numbers

@Nodd

Plus now we have instant preview, so any changes on an edited theme will reflect in the preview right away (without the need to apply the changes)

goanpeca · 2015-12-09T09:08:45Z

It is looking pretty good already, but I guess the final touch would be to have the preview in the editor dialog, and have it update in every single change inside the actual dialog.

What do you think?

Nodd · 2015-12-09T09:50:43Z

Here is my habitual flow of nitpicks:

Maybe current cell and current line should be in the Background group, since the are applied to blocks of code and not particular words or characters.
Background group could be before Highlight group since it's more general (not sure about this one)
Normal text should be on top
builtin and keyword should be next of each other

having the preview in the same dialog would be nice too, but you could also put the edition widgets in the main panel next to the current preview. Maybe it's too big to be practical, I'm not sure.

There is too much space between the "Preview" label and the preview widget, I'd completely remove the label since it's already quite obvious.

Nodd · 2015-12-09T09:53:58Z

spyderlib/plugins/configdialog.py

+        self.preview_editor.setup_editor(linenumbers=True,
+                                         markers=False,
+                                         tab_mode=False,
+                                         font=QFont("Courier New", 10),


The font should not be hard coded, the configured fond should be used instead.

Nodd · 2015-12-09T09:59:37Z

After a very quick overview of the code, it seems that all the definitions are stored in the main configuration file. Ideally each scheme should be in its own file so that it's easier to share, but I'd leave that for another PR (along with import/export)

ccordoba12 · 2015-12-09T13:26:08Z

I guess the final touch would be to have the preview in the editor dialog

It's fine where it is for me. I think few people will go into the trouble of customizing themes, but most will like to see what a theme looks like instead.

Maybe current cell and current line should be in the Background group

I don't know about this one, but I agree with the other @Nodd suggestions.

goanpeca · 2015-12-09T14:59:46Z

@Nodd, as usual thanks for being a pain in the (_!_)

Maybe current cell and current line should be in the Background group, since the are applied to blocks of code and not particular words or characters.

I do not agree, cause the idea is to have a highlight group.... independent of what it highlights

Background group could be before Highlight group since it's more general (not sure about this one)

I am not sure what general mean? you mean it would be changed more often?

Normal text should be on top builtin and keyword should be next of each other

Sure, it make sense, I was waiting for you to give me the full order, I just grouped them together...

There is too much space between the "Preview" label and the preview widget, I'd completely remove the label since it's already quite obvious.

Sure

having the preview in the same dialog would be nice too, but you could also put the edition widgets in the main panel next to the current preview. Maybe it's too big to be practical, I'm not sure.

No no no no and no, the idea was to take away so much visual noise from the user when selecting, a theme. The casual user really just selects a default, a power user would either import one, or make one.

The edition dialog is already too large to bring it back to the main menu.

After a very quick overview of the code, it seems that all the definitions are stored in the main configuration file. Ideally each scheme should be in its own file so that it's easier to share, but I'd leave that for another PR (along with import/export)

👎

To do this I think we need a common use color schemes format, which probably exists and stores stuff as XML or JSON, that is the point of having the Export options, so that you can share them in any format we decide to support, that will come in another PR when those "standards" are pointed out by someone. Any preferences in Spyder is built on the CONF idea, which makes life easy, but restricts us to keep using it, so having separate files would be another layer of possible bugs...

goanpeca · 2015-12-09T15:47:14Z

@Nodd, @ccordoba12

Look update

blink1073 · 2015-12-09T19:07:39Z

I would leave out the create new button for now, and leave that for another PR, as it is just inviting confusion at the moment. Otherwise, I am 👍 for merging and iterating.

goanpeca · 2015-12-09T19:13:05Z

@blink1073, I will remove the button but leave the code that does the work ok, as it is perfectly functional?

blink1073 · 2015-12-09T19:16:56Z

Yep, makes sense to me.

goanpeca · 2015-12-09T19:30:55Z

@blink1073 by the way did you actually test it :-p ?

blink1073 · 2015-12-09T19:31:47Z

Yes, hence my comment about it being confusing :)

goanpeca · 2015-12-09T20:13:06Z

@blink1073 was having some issues with the dialog being too tall, so now is a two columns layout

blink1073 · 2015-12-09T20:19:05Z

Perfect, thank you, I am no longer confused. :)

Nodd · 2015-12-10T00:45:27Z

My main concern about the config file full of everything is that when there is a problem in the config we have to reset everything. I'd bet that it's not the color definitions that will cause spyder to fail, but they will be reset anyway along everything else.
Way too many issues are solved by spyder --reset, we have something to solve here (on the long term).

It's very nice on 2 columns, but actually I'd change the order again (sorry, I was on my phone before):

Normal text
Comment
String
Number
Keyword
Builtin
Definition
Instance (should be renamed to self, or do whe use this color for something else ?)
Current cell
Current line
Occurrence
Matched parens
Unmatched parens
Link

I agree that technically cells and current line are highlighted by the color, but when working on code it's more of a general visual help rather than an important information about what I'm doing in my code. I don't want to start a big debate, as putting them in highlight works too.

ccordoba12 · 2015-12-23T01:25:11Z

spyderlib/plugins/configdialog.py

+        # Checks that this is indeed a default scheme
+        scheme = self.current_scheme
+        names = self.get_option('names')
+        if scheme not in names:


This shouldn't be if scheme in names: ?

Yep, I made a mistake :-)

goanpeca · 2015-12-23T03:23:06Z

@ccordoba12 made the fixes, thanks for the review

ccordoba12 · 2015-12-23T15:05:21Z

spyderlib/plugins/configdialog.py

+
+    def set_scheme(self, scheme_name):
+        """
+        Set the current stack in the dialog to the scheme with 'schema_name'.


Typo here: schema_name -> scheme_name

ccordoba12 · 2015-12-23T22:29:18Z

There's one little error I discovered with live testing: even though you can change a custom scheme name in the scheme editor dialog, that name is not reflected in the schemes combo box of the new preferences pages.

I think it should be changed there too :-)

goanpeca · 2015-12-28T14:48:01Z

@ccordoba12 fixed the issue and related comments

goanpeca · 2016-01-27T05:02:03Z

@ccordoba12 merged with master....

ccordoba12 · 2016-01-27T13:49:02Z

I can't open our Preferences after your update. This is the traceback

Traceback (most recent call last):
  File "/home/carlos/Projects/spyder/github-repo/spyderlib/spyder.py", line 2614, in edit_preferences
    widget.initialize()
  File "/home/carlos/Projects/spyder/github-repo/spyderlib/plugins/configdialog.py", line 70, in initialize
    self.setup_page()
  File "/home/carlos/Projects/spyder/github-repo/spyderlib/plugins/configdialog.py", line 995, in setup_page
    self.update_combobox()
  File "/home/carlos/Projects/spyder/github-repo/spyderlib/plugins/configdialog.py", line 1030, in update_combobox
    self.scheme_choices_dict[self.get_option('{0}/name'.format(n))] = n
  File "/home/carlos/Projects/spyder/github-repo/spyderlib/plugins/configdialog.py", line 49, in get_option
    return CONF.get(self.CONF_SECTION, option, default)
  File "/home/carlos/Projects/spyder/github-repo/spyderlib/config/user.py", line 380, in get
    raise cp.NoOptionError(option, section)
ConfigParser.NoOptionError: No option 'Custom/name' in section: 'color_schemes'

goanpeca · 2016-01-27T17:51:48Z

@ccordoba12 I dont get that error, can you reset your ini ???

There used to be a Custom thing but the ini changed version to change this default....

Should not it be getting that change in version? Should I bump to 25 the version then?

goanpeca · 2016-02-01T02:04:20Z

@ccordoba12 ?

ccordoba12 · 2016-02-01T05:26:41Z

spyderlib/plugins/configdialog.py

    def setup_page(self):
-        tabs = QTabWidget()
        names = self.get_option("names")


Add here names.pop(names.index(u'Custom'))

ccordoba12 · 2016-02-01T05:40:42Z

I added as comments how to fix the bug I'm seeing. The problem is the old Custom scheme has not name attribute (added by you to the other schemes). And Custom is returned as a valid scheme name when you use names = self.get_option('names'), if you are coming from 2.3.

I think the best thing to do (as I did) is to remove Custom from names. Another option would be to remove all Custom/* options from users spyder.ini. But that can't be done by a bump in CONF_VERSION because there is no Custom section in conf/main.py. Besides, removing those options must be done once and only once.

Of course, a potential problem will occur if a user decides to name a color scheme Custom :-s

goanpeca · 2016-02-01T05:45:27Z

@ccordoba12 thanks for the suggested fix.

There is no potential problem, 'cause all the custom names are stored in a different key.

ccordoba12 · 2016-02-01T05:46:27Z

Oh yeah! But now I'm seeing this error when trying to create a new scheme:

Traceback (most recent call last):
  File "/home/carlos/Projects/spyder/github-repo/spyderlib/plugins/configdialog.py", line 1081, in update_preview
    scheme_name = self.current_scheme
  File "/home/carlos/Projects/spyder/github-repo/spyderlib/plugins/configdialog.py", line 1016, in current_scheme
    return self.scheme_choices_dict[self.current_scheme_name]
KeyError: u''
Traceback (most recent call last):
  File "/home/carlos/Projects/spyder/github-repo/spyderlib/plugins/configdialog.py", line 1055, in update_buttons
    current_scheme = self.current_scheme
  File "/home/carlos/Projects/spyder/github-repo/spyderlib/plugins/configdialog.py", line 1016, in current_scheme
    return self.scheme_choices_dict[self.current_scheme_name]
KeyError: u''

ccordoba12 · 2016-02-01T05:48:17Z

And this error when trying to delete it (after closing and opening Preferences again after the error mentioned above):

Traceback (most recent call last):
  File "/home/carlos/Projects/spyder/github-repo/spyderlib/plugins/configdialog.py", line 1158, in delete_scheme
    self.set_scheme('spyder')
  File "/home/carlos/Projects/spyder/github-repo/spyderlib/plugins/configdialog.py", line 1182, in set_scheme
    dlg.set_scheme(scheme_name)
  File "/home/carlos/Projects/spyder/github-repo/spyderlib/plugins/configdialog.py", line 1231, in set_scheme
    self.stack.setCurrentIndex(self.order.index(scheme_name))
ValueError: 'spyder' is not in list

goanpeca · 2016-02-01T05:51:23Z

Are this errors because of the changes you suggested?

ccordoba12 · 2016-02-01T05:53:19Z

I don't think so. My changes only remove Custom as a possible scheme. These errors are in your code.

goanpeca assigned ccordoba12 Dec 8, 2015

goanpeca added this to the V3.0beta3 milestone Dec 8, 2015

goanpeca reviewed Dec 8, 2015
View reviewed changes

goanpeca added User Interface (UI) User Experience (UX) labels Dec 9, 2015

Nodd reviewed Dec 9, 2015
View reviewed changes

ccordoba12 reviewed Dec 23, 2015
View reviewed changes

Fix errors, refactor code

80fd517

ccordoba12 reviewed Dec 23, 2015
View reviewed changes

Update custom name on combobox on scheme creation. Remove comments

5f1b8e9

This was referenced Dec 28, 2015

PR: Remove font groups from plugins and move to general preferences #2885

Closed

Bookmarks for Spyder Editor #910

Open

ccordoba12 mentioned this pull request Jan 6, 2016

Unable to change the interface colors despite saving them in the preferences. #1996

Closed

Merge with master and fixed typos

a904920

ccordoba12 reviewed Feb 1, 2016
View reviewed changes

ccordoba12 mentioned this pull request Mar 6, 2016

PR: Improve Syntax Coloring preferences page and set color scheme for all plugins there #3036

Merged

ccordoba12 removed this from the v3.0beta3 milestone Mar 6, 2016

ccordoba12 closed this Mar 6, 2016

nbara mentioned this pull request Nov 30, 2016

Support 'theme files' #3768

Open

PR: Modify color scheme preferences #2861

PR: Modify color scheme preferences #2861

Conversation

goanpeca commented Dec 8, 2015

Description

Final Screenshots

goanpeca commented Dec 8, 2015

Initial screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blink1073 commented Dec 8, 2015

goanpeca commented Dec 8, 2015

ccordoba12 commented Dec 8, 2015

goanpeca commented Dec 8, 2015

Nodd commented Dec 8, 2015

goanpeca commented Dec 9, 2015

Look update

goanpeca commented Dec 9, 2015

Nodd commented Dec 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nodd commented Dec 9, 2015

ccordoba12 commented Dec 9, 2015

goanpeca commented Dec 9, 2015

goanpeca commented Dec 9, 2015

Look update

blink1073 commented Dec 9, 2015

goanpeca commented Dec 9, 2015

blink1073 commented Dec 9, 2015

goanpeca commented Dec 9, 2015

blink1073 commented Dec 9, 2015

goanpeca commented Dec 9, 2015

blink1073 commented Dec 9, 2015

Nodd commented Dec 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Dec 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 commented Dec 23, 2015

goanpeca commented Dec 28, 2015

goanpeca commented Jan 27, 2016

ccordoba12 commented Jan 27, 2016

goanpeca commented Jan 27, 2016

goanpeca commented Feb 1, 2016

Choose a reason for hiding this comment

ccordoba12 commented Feb 1, 2016

goanpeca commented Feb 1, 2016

ccordoba12 commented Feb 1, 2016

ccordoba12 commented Feb 1, 2016

goanpeca commented Feb 1, 2016

ccordoba12 commented Feb 1, 2016