-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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: Remove Spyder 2 icon set because it's incomplete #20997
Conversation
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.
Quick review for you @jsbautista. And as usual, thanks for your work on this!
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 @jsbautista for your continued work on this PR! I left a suggestion to improve the message we show users in case our icon theme fails to load. But please don't apply it until @CAM-Gerlach has said what he thinks about it
Other than that, this looks good to me.
As for the actual problem, I'll be able to investigate it further on the user's machine this Friday, as given as far as I can tell, it isn't clear that either of these two issues is the culprit here. |
Co-authored-by: C.A.M. Gerlach <[email protected]> Co-authored-by: Carlos Cordoba <[email protected]>
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 for your work here @jsbautista ! Left a comment to remove a line (the one after the info message when the font is unable to be loaded) that was used to fallback the icon_theme
setting to be 'spyder 2'
.
Also, since here we will remove the .png
files for the Spyder 2 icons. Maybe we should also remove the related entries from the NOTICE.txt
file?:
Lines 1339 to 1342 in 2771c0e
spyder/images/spyder2_icon_theme/dictedit.png | |
spyder/images/spyder2_icon_theme/not_found.png | |
spyder/images/spyder2_icon_theme/whole_words.png | |
spyder/images/spyder2_icon_theme/win_env.png |
And
Lines 1471 to 1475 in 2771c0e
spyder/images/spyder2_icon_theme/advanced.png | |
spyder/images/spyder2_icon_theme/help.png | |
spyder/images/spyder2_icon_theme/italic.png | |
spyder/images/spyder2_icon_theme/vcs_browse.png | |
spyder/images/spyder2_icon_theme/vcs_commit.png |
Also, @ccordoba12 are there more files that will need to be removed?
Sorry, I really don't know. You need to check if there are more files around that need to be removed. |
Checking the image files around, seems like the ones located in the following directories could also be removed:
|
(And need to be, if we're removing the appropriate attribution, copyright statement and license from the |
Yeah, please don't remove that one. But you could relocate it to a better place.
Very good point! Then we should also remove the mentions about other icon sets in the |
Yup, and also in |
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 @jsbautista for removing the pending icons and updating the files that reference them! I think there is one more place left to remove references to the spyder 2 icons and it's at:
spyder/spyder/widgets/about.py
Lines 141 to 150 in d90232c
Most Spyder 2 theme icons sourced from the | |
<a href="https://www.everaldo.com">Crystal Project iconset</a> | |
(© 2006-2007 Everaldo Coelho; LGPL 2.1+). | |
Other icons from | |
<a href="http://p.yusukekamiyamane.com/">Yusuke Kamiyamane</a> | |
(© 2013 Yusuke Kamiyamane; CC-BY 3.0), | |
the <a href="http://www.famfamfam.com/lab/icons/silk/">FamFamFam | |
Silk icon set</a> 1.3 (© 2006 Mark James; CC-BY 2.5), and | |
the <a href="https://www.kde.org/">KDE Oxygen icons</a> | |
(© 2007 KDE Artists; LGPL 3.0+). |
Other than that this LGTM 👍
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.
Looks good to me now, thanks @jsbautista for your work on this!
Note: The failure in our tests seems unrelated to this, so I'd suggest to merge despite it.
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 @jsbautista !
@CAM-Gerlach, are you ok to merge this one? |
Merging as discussed in the dev meeting |
Description of Changes
Resolve Spyder 2 icon set incomplete in Spyder
Issue(s) Resolved
Fixes spyder-ide/ux-improvements#43
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:
@jsbautista