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

silx.gui.plot: Removed deprecated methods RegionOfInterest.get|setLabel #3810

Merged
merged 1 commit into from
May 11, 2023

Conversation

t20100
Copy link
Member

@t20100 t20100 commented May 10, 2023

This deprecation removal was initially in PR #3799 but is extracted as a separate PR for discussion.

Related to PR #3799 and issue #3754

@t20100 t20100 added this to the 2.0.0 milestone May 10, 2023
@t20100 t20100 requested a review from vallsv May 10, 2023 07:31
@t20100 t20100 changed the title Remove deprecated methods RegionOfInterest.get|setLabel silx.gui.plot: Removed deprecated methods RegionOfInterest.get|setLabel May 10, 2023
@t20100 t20100 force-pushed the remove-RegionOfInterset-setLabel branch from 64ebced to dbecca6 Compare May 11, 2023 11:10
@vallsv
Copy link
Contributor

vallsv commented May 11, 2023

Could we check instead if it is possible to use setLabel?

Is setName have a meaning for something else here?
I guess that's inherited, but what for in the case of ROIs?

@vallsv
Copy link
Contributor

vallsv commented May 11, 2023

What i mean for the ROIs it is used to displayed a text like a label.

  • I guess you could have 2 ROIs with the same name
  • You can have space in the name

Maybe we just have to distinguish the 2 concepts

@t20100
Copy link
Member Author

t20100 commented May 11, 2023

This was deprecated in PR #2684 when unifying the API with those of CurvesROIWidget and to have something similar to plot Items where the name is said to be the "legend".

BTW, the name does not need to be unique and can be any str.

I agree that there can be 2 slightly different purposes:

  • A name that can be used to represent the ROI item in a list widget for example
  • A label that is displayed next to the ROI in the plot

With current implementation there a single string for both, and since name is used elsewhere I would stick to it now.

If we need it, we can later add a get|setLabel again but with a different behaviour: if label is not None, it is used instead of name for the display in the plot.

@vallsv
Copy link
Contributor

vallsv commented May 11, 2023

I feel like it's a good idea.

The displayed stuff could be overwritten by setLabel in the future if we need.

But i think i already need such thing. Right now i have to hold some key/roi mapping on Flint.
This could be replaced by such setName/setLabel.

Let's merge that PR for now.

@t20100 t20100 merged commit 4b68781 into silx-kit:main May 11, 2023
@t20100 t20100 deleted the remove-RegionOfInterset-setLabel branch May 11, 2023 12:32
@vasole
Copy link
Member

vasole commented May 12, 2023

Well, there are two different concepts:

  • One is to uniquely identify the ROI.
  • The other one is to set what is displayed in the plot.

For instance, you can have multiple ROIs in a 1D plot (Fe, Co, Proteins, Lipids...) but all of them can display MIN and MAX when activated.

I find all these plot deprecation going in the opposite direction to what silx intended to be.

If silx is just a library for flint, you can forget my comment above.

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.

3 participants