placement.skip_fullscreen can not be chained with other placement methods.

[nawuko]

Output of awesome --version: https://github.com/awesomeWM/awesome/commit/b7bac1dc761f7e231355e76351500a97b27b6803 (latest, self compiled, on nixos)

How to reproduce the issue:

Chain placement functions with placement.skip_fullscreen when the client is fullscreen

Example: awful.placement.skip_fullscreen + awful.placement.centered

Actual result:

Lua error (width is null) only happens in the codepath when d.fullscreen is true

Proposed Solution:

The return from https://github.com/awesomeWM/awesome/blob/master/lib/awful/placement.lua#L1669 (when d.fullscreen is true) is not compatible with the current placement function api, just returning return get_screen(d.screen).geometry solved the problem for me.

Git Blame Mentions: @actionless

[Elv13]

@actionless Is there any reason why the code returns:

if d.fullscreen then
        return {get_screen(d.screen).geometry, {}, true}
else

Instead of get_screen(d.screen).geometry?

[actionless]

@Elv13 without that chaining wasn't working - mb return value type of other placement functions changed?

[Elv13]

mb return value type of other placement functions changed?

It doesn't appear so. I don't see anything in git log --patch awful/placement.lua which could have affected this code. Also, the return value of a placement function has to be an x/y/width/height table or nil, in that codepath, it isn't.

[actionless]

but all the tests were passing: https://github.com/awesomeWM/awesome/pull/3073/files, also it still works well in my config

[Elv13]

Yeah, but with the return type being different, that will totally create corner cases where it will misbehave, even if the main codepath works.

[actionless]

that also covered by tests: https://github.com/awesomeWM/awesome/pull/3073/files#diff-453c384ca6d23b2756424880e139b44f5933a94dcd3f57eaadfe2b8171fad3b6R21

[Elv13]

I am not arguing that the feature doesn't work, I am mentioning that the return type is different than all other placement functions. If that was fixed, it would not be possible to have case where it does arithmetic on the nil width. I mean, it's clearly possible for width to be accessed in some combination of placement function. Even if we had test with skip_fullscreen against every other placement functions, it would still be possible for someone to trigger the problem with a custom placement function.

[actionless]

my point that before applying the fix, first should be created the test which would fail, and will pass after such fix

[actionless]

i think copying existing test which i linked above, but swaping the order of placement functions, should do that

[Elv13]

local p = awful.placement.skip_fullscreen + awful.placement.centered
p(client.focus)

stack traceback: lib/awful/placement.lua:398: in upvalue 'area_common' lib/awful/placement.lua:429: in upvalue 'geometry_common' lib/awful/placement.lua:1217: in function <lib/awful/placement.lua:1210> (...tail calls...) lib/awful/placement.lua:166: in function <lib/awful/placement.lua:143> (...tail calls...) build/awesomerc.lua:219: in field 'on_press' lib/awful/key.lua:303: in function <lib/awful/key.lua:298> error: lib/awful/placement.lua:398: attempt to perform arithmetic on a nil value (field 'width')

diff --git a/lib/awful/placement.lua b/lib/awful/placement.lua
index 7802bcf8b..1395ef33a 100644
--- a/lib/awful/placement.lua
+++ b/lib/awful/placement.lua
@@ -1671,7 +1671,7 @@ function placement.skip_fullscreen(d, args)
     d = d or capi.client.focus

     if d.fullscreen then
-        return {get_screen(d.screen).geometry, {}, true}
+        return get_screen(d.screen).geometry
     else
         local ngeo = geometry_common(d, args)
         remove_border(d, args, ngeo)

Still passes the tests and the error no longer exist.

[actionless]

local p = awful.placement.skip_fullscreen + awful.placement.centered

yes, that's what i meant 👍

[actionless]

i'll leave it open until someone would submit a PR