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.items.roi: Added RegionOfInterest's getText and setText methods #3847

Merged
merged 4 commits into from
Jun 12, 2023

Conversation

t20100
Copy link
Member

@t20100 t20100 commented Jun 9, 2023

Added 2 methods to change the displayed text of ROIs without changing its name.
The default behavior (setText(None)) is to display the ROI's name.

This PR also contains a fix for PySide6 for RegionOfInterest.

closes #3817

@t20100 t20100 added this to the 2.0.0 milestone Jun 9, 2023
@t20100 t20100 requested a review from vallsv June 9, 2023 15:14
Comment on lines +70 to +72
qt.QObject.__init__(self)
if parent is not None:
self.setParent(parent)
Copy link
Member Author

Choose a reason for hiding this comment

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

This was needed with PySide6 6.5.0 on macos with python3.10

Comment on lines +444 to +452
def getText(self) -> str:
"""Returns the currently displayed text for this ROI"""
return self.getName() if self.__text is None else self.__text

def setText(self, text: Optional[str] = None) -> None:
"""Set the displayed text for this ROI.

If None (the default), the ROI name is used.
"""
Copy link
Member Author

Choose a reason for hiding this comment

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

I choose set|getText rather than set|getLabel since plot markers already have some set|getText methods.

Comment on lines +457 to +462
def _updateText(self, text: str) -> None:
"""Update the text displayed by this ROI

Override in subclass to custom text display
"""
pass
Copy link
Member Author

Choose a reason for hiding this comment

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

This has moved from HandleBasedROI

@@ -437,6 +441,26 @@ def setVisible(self, visible):
self._visible = visible
self._updated(items.ItemChangedType.VISIBLE)

def getText(self) -> str:
"""Returns the currently displayed text for this ROI"""
return self.getName() if self.__text is None else self.__text
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to decouple the text property from the name.

You could see the use of the name as a display mode (which we don't have to handle explicitly).

What do you think?

But i dont see any problem with the actual implementation.

Copy link
Member Author

@t20100 t20100 Jun 12, 2023

Choose a reason for hiding this comment

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

What do you mean? Having getText always returning self.__text (eventually None) and an extra method, e.g., getDisplayedText, to retrieve what should be displayed?

Copy link
Member Author

Choose a reason for hiding this comment

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

After discussion, this is fine as it is

@vallsv
Copy link
Contributor

vallsv commented Jun 12, 2023

Very nice. I think this will simplify more things than i was expecting.

@t20100 t20100 merged commit de4da24 into silx-kit:main Jun 12, 2023
@t20100 t20100 deleted the roi-set-text-display branch June 12, 2023 14:09
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.

Add setLabel to the ROIs
2 participants