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

Add subtool for changing window title #2407

Merged
merged 5 commits into from
Jun 1, 2023

Conversation

Carifio24
Copy link
Member

This PR contains an implementation of a feature request that has been mentioned a few times in the glue meetings, which is the ability to change viewer titles. This is done via a subtool (under the same tool menu as the tool for moving viewer), which opens a dialog that allows users to change the title of the viewer. This also adds a new icon for the tab-moving icon to help distinguish, since there are now multiple subtools in that menu.

At the viewer level, I implemented this by adding a title callback property to the base ViewerState class. This allows viewer titles to be persisted via a session file as well. Since the title in the state is just a string, we should be able to use this in a non-Qt context as well.

@jfoster17
Copy link
Member

Nice! I'm super excited to have this feature in glue and this looks like a reasonable way to put this in. If I had a nitpick it would be that we now have sort of redundant self.LABEL and self.title attributes.

Additionally, the Table Viewer does not inherit from common.qt.data_viewer.DataViewer (though it does inherit state from ViewerState) so this does not allow the user to rename Table Viewers. That's... mostly okay... because a TableViewer title already shows the name of the data set being display and I assume that's normally what a user would want.

The Table Viewer directly sets the window_title property (from BaseQtViewerWidget) -- just in case we need a third way to refer to this!

@Carifio24
Copy link
Member Author

It's true that self.LABEL and self.state.title are a bit redundant - this PR essentially sets the meaning of LABEL to be "viewer class name/default title". I wanted the viewer title to be in the state for persistence via a session file, which was my main reason for adding that rather than reusing the existing label. I also didn't want to remove it in case there are any downstream applications that use it - this goes back to the larger-scale question about what's part of the public API.

The table viewer actually does inherit from DataViewer, but this implementation hadn't been adding the tool to it (because I forgot that it has inherit_tools = False), and it wouldn't have worked anyways because of the difference in window titling mechanisms that you pointed out.

I've added another commit that I think helps resolve this - basically, the approach is to leave self.state.title as None until the user explicitly sets it (before I was setting it to LABEL if it was empty or None). This lets us know whether any window title comes from default behavior or not, and do things like keep the current table titling behavior (including updating as the data layer changes) until the user sets the name themselves. I think the table viewer is the only built-in viewer that has any dynamic title functionality, but maybe some custom viewers could use this.

Copy link
Member

@astrofrog astrofrog 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, thank you!

@astrofrog astrofrog merged commit 73a2679 into glue-viz:main Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants