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 functionality for moving viewers between tabs #2387

Merged
merged 7 commits into from
May 15, 2023

Conversation

Carifio24
Copy link
Member

@Carifio24 Carifio24 commented Apr 13, 2023

This PR adds the ability to move a viewer between tabs. This is achieved via the new GlueApplication method move_viewer_to_tab, and exposed in the UI via a subtool (as discussed in the glue meeting on 4/13/2023). As discussed there, this is implemented as a subtool with the idea that we could put tools to do other "window-y" operations (like changing the viewer title) in this menu as well. (I should also note that we can't easily add something like this to the title bar's context menu, as the title bar comes from the system window manager.)

In the UI, this looks as below

move-viewer-tool.mp4

move_tabs

As for the actual implementation, it's relatively straightforward. As explained in the comment in move_viewer_to_tab, I chose to destroy the current GlueMdiSubWindow and then call add_widget for the new tab, rather than simply re-parent the existing one, because this allows us to get the nice behavior (that we also get in e.g. new_data_viewer) where the window will be placed in a nice location in the new tab. If we just re-parent then the window is given the same position in the new tab it had in the old one, which can lead to one viewer covering another (for example, if the user has two viewers that they haven't moved/resized in different tabs).

If anyone has any thoughts (particularly on the UI/icons), please let me know!

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! Just one small request regarding checking that state is preserved

# viewer's current tab
parent = viewer.parent()
self.app.move_viewer_to_tab(viewer, 0)
assert parent is viewer.parent()
Copy link
Member

Choose a reason for hiding this comment

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

Could you test that viewer state is preserved when moving it between tabs?

Copy link
Member Author

@Carifio24 Carifio24 Apr 27, 2023

Choose a reason for hiding this comment

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

@astrofrog I've added in that check

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.

Thanks! This works nicely, except for a small issue I've noticed - if I keep moving an image viewer between two tabs it keeps shrinking vertically each time it is moved (it might happen with other viewers too but haven't checked yet).

Another things that would be nice is if the default selected tab in the dropdown in the dialog is not set to the current tab, if there is more than one tab, but don't worry if you don't have time to implement this.

@Carifio24
Copy link
Member Author

@astrofrog Good catch on the resizing! I hadn't noticed that, but yeah it's very apparent if you move the same viewer back and forth a few times. A viewer actually will shrink horizontally as well, though it's much less.

It looks like what's happening is that when we call a Qt viewer's mdi_wrap method, we resize the MDI window to match the viewer size, which then causes the viewer to shrink a small amount to fit inside. Then when we move, the new MDI wrapper is again sized based on the viewer, which is now slightly smaller...rinse and repeat. To fix this, I just resized the viewer to the MDI window's current size (after it's removed from the MDI area, so this should never be visible to the user).

As for the default selected tab, I just made it 1 if the current tab is 0 and there are multiple tabs, and 0 otherwise. This way the default is never the current tab if that can be avoided, and since tab order doesn't have any particular meaning I don't think we can be any more clever.

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 great, thanks!

@astrofrog astrofrog merged commit c0eb36c into glue-viz:main May 15, 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.

2 participants