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

Use XCB to check if an application is fullscreen #187

Merged
merged 2 commits into from
Jul 19, 2019

Conversation

tatokis
Copy link

@tatokis tatokis commented Jun 6, 2019

On multi-monitor environments using xrandr, the rect_is_fullscreen() function fails to detect fullscreen applications.

This happens because root_width and root_height are the sum of all the monitor resolutions.
For example, in my case that would be 5520x1080.

A fullscreen application only goes fullscreen on one of the monitors, with its size being the resolution of one monitor only. Thus, that check fails.

This PR works around that issue by querying the EWMH _NET_WM_STATE Atom [0] using XCB.
If it contains the _NET_WM_STATE_FULLSCREEN Atom, then the application is considered fullscreen.

If the current WM does not support EWMH, it should gracefully fall back to the standard behaviour so that it doesn't cause any regressions.

[0] https://specifications.freedesktop.org/wm-spec/wm-spec-1.3.html#idm140130317598336

@codecov
Copy link

codecov bot commented Jun 6, 2019

Codecov Report

Merging #187 into next will decrease coverage by 0.04%.
The diff coverage is 5.55%.

Impacted file tree graph

@@            Coverage Diff            @@
##            next     #187      +/-   ##
=========================================
- Coverage   12.6%   12.56%   -0.05%     
=========================================
  Files         42       42              
  Lines       8079     8097      +18     
=========================================
- Hits        1018     1017       -1     
- Misses      7061     7080      +19
Impacted Files Coverage Δ
src/config.h 21.42% <ø> (ø) ⬆️
src/config.c 42.19% <ø> (ø) ⬆️
src/atom.h 0% <ø> (ø) ⬆️
src/win.c 0% <0%> (ø) ⬆️
src/options.c 17.97% <0%> (-0.07%) ⬇️
src/config_libconfig.c 55.64% <100%> (+0.17%) ⬆️
src/utils.h 21.05% <0%> (-10.53%) ⬇️

@yshui
Copy link
Owner

yshui commented Jun 7, 2019

@tatokis Thanks for the patch!

It is unclear what is the desired behavior when a window is fullscreen on one of the monitors. If the screen is unredirected in that case, all monitors will lost composition. Is that a motivation for your patch? If not, I am considering stop using win_is_fullscreen as part of the unredirect criteria.

@tatokis
Copy link
Author

tatokis commented Jun 7, 2019

@yshui

It is unclear what is the desired behavior when a window is fullscreen on one of the monitors. If the screen is unredirected in that case, all monitors will lost composition. Is that a motivation for your patch?

If you are running an application in fullscreen, and it is in focus, then chances are you are mostly looking at, and interacting with the fullscreen application almost exclusively. The other monitors will most likely be displaying idle applications such as an IRC client, or other kinds of system stats.

This is true for games, where the cursor is grabbed, and unredirecting is also a must in order to achieve lower latency and no vsync (especially if it's forced on for other applications through the compositor to remove tearing).

If you switch to another application, by pressing for example alt tab, then the desktop will be redirected again until the fullscreen application is back in focus. What that basically means is that once you are done with the fullscreen application, even temporarily, the normal -and expected- behaviour will be resumed.

As an example, Unity 7 with compiz handles unredirection this way, which has been my primary desktop environment for many years now, and the unredirection configuration works quite well in my opinion.
I believe that there isn't much of a loss if other monitors in the display get unredirected, since the fullscreened application will have most of the user's attention anyway.

If you believe this behaviour might confuse people, it could be implemented as an optional CLI argument.

Either way, I think this behaviour is better than the existing implementation, especially when it comes to playing games, because as-is there is simply no way to automatically unredirect a game, without having to mess about with writing scripts.
And even then, the whole display will still be unredirected.

@yshui
Copy link
Owner

yshui commented Jun 7, 2019

@tatokis I think those are really great points.

But since this changes the behavior of compton, I would like to:

  1. Have it as a configurable option. (Preferably have the new behavior on by default, and have an option to disable it).
  2. Wait after v7 release, so we have a chance to gather people's feedback.

Is that OK with you?

@tatokis
Copy link
Author

tatokis commented Jun 7, 2019

I have added the no-ewmh-fullscreen option to revert the original behaviour.

I'm fine with waiting for whenever you want. You can send a message before you want to merge this and I'll rebase it in case there are any conflicts.

I had to force push because I needed to fix a typo and fix the commit message

@yshui
Copy link
Owner

yshui commented Jun 8, 2019

Thanks a lot!

@yshui yshui added this to the v8 milestone Jun 28, 2019
@yshui
Copy link
Owner

yshui commented Jul 18, 2019

@tatokis Hi, can you rebase on top of the latest next branch so I can merge it?

Thanks a lot!

This arg reverts to the old behaviour of checking for fullscreen
windows.
@tatokis
Copy link
Author

tatokis commented Jul 19, 2019

Should be okay now

@yshui
Copy link
Owner

yshui commented Jul 19, 2019

@tatokis thanks a lot.

This pull request was closed.
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