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

ColormapDialog: upgrade _DataInPlotMode to DisplayMode #3964

Merged
merged 24 commits into from
Nov 28, 2023

Conversation

payno
Copy link
Member

@payno payno commented Nov 16, 2023

The goal of this PR is to allow user select the display mode of the ColormapDialog for the min / max value (data histogram or data range)

TODO

  • move _DataInPlotMode to public API
  • add getter for _dataInPlotMode
  • rename setColorDialog to setColormapDialog ? This would make more sense to me. Especially as we are moving to 2.0
    • add deprecation warning
  • add a getColor(map)Dialog to ColormapAction

EXTRA

Changelog:

move _DataInPlotMode to public API.
close #3963

@vasole
Copy link
Member

vasole commented Nov 16, 2023

rename setColorDialog to setColormapDialog ? This would make more sense to me

That is change to a public API without any deprecation warning. I would appreciate more conservative changes.

For instance, that can break PyMca:

image

@payno payno requested review from vallsv and t20100 November 16, 2023 10:19
@vasole
Copy link
Member

vasole commented Nov 16, 2023

@payno

Are you sure it is not actually a Color dialog (to choose a color and not a colormap)?

image

@payno
Copy link
Member Author

payno commented Nov 16, 2023

Well the expected object is a ColormapDialog and the associated action is named ColormapAction so at least it would be coherent. But then depends on the lexical language people are using probably

Copy link
Member

@t20100 t20100 left a comment

Choose a reason for hiding this comment

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

It is worth checking where setColorDialog is used inside silx and update to setColormapDialog to avoid deprecation warnings.
It would be nice to see where _createDialog is used and if it can't be replaced by usage setColormapDialog... but I guess not (or it would not be there).

src/silx/gui/dialog/ColormapDialog.py Outdated Show resolved Hide resolved
src/silx/gui/dialog/ColormapDialog.py Outdated Show resolved Hide resolved
src/silx/gui/plot/actions/control.py Outdated Show resolved Hide resolved
src/silx/gui/plot/actions/control.py Outdated Show resolved Hide resolved
src/silx/gui/plot/actions/control.py Outdated Show resolved Hide resolved
src/silx/gui/plot/actions/control.py Show resolved Hide resolved
@t20100
Copy link
Member

t20100 commented Nov 16, 2023

Regarding public API changes, we should make explicit what we actually do:

No public API (= stuff without a _ prefix) change without deprecation warning.*

*: Unless impossible to deprecate, but it should be the exception.

@t20100
Copy link
Member

t20100 commented Nov 16, 2023

BTW, +1 for the method renaming to use a consistent name

@payno
Copy link
Member Author

payno commented Nov 16, 2023

It is worth checking where setColorDialog is used inside silx and update to setColormapDialog to avoid deprecation warnings. It would be nice to see where _createDialog is used and if it can't be replaced by usage setColormapDialog... but I guess not (or it would not be there).

For the first one clearly. For _createDialog I don't see it used internally (except for the ColormapAction ). I guess it is used outside by other projects (flint ?)

@t20100
Copy link
Member

t20100 commented Nov 17, 2023

I propose to keep a possible change of _createDialog for another PR.

src/silx/gui/dialog/ColormapDialog.py Outdated Show resolved Hide resolved
@@ -749,12 +755,26 @@ def _plotGammaMarkerConstraint(self, x, y):
x = min(x, vmax)
return x, y

def setDataInPlotMode(self, mode: str | DataInPlotMode):
Copy link
Member

Choose a reason for hiding this comment

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

Maybe there is a better naming for: setDataInPlotMode and DataInPlotMode?
PlotMode? DisplayMode? RangeDisplayMode?

Copy link
Member Author

Choose a reason for hiding this comment

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

+1 for DisplayMode

Copy link
Member Author

Choose a reason for hiding this comment

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

But then I would also rename DataInPlotMode by DisplayMode

Copy link
Member Author

Choose a reason for hiding this comment

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

@vallsv do you mind if we change _DataInPlotMode to DisplayMode ? As those are Enum we won't be able to deprecate them

Copy link
Member

Choose a reason for hiding this comment

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

_DataInPlotMode is private, so no need for deprecation for me

@payno payno changed the title DRAFT: ColormapDialog: move _DataInPlotMode to public API ColormapDialog: upgrade _DataInPlotMode to DisplayMode Nov 23, 2023
@t20100 t20100 added this to the 2.0.0 milestone Nov 28, 2023
@@ -66,6 +66,7 @@
import logging

import numpy
from future import __annotations__
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be the very first of executable line of code right after the module docstring.

And it's: from __future__ import annotations ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

nice one ^^

@t20100 t20100 merged commit 352507a into silx-kit:main Nov 28, 2023
7 checks passed
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.

Using 'histogram' mode of the ColorMapDialog from PlotWindow
3 participants