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 gravity handling when reparenting a window back to the root window #2323

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

psychon
Copy link
Member

@psychon psychon commented Jul 25, 2018

Fixes #2308
CC @sigprof (I was faster than "coming weekend", whoooh; the most complicated part of this was that I broke test-gravity at first, but did not notice the error (this now has to wait for a specific PropertyNotify to know that unmapping is done, but it also listened for other PropertyNotify messages; the result was that that the state machine got really, really confused)).

@psychon
Copy link
Member Author

psychon commented Jul 25, 2018

Meh. :-(
This works when running just test-gravity.lua, but Travis points out that this fails when run together with the other tests. What the heck!?!

Read on stderr: ERROR: For window with gravity SouthWest in state MAPPED, expected y = 43+-1, but got 43
Read on stderr: ERROR: For window with gravity South in state MAPPED, expected y = 43+-1, but got 43
Read on stderr: ERROR: For window with gravity SouthEast in state MAPPED, expected y = 43+-1, but got 43

@psychon
Copy link
Member Author

psychon commented Jul 25, 2018

Uhm... okay. For some reason, the titlebars have a different size when just running the test vs when running all tests (22px for "make check", 16px for just the one test). I don't know why, nor do I know if this is a red herring or the actual problem (since the problem is a difference of one pixel, I would guess that some rounding in the division is involved, but both these sizes are even...)

@blueyed
Copy link
Member

blueyed commented Jul 26, 2018

Related to some other config (file) being used maybe?

@sigprof
Copy link
Contributor

sigprof commented Jul 26, 2018

Not sure what exactly breaks the test, but the code in titlebar_resize() is definitely incorrect for gravities which require centering. The problem is that titlebar_resize() passes only the size delta into xwindow_translate_for_gravity(), and if that delta is divided by 2 and truncated, the end result may be wrong.

To test this, I added some bindings to clientkeys:

    awful.key({ modkey,           }, "KP_Add",
        function (c)
            local _, size = c.titlebar_top(c)
            c.titlebar_top(c, size + 1)
        end,
        {description = "increase titlebar height", group = "client"}),
    awful.key({ modkey,           }, "KP_Subtract",
        function (c)
            local _, size = c.titlebar_top(c)
            if size > 0 then
                c.titlebar_top(c, size - 1)
            end
        end,
        {description = "decrease titlebar height", group = "client"}),

Then if I run https://github.com/ccg/metacity/blob/master/src/wm-tester/test-gravity.c and use Mod4+KP_Add and Mod4+KP_Subtract on windows with WestGravity, CenterGravity and EastGravity, I see that those windows do not remain vertically centered when the titlebar height is changed. Note that clicking with button 1 inside those windows will make test-gravity reposition them properly.

Changing the titlebar size in steps of 2 instead of 1 makes the problem disappear, because in this case truncation to 0 does not happen. Changing the border width also does not cause problems, because the total width occupied by borders on opposite sizes is always even.

The proper way to calculate the coordinates is to call xwindow_translate_for_gravity() twice — first with the old titlebar sizes, then with the new ones — and then use the difference of those results.

However, all of this should not matter if the titlebar size is changed from 0 to the final value in one step, which is what should usually happen — so the problem with that test is most likely caused by something else.

@sigprof
Copy link
Contributor

sigprof commented Jul 26, 2018

I tried to run test-gravity.lua with both even and odd titlebar size (needed to edit awesomerc.lua for this, because I did not find a way to replace the request::titlebars handler in test-gravity.lua), but was unable to reproduce the Travis failure. (However, test-awful-rules.lua fails if the titlebar size is changed, because it contains local tb_height = gears.math.round(beautiful.get_font_height() * 1.5).)

@psychon
Copy link
Member Author

psychon commented Aug 8, 2018

Rebased on master. Still fails on Travis, but no longer fails locally. No idea what to do about this...

@actionless
Copy link
Member

mb add scrot call and upload it to fileshare, so you could see what happens in xvfb inside travis:

https://github.com/themix-project/oomox-gtk-theme/blob/master/test/test.sh#L138-L141

@psychon
Copy link
Member Author

psychon commented Aug 8, 2018

A screenshot wouldn't tell me more than what the error message already says: Something is off by one pixel. I guess I would rather need to add lots of debug-printfs to the C code to figure out where this difference comes from (a race between... something?). But debugging on Travis is painful since the round-trip-time is so high. :-(

@actionless
Copy link
Member

are you able to reproduce the problem inside docker image with the same ubuntu version as on travis? (i guess, 12.04)

@psychon
Copy link
Member Author

psychon commented Aug 9, 2018

I don't know and I am not going to install docker in the hopes that this might be specific to the ubuntu version. All the code that decides the positions of windows is in awesome, so the same between what is run on Travis and what is run here. I might stare a bit at test-gravity.c and see if I can come up with a possible race that would explain this difference... :-/

This extends test-gravity with a test that checks that the window
position does not change if a window is unmapped and mapped again. This
new test currently fails.

Related-to: awesomeWM#2308
Signed-off-by: Uli Schlachter <psychon@znc.in>
A window can be mapped again later after it was hidden. To make sure it
ends up at the correct position, we have to (inversely) translate it for
its gravity, so that the application of gravity when the window becomes
visible again does not cause problems.

Fixes: awesomeWM#2308
Signed-off-by: Uli Schlachter <psychon@znc.in>
@psychon
Copy link
Member Author

psychon commented Aug 19, 2018

Rebased on master for #2355

@codecov
Copy link

codecov bot commented Aug 19, 2018

Codecov Report

Merging #2323 into master will decrease coverage by 7.61%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2323      +/-   ##
==========================================
- Coverage   84.03%   76.42%   -7.62%     
==========================================
  Files         477      332     -145     
  Lines       32671    18376   -14295     
==========================================
- Hits        27454    14043   -13411     
+ Misses       5217     4333     -884
Flag Coverage Δ
#c_code ?
#functionaltests ?
#lua53 76.42% <ø> (-10.76%) ⬇️
#samples 71.7% <ø> (ø) ⬆️
#unittests 58.32% <ø> (ø) ⬆️
Impacted Files Coverage Δ
lib/beautiful/gtk.lua 6.33% <0%> (-87.2%) ⬇️
lib/gears/wallpaper.lua 11.36% <0%> (-81.23%) ⬇️
lib/awful/layout/suit/fair.lua 19.14% <0%> (-78.73%) ⬇️
lib/awful/client/shape.lua 22.95% <0%> (-77.05%) ⬇️
lib/awful/layout/suit/spiral.lua 23.68% <0%> (-76.32%) ⬇️
lib/wibox/init.lua 25.27% <0%> (-68.69%) ⬇️
lib/awful/spawn.lua 27.67% <0%> (-58.41%) ⬇️
lib/awful/ewmh.lua 30.81% <0%> (-58.4%) ⬇️
lib/awful/layout/suit/corner.lua 26.73% <0%> (-58.23%) ⬇️
lib/wibox/drawable.lua 42.16% <0%> (-55.12%) ⬇️
... and 181 more

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.

None yet

4 participants