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

CompoundEditor : Fix maximizing detached windows #6201

Open
wants to merge 1 commit into
base: 1.5_maintenance
Choose a base branch
from

Conversation

ericmehl
Copy link
Collaborator

@ericmehl ericmehl commented Jan 8, 2025

Qt can only setScreen() on the top-level Window (https://doc.qt.io/qt-5/qwindow.html#setScreen), which means that layouts with detached windows that set to maximized would not be moved to a non-primary screen. By setting the window geometry based on the desired screen, before maximizing, the detached windows get maximized on the right screen.

There's a bit of an assumption here when doing window.setGeometry() that the target screen will have at least enough resolution to hold the originally created window. From what I've seen so far, if the screen can't hold the dimensions requested, at worst you get a warning on the terminal that the window is being resized.

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

Qt can only `setScreen()` on the top-level Window, which meant that
layouts with detached windows that were set to maximized would not be
moved to a non-primary screen. By setting the window geometry based on
the desired screen, before maximizing, the detached windows get
maximized on the right screen.
Copy link
Member

@johnhaddon johnhaddon left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on Eric - it will be good to have it fixed. I can't test as I only have the one monitor, but I've made some comments based on what I see in the code and the feedback on the PR build in Discord.

Qt can only setScreen() on the top-level Window

I don't see how that is relevant here - detached panels are top-level windows aren't they?

Comment on lines +1432 to +1439
# \todo In Qt6, `setScreen()` is added to `QWidget` which we should
# be able to use to set the screen including for detached windows.

window.setWindowState( QtCore.Qt.WindowNoState )

screenGeom = targetScreen.availableGeometry()
geometry = window.geometry()
window.setGeometry( screenGeom.x(), screenGeom.y(), geometry.width(), geometry.height() )
Copy link
Member

Choose a reason for hiding this comment

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

If this is a fix related to maximisation, then perhaps it belongs down below in the elif boundData["maximized"] block? It seems that could also fix the problem reported on Discord, where this code isn't being run for the primary screen.

There's a bit of an assumption here when doing window.setGeometry() that the target screen will have at least enough resolution to hold the originally created window.

Alternatively, perhaps we should always restore the geometry (our final else : clause below) and then maximise? From the looks of it, the code for restoring the geometry is smart enough not to overshoot the screeen.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is a fix related to maximisation, then perhaps it belongs down below in the elif boundData["maximized"] block?

If we take this approach, it might be worth double-checking that the same issue doesn't apply for the boundData["fullscreen"] case, it may need the same special handling as maximisation to ensure we end up fullscreen on the correct screen? If so, that's likely a vote for always restoring the geometry...

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