Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fullscreening a window leaves a gap of size <border_width> for some clients #1607

Closed
psychon opened this Issue Feb 26, 2017 · 12 comments

Comments

Projects
None yet
8 participants
Owner

psychon commented Feb 26, 2017

Output of awesome --version:

As you wish: :-)

awesome 9999 (Harder, Better, Faster, Stronger)
 • Compiled against Lua 5.2.4 (running with Lua 5.2)
 • D-Bus support: ✔
 • execinfo support: ✔
 • xcb-randr version: 1.5
 • LGI version: 0.9.1

How to reproduce the issue:

  • Play a video with mpv/mplayer
  • Fullscreen the video player (either through awesome via mod4+f or through the client by pressing f)

Actual result:

To quote xwininfo:

  Absolute upper-left X:  1
  Absolute upper-left Y:  1

Expected result:

The window should end up in the upper left corner of the screen.

Some more information on what is going on:

ICCCM brings us this great thing that is window gravities[*]. For mplayer the specified value can be seen here:

WM_NORMAL_HINTS(WM_SIZE_HINTS):
		program specified location: 640, 404
		program specified size: 640 by 272
		program specified minimum size: 4 by 4
		program specified minimum aspect ratio: 640/272
		program specified maximum aspect ratio: 640/272
		program specified base size: 0 by 0
		window gravity: Static

For example Chromium does not have a window gravity-entry in its WM_NORMAL_HINTS and the bug is not reproducible with Chromium.

What happens is that awful.ewmh first calls awful.placement.maximize with ignore_border_width = true and only later sets c.border_width = 0. Changing the border width invokes behaviour specified by ICCCM (changing the size without specifying a new position). mplayer having a Static-gravity means that we should move it so that "it stays where it was before we added/removed window decorations", which means the window is shifted to the bottom/right by its previous border width (well, stays where the border previously moved it...).

No idea how this should be fixed. Anyone wants to hack the C code so that the gravity is more-or-less ignored for changes done from Lua and only applied in places where ICCCM requires us to do so?

[*]: Actually, gravities are an X11-thing, but ICCCM adds them to one more use and this bug happens to be about this part...

P.S.: Assigning to the v4.1 milestone under the assumption that this might be a regression. I haven't actually checked. Anyone who thinks "this is not a regression" or "this shouldn't be in the milestone" is free to remove it (assuming he has the necessary access rights for that...).
P.P.S.: Assigning to @Elv13 since this is about "his" code and I do not have a good right now and am on sick leave anyway.
P.P.P.S.: By the way, should we do something about all the places of code which have EWMH in their name, but are actually implementing things from ICCCM?

CC @karlyeurl

@psychon psychon added this to the v4.1 milestone Feb 26, 2017

Elv13 added a commit to Elv13/awesome-1 that referenced this issue Feb 26, 2017

[WIP] placement: Use the gravity helper in `.maximize`
Not enough to fix #1607 as `restore` doesn't support it.
Contributor

j605 commented Mar 2, 2017

I added a client rule for mpv in rc.lua to set the border_width to zero as a work around for now.

psychon added a commit to psychon/awesome that referenced this issue Mar 2, 2017

Force client's border_width=0 while fullscreen
This commit makes the C code force a client's border_width to be zero
while it is fullscreen. This avoids problems which otherwise happen when
trying to do the same from the C code, because changing the border width
means that the position is corrected for its gravity.

Fixes: awesomeWM#1607
Signed-off-by: Uli Schlachter <psychon@znc.in>

r3lgar commented Mar 4, 2017

Can't reproduce with mpv 0.24.0.

@lcpz lcpz referenced this issue in lcpz/awesome-copycats Mar 12, 2017

Closed

multi-monitor window effects overhang #160

@psychon psychon modified the milestones: v4.2, v4.1 Mar 18, 2017

jhnsmth commented Mar 25, 2017

Is there any chance this will be fixed soon ? I'm happy to help with that.
I can also confirm the issue referenced above.

sw9 commented Apr 26, 2017

It appears that Virtualbox fullscreen also suffers from this issue.

Owner

blueyed commented Apr 26, 2017

@jhnsmth
Maybe this helps in this regard? #1608

jhnsmth commented Apr 28, 2017

@blueyed sorry, this issue slipped my mind. I'll take another look over the weekend.

Owner

psychon commented Apr 29, 2017 edited

I just came up for the following "oneliner" to work around this, no idea if it works:

client.connect_signal("property::fullscreen", function(c)
  if c.fullscreen then
    gears.timer.delayed_call(function()
      if c.valid then
        c:geometry(c.screen.geometry)
      end
    end)
  end
end)

(When a client becomes fullscreen, then resize it to cover the fullscreen a short moment later)

(An 'end' is missing)

Does not work for me on 4.1.

Owner

psychon commented Apr 30, 2017

Whoops. Sorry for the missing end. The other problem was that it should be c.screen.geometry instead of c.screen:geometry(). After fixing both of those, this now actually works for me with mplayer (yes, I really did test something that I wrote ;-)).
And no, this is not a fix that I'd like to see in awesome itself.

Still does not work for me on 4.1. I tried mpv and mplayer.

Owner

psychon commented Apr 30, 2017 edited

I tested in Xephyr with the default config and after xsetroot -solid red to make problems easier to see. Running mplayer and pressing "f" made a "red line" visible at the left of Xephyr. With the above appended to the default config, this no longer happens.

Edit: BTW this was previously fixed in #701 and broken again in 631e75a

psychon added a commit to psychon/awesome that referenced this issue May 1, 2017

Fix fullscreen clients with gravity != NorthWest
Once upon a time, 4b9584f already fixed this problem: We have to
set the border width to zero before applying the new geometry to the
client, because changing the border width makes the client move
according to its gravity.

Then came e543879 and made this code use awful.placement instead
of just fullscreening the client itself (without explaining why in the
commit message!). After this commit, the border width was just ignored
and left as-is. This was then fixed in 0bf8bb6 (no idea which
callback the commit message refers to, the old code was basically just
c.border_width=0, c:geometry(screen_geo)). However, now the border width
was again changed after the geometry and the bug that was fixed by
4b9584f was back.

This commit Fixes this regression again by making sure that the border
width is set to zero before the geometry is set. This becomes slightly
more complicated, because now it is also awful.placement's job to
restore the old border width.

This is why this commit adds a new option to awful.placement so that it
sets the border width to zero after creating its memento of the old
border width.

Fixes: awesomeWM#1607
Signed-off-by: Uli Schlachter <psychon@znc.in>

jhnsmth commented May 8, 2017

Sorry for such a long delay. I did some testing...

version 4.0.105.gbfb35349 - everything is fine.
current master (4.1.63.g98a43581) - as described in this issue
#1608 rebased on master - the same
#1614 rebased on master - after un-fullscreening mpv loses borders completely.
#1764 - fixed!

I also would like to mention a problem, which looks like copycat-killer/awesome-copycats#160 and might be related to this one. I have two screens and in some cases windows (e.g. thunar) overhang on the other screen. Unfortunately I didn't manage to find a stable reproducing scenario. But I believe the problem was related to maximized_horizontal and maximized_vertical, as after I unset them (with shortcuts) everything seems fine. It never happened with 4.0.105.gbfb35349.

Hope this helps.

@blueyed blueyed closed this in #1764 May 13, 2017

blueyed added a commit that referenced this issue May 13, 2017

Fix fullscreen clients with gravity != NorthWest (#1764)
Once upon a time, 4b9584f already fixed this problem: We have to
set the border width to zero before applying the new geometry to the
client, because changing the border width makes the client move
according to its gravity.

Then came e543879 and made this code use awful.placement instead
of just fullscreening the client itself (without explaining why in the
commit message!). After this commit, the border width was just ignored
and left as-is. This was then fixed in 0bf8bb6 (no idea which
callback the commit message refers to, the old code was basically just
c.border_width=0, c:geometry(screen_geo)). However, now the border width
was again changed after the geometry and the bug that was fixed by
4b9584f was back.

This commit Fixes this regression again by making sure that the border
width is set to zero before the geometry is set. This becomes slightly
more complicated, because now it is also awful.placement's job to
restore the old border width.

This is why this commit adds a new option to awful.placement so that it
sets the border width to zero after creating its memento of the old
border width.

Fixes: #1607
Signed-off-by: Uli Schlachter <psychon@znc.in>

psychon added a commit to psychon/awesome that referenced this issue May 26, 2017

Add a test case for some recent bug
This test would have caught
awesomeWM#1607.

Signed-off-by: Uli Schlachter <psychon@znc.in>

Elv13 added a commit to psychon/awesome that referenced this issue May 26, 2017

Add a test case for some recent bug
This test would have caught
awesomeWM#1607.

Signed-off-by: Uli Schlachter <psychon@znc.in>

psychon added a commit to psychon/awesome that referenced this issue May 26, 2017

Add a test case for some recent bug
This test would have caught
awesomeWM#1607.

Signed-off-by: Uli Schlachter <psychon@znc.in>

petoju added a commit to petoju/awesome that referenced this issue Sep 10, 2017

Fix fullscreen clients with gravity != NorthWest (#1764)
Once upon a time, 4b9584f already fixed this problem: We have to
set the border width to zero before applying the new geometry to the
client, because changing the border width makes the client move
according to its gravity.

Then came e543879 and made this code use awful.placement instead
of just fullscreening the client itself (without explaining why in the
commit message!). After this commit, the border width was just ignored
and left as-is. This was then fixed in 0bf8bb6 (no idea which
callback the commit message refers to, the old code was basically just
c.border_width=0, c:geometry(screen_geo)). However, now the border width
was again changed after the geometry and the bug that was fixed by
4b9584f was back.

This commit Fixes this regression again by making sure that the border
width is set to zero before the geometry is set. This becomes slightly
more complicated, because now it is also awful.placement's job to
restore the old border width.

This is why this commit adds a new option to awful.placement so that it
sets the border width to zero after creating its memento of the old
border width.

Fixes: awesomeWM#1607
Signed-off-by: Uli Schlachter <psychon@znc.in>

petoju added a commit to petoju/awesome that referenced this issue Sep 10, 2017

Add a test case for some recent bug
This test would have caught
awesomeWM#1607.

Signed-off-by: Uli Schlachter <psychon@znc.in>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment