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

new color map code, based on a BSD-3 licensed module #2335

Merged
merged 2 commits into from
Oct 31, 2022

Conversation

rozyczko
Copy link
Member

@rozyczko rozyczko commented Oct 30, 2022

Description

Replaced QrangeSlider module with bad licensing (GPL) with similar module with good licensing (BSD).
Improved behaviour and visuals as well.

Fixes #1944

How Has This Been Tested?

Tested on Win10

Review Checklist (please remove items if they don't apply):

  • Code has been reviewed
  • Functionality has been tested
  • Windows installer (GH artifact) has been tested (installed and worked)
  • MacOSX installer (GH artifact) has been tested (installed and worked)
  • User documentation is available and complete (if required)
  • The introduced changes comply with SasView license (BSD 3-Clause)

@wpotrzebowski wpotrzebowski self-requested a review October 30, 2022 19:01
Copy link
Contributor

@lucas-wilkins lucas-wilkins left a comment

Choose a reason for hiding this comment

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

Looks good

src/sas/qtgui/Plotting/ColorMap.py Outdated Show resolved Hide resolved
@wpotrzebowski
Copy link
Contributor

Looks good and works on Mac. These are not really related to this PR, however the list of color maps is unneceserally long and one can input min value higher than max (withouith reversing map). Probably this condition can be changed but I wouldn't worry about it. Also I am wondering what should interaction for reseting to very initial values (it now remembers values from previous setup). Should "Home" from navigation menu do that?

Assure correctness of vmin/vmax
Add typehints
@rozyczko
Copy link
Member Author

the list of color maps is unneceserally long

I think it makes sense to enable users to use all the available color maps from the MPL dictionary.
Otherwise, we would have to do some categorisation and I don't really know which of those maps are common and which are not...

one can input min value higher than max (withouith reversing map).

This, actually, was a serious UI issue. Fixed.

Also I am wondering what should interaction for reseting to very initial values (it now remembers values from previous setup).

Without rewriting a large part of the code I don't see how this can be easily achieved. If you close the widget and then reopen it, the values come back to the original numbers, though.

@wpotrzebowski wpotrzebowski merged commit 64d0e07 into main Oct 31, 2022
@wpotrzebowski wpotrzebowski deleted the 1944_color_map branch October 31, 2022 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Color range slider code needs to change
3 participants