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

PR: Add some color schemes from Eclipse #8381

Merged
merged 11 commits into from Jun 20, 2019
Merged

PR: Add some color schemes from Eclipse #8381

merged 11 commits into from Jun 20, 2019

Conversation

PanderMusubi
Copy link
Contributor

Description of Changes

See issue

Issue(s) Resolved

Fixes #8371

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: PanderMusubi

@pep8speaks
Copy link

pep8speaks commented Dec 6, 2018

Hello @PanderMusubi! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-06-20 15:38:24 UTC

@ccordoba12 ccordoba12 added this to the v4.0beta3 milestone Dec 6, 2018
@ccordoba12
Copy link
Member

Thanks for your contribution!

@CAM-Gerlach, could you review this one? Thanks!

@PanderMusubi
Copy link
Contributor Author

@CAM-Gerlach there will be one more commit, please review it only when that is in.

@PanderMusubi
Copy link
Contributor Author

@CAM-Gerlach latest commit is there. :)

@CAM-Gerlach
Copy link
Member

@CAM-Gerlach @ccordoba12 Okay, thanks—looking at it now.

@CAM-Gerlach CAM-Gerlach changed the title added color schemes which are converted from Eclipse color themes PR: Add color schemes which are converted from Eclipse color themes Dec 6, 2018
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I tested each of them, and they all seem to work and display fine. However, one thing—we should be consistent about how we capitalize the names, so please make all of them Title Case—you're missing a couple.

Besides that, I don't see a problem with it, other than the theme list starting to grow to a bit of an unwieldy length—if/when we merge this, it would be nice to follow up with a PR that provides some clear indication of whether a theme in the list is light or dark, so users can see at a glance—of course, that would be a separate PR.

spyder/config/main.py Outdated Show resolved Hide resolved
spyder/config/main.py Outdated Show resolved Hide resolved
@PanderMusubi
Copy link
Contributor Author

@CAM-Gerlach see latest commit.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Dec 11, 2018

Thanks @PanderMusubi ! This pretty much looks good to me now.

You're going to need to rebase on the latest master, since we also just updated the Spyder Dark color scheme which resulted in a conflict with yours.

While you're at it, could you fixup (via e.g, interactive rebase) your second and fourth commits into your first and third, and edit the latter's commit message to conform to the universal imperative tense and capatilized convention ("Add color schemes converted from Eclipse color themes", "Optimize and add current cell colors to new Eclipse color schemes")? Thanks.

CAM-Gerlach
CAM-Gerlach previously approved these changes Dec 24, 2018
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks! Just needs a merge with master now to resolve the expected conflict from modifying the Spyder Dark theme.

@CAM-Gerlach
Copy link
Member

@PanderMusubi Hey, if you can merge (or rebase) with master to fix the conflict with the default theme being changed, that would be great and this should be ready to go, pending @ccordoba12 's review. Thanks!

@ccordoba12
Copy link
Member

I need to see an screenshot per each color scheme added. I don't know if I'll agree to all of them to be included, hence the screenshots.

@CAM-Gerlach
Copy link
Member

Okay, that's going to be a lot of screenshots, heh. I can do it a little later.

@PanderMusubi
Copy link
Contributor Author

For this and future PRs, please use https://github.com/PanderMusubi/convert-theme-to-scheme which will generate the Python code to include. It also generated an INI file to easily test the schemes. I will leave the squashing and rebasing to you. If you need me to help with convert-theme-to-scheme, please let me know.

@goanpeca goanpeca self-assigned this Apr 10, 2019
@goanpeca
Copy link
Member

I will update the PR and post the screenshots

@CAM-Gerlach
Copy link
Member

Thanks @goanpeca ! I had started going through them, but with the very long lag to switch syntax schemes on my machine it ended up getting bumped for other priorities.

@ccordoba12 ccordoba12 modified the milestones: v4.0beta3, v4.0beta4 May 18, 2019
@steff456

This comment has been minimized.

@ccordoba12

This comment has been minimized.

@steff456
Copy link
Member

Black Pastel

image

Frontenddev

image

Gedit Original Oblivion

image

Havenkark

image


Inkpot

image


Minimal

image


Mr

image


NightLionAptana Theme
image


Notepad++ Like

image


Oblivion

image


** Obsidian**
image


Pastel
image


RecognEyes
image


Retta

image


Roboticket

image


Schuss
image


Sublime Text Monokai Extended
image


Sublime Text 2
image


Sunburst
image


Tango
image


Vibrant Ink
image


Wombat
image

@goanpeca
Copy link
Member

goanpeca commented Jun 13, 2019

@ccordoba12 we could add these styles as a plugin that adds them?

It might also make sense to not keep the styles on the spyder.ini anymore.

@ccordoba12
Copy link
Member

we could add these styles as a plugin that adds them?

Not right now.

It might also make sense to not keep the styles on the spyder.ini anymore.

Good point @goanpeca!

@steff456, please move the entire appearance section as a dict to a new file called spyder/config/appearance.py with the name APPEARANCE, and import it from there to spyder/config/main.py.

@ccordoba12
Copy link
Member

I'll take a look this weekend at what schemes should stay and be removed.

@goanpeca, any particular preferences?

@goanpeca
Copy link
Member

I like the dark ones :-) pastel

@ccordoba12
Copy link
Member

Ok, I think we should remove these themes:

  • Black pastel (too low contrast)
  • Frontenddev (too similar to our Emacs theme)
  • Gedit Original Oblivion (too low contrast)
  • Havenkark (too low contrast and I don't like the select line color)
  • Mr (too similar to IDLE)
  • RecognEyes (too similar to our theme)
  • Schuss (too low contrast)
  • Sublime Text 2 (I don't like the colors and we already have Monokai)
  • Sunburst (too low contrast)
  • Tango (too low contrast)
  • Wombat (I don't like the colors)

@spyder-ide/core-developers, @spyder-ide/junior-developers, what do you think?

@ccordoba12
Copy link
Member

@steff456, please rename Notepad++ Like to Notepad++.

@ccordoba12
Copy link
Member

ccordoba12 commented Jun 16, 2019

@steff456, I just noticed that none of the new themes provides a currentcell color. Please add one by using a color picker and getting a lighter version of the background color for the dark themes. For the light themes you can use the light yellow used in the Spyder theme or the pink color used in the IDLE theme (depending on which one suits the theme better).

@ccordoba12 ccordoba12 changed the title PR: Add color schemes which are converted from Eclipse color themes PR: Add some color schemes from Eclipse Jun 19, 2019
@goanpeca
Copy link
Member

@ccordoba12 to prevent this in the future I suggest we add a simple test that will iterate and check that:

  • All the expected keys are found for every theme
  • All the values are correct color values (or True or False)

Thoughts?

@ccordoba12
Copy link
Member

ccordoba12 commented Jun 19, 2019

The key was there, the problem was that the color was not defined to something useful.

@goanpeca
Copy link
Member

Ah ok got it

@ccordoba12
Copy link
Member

@steff456, last comments about the themes we chose:

  • Notepad++: currentline cannot be distinguished from currentcell.
  • Pastel: The difference between currentline and currentcell is very small. Please make the difference clearer.
  • Roboticket: currentline cannot be distinguished from currentcell.

spyder/config/appearance.py Outdated Show resolved Hide resolved
spyder/config/appearance.py Outdated Show resolved Hide resolved
spyder/config/main.py Outdated Show resolved Hide resolved
spyder/config/main.py Outdated Show resolved Hide resolved
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @steff456 for your help to finish this one and @PanderMusubi for you initial submission.

@ccordoba12 ccordoba12 merged commit 7a43f10 into spyder-ide:master Jun 20, 2019
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.

Support color scheme Sublime Text Monokai Extended (and more)
6 participants