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

fix fullscreen using pointer on several monitors #2049

Merged
merged 1 commit into from
Mar 4, 2022

Conversation

ainquel
Copy link
Contributor

@ainquel ainquel commented Mar 1, 2022

This PR fixes the problem described in #928 (at least on my computer)

I'm using several monitors with dock bar on all monitors and top bar on the main monitor.

It seems that when the guake's window height is larger than the height of the monitor excluding top/dock bars height, the monitor is sometimes "ignored" by gdk. If I'm resizing the guake window to be smaller than this height, it works.

The bug is at screen.get_monitor_at_point(x, y).

It returns the number of the wrong monitor when I'm encountering the problem.

In this PR, I've reworked the method get_final_window_monitor to use latest gdk APIs (as pointed out by the TODO in the code) and instead of using .get_geometry(), I'm using method .get_workarea() which seems to return the rectangle "usable" by an application (without dock / top bars or else)

I've also tested manually all guake's options of "Appear on mouse display" on single/multiple monitors and it works as expected.

I'm not used to guake and gdk libs so I don't know if it can break things or if you have specific "integration" tests to do other than tests in the lib ?

Copy link
Collaborator

@Davidy22 Davidy22 left a comment

Choose a reason for hiding this comment

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

We do have tests for some things, this specific case is not one of the things covered by the test suite among other multi-monitor things. Might be nice to have. For now, the code looks alright and works for me as well, just want to check one thing before merging.

dest_screen = screen.get_primary_monitor()

# Use primary display if configured
if dest_screen == ALWAYS_ON_PRIMARY:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the always on primary option in the "Appear on display" drop-down is for using the system configured primary display if chosen, while other options in the drop-down let the user override with a specific monitor. Looks like the explicit check is removed here in favor of a catch-all if the search for a specific monitor falls through. Not noticing adverse side effects from the removal of this, raising this in case any potential edge case comes to mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any side effects as well as it merges the behavior of lines 309 and 313.

I can add it back if you want to.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looked fine, just wanted to make sure we didn't accidentally kill any of the defined monitor options.

@Davidy22 Davidy22 merged commit 9ef93a2 into Guake:master Mar 4, 2022
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.

2 participants