/matplotlib

Avoid duplicate cmap in image options. #6346

Merged
merged 1 commit into from Apr 28, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
GUI/Qt
Projects
None yet
Milestone
  v2.0.0
4 participants
@anntzer @tacaswell @mdboom @QuLogic
@anntzer
Contributor

anntzer commented Apr 28, 2016

The previous implementation (#5469) was incorrect and would yield a double entry
in the cmap combobox.
@anntzer anntzer Avoid duplicate cmap in image options. 
The previous implementation was incorrect and would yield a double entry
in the cmap combobox.

All checks have passed

3 successful checks
continuous-integration/appveyor/pr — AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage remained the same at 69.667%
Details
ad4feab

mdboom added the needs_review label Apr 28, 2016

@tacaswell tacaswell commented on the diff Apr 28, 2016

View full changes
lib/matplotlib/backends/qt_editor/figureoptions.py
@@ -137,7 +137,7 @@ def prepare_data(d, init):
for label in imagelabels:
image = imagedict[label]
cmap = image.get_cmap()
- if cmap not in cm.cmap_d:
@tacaswell

tacaswell Apr 28, 2016

Owner

would it be better to check that cmap.name not in cm.cmap_d?

@anntzer

anntzer Apr 28, 2016

Contributor

I thought someone could accidentally create a custom cmap with the same name as one in cmap_d (although it's a pretty theoretical risk and it'd make the combobox awkward anyways), so checking on cmap identity should be safer.

tacaswell added this to the 2.1 (next point release) milestone Apr 28, 2016

@tacaswell
Owner

tacaswell commented Apr 28, 2016

👍 on this.

RE my comment; I do not have an opinion one way or the other, just curious why you picked this implementation.

@tacaswell tacaswell merged commit 6b45409 into matplotlib:master Apr 28, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 69.667%
Details

tacaswell removed the needs_review label Apr 28, 2016

QuLogic added the GUI/Qt label May 2, 2016

This was referenced Jul 10, 2016

Merged
Closed
Merged

@QuLogic QuLogic modified the milestone: 2.0 (style change major release), 2.1 (next point release) Jul 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment