awesomeWM / awesome

awesome window manager
https://awesomewm.org/
GNU General Public License v2.0
6.39k stars 597 forks source link

Strange errors with rules #2897

Closed vp1981 closed 4 years ago

vp1981 commented 5 years ago

Output of awesome --version:

awesome v4.3-486-gf294ab37 (Too long) • Compiled against Lua 5.3.5 (running with Lua 5.3) • D-Bus support: yes • xcb-errors support: no • execinfo support: yes • xcb-randr version: 1.6 • LGI version: 0.9.2

How to reproduce the issue:

Not sure because seems default rc.lua works fine but mine, worked well until recent changes (after commit 78c349677099e06f6059c7a6ea5adee3e6c1713b I think), produces four error messages for every new window.

Actual result:

No error messages.

Expected result:

Error messages appear after every new window is shown, see screenshot.

2019-10-08-084923_577x167_scrot

According to ~/.xsession-errors these "errors" connected with gears/matcher.lua and awful/rules.lua. I checked my rc.lua several times, adapted it to some changes, tried without library lain but still see these messages.

My rc.lua.

P.S. Besides these messages all seems work fine. P.P.S. I had to comment the "focus" and "unfocus" stuff because otherwise I get much more error messages (but of different type).

wimstefan commented 5 years ago

I can confirm this behaviour.

awesome v4.3-486-gf294ab37-dirty (Too long)
 • Compiled against Lua 5.1.5 (running with Lua 5.1)
 • D-Bus support: yes
 • xcb-errors support: no
 • execinfo support: yes
 • xcb-randr version: 1.6
 • LGI version: 0.9.2

I haven't changed my rc.lua for quite a while ....

Elv13 commented 5 years ago

I made a PR with an emergency fix. Please test. It wont fix the problem fully, but should prevent the error and "kind of work" until the proper solution and tests are ready and reviewed. There might be more problem the tests missed.

The regression is due to a new series of PRs intended to sunset an old API pattern (like my_client:geometry() as both a getter and a setter) in favor of the standardized property (like my_screen.geometry). This should not have made a visible change until the real deprecation kicked in.

wimstefan commented 5 years ago

I made a PR with an emergency fix. Please test. It wont fix the problem fully, but should prevent the error and "kind of work" until the proper solution and tests are ready and reviewed. There might be more problem the tests missed.

The regression is due to a new series of PRs intended to sunset an old API pattern (like my_client:geometry() as both a getter and a setter) in favor of the standardized property (like my_screen.geometry). This should not have made a visible change until the real deprecation kicked in.

Thank you for the quick workaround! It works of course :)

I've checked my rules after reading your comment in the PR but I can't see where I'm mixing old and new API. When I switched to Awesome 4.0 I've created my rc.lua from scratch as advised and tried to migrate the old configuration to the new standards. Since I've no degree in Computer Sciences I dare to say that I don't fully understand the configuration and hence easily might have gone wrong in the migration process.

Funny enough this error only occurs on one system which has a few extra rules and different geometries: https://github.com/wimstefan/dotfiles/blob/master/config/awesome/rc.lua#L1042-L1150

I'm curious to hear where my config is error prone ... ;-)

Elv13 commented 5 years ago

Let me expand a bit on what's happening here and why it wasn't detected during testing.

Once upon a time, someone implemented the key and button APIs. They were very low level and you had to specify the exact modifiers you wished to use. For example, CapsLock or NumLock would have to be explicitly specified or if they were checked, everything would behave weirdly. Then someone made a wrapper around those called awful.key and awful.button. Those wrapper would just generate 4 key or button so NumLock and Mod2 could be ignored. But since root.keys() and root.buttons() still expected key and button, instead of being objects, awful.key and awful.button were just returning an array of key and button objects. Then a function called awful.utils.join (later renamed gears.table.join) would take n arrays as parameters, then "foreach" them all and put the result into a new, larger array. This hack made rc.lua mode readable, but it turned out to be a bad idea for everything else. Things like the hotkey_popup had to basically reverse engineer the layout just to be able to store the description. It is also nearly impossible to remove keys from clients or the root wallpaper because their awful.key is lost and you have to parse the raw array of keys, locate where the 4 relevant entries are, then generate a new array without them.

Then the second problem is how those accessors worked. Instead of having a getter and a setter, you only had 1 method which did both. Then newer components like the screen class started using properties for that. Back then (10 years ago), the objects did not support setting random properties on them. You had things like awful.tag.setproperty and awful.client.property.set (note the inconsistencies in the name scheme) to work around that by storing data in an associative array.

Then, around 2016, we/I started to contribute a very large number of major changes to clean this up. The latest of which attack the hardest problem (the one mentioned above). It is very, very hard to fix this without breaking the API. To do it, I had to do the following thing:


-- Before:
-- local old_class = {}
--function old_class:buttons(new) if new then self.real_buttons = new end; return self.real_buttons or {} end

old_class._buttons = old_class.buttons

local function create_buttons_compat(obj)
    return setmetatable(self.real_buttons_objs or {}, { __call = old_class._buttons })
end

setmetatable(old_class, {
    __index = function(obj, key)
        if key == "buttons" then
            return create_buttons_compat(obj)
        else
            return rawget(obj, key)
        end
    end,
    __newindex = function(old, key, value)
        if key == "buttons" then
            obj.real_buttons_objs = value
        else
            rawset(obj, key, value)
        end
    end
})

Then this works fine if you only use the old API or only the new API. It often work when you mix them because I added some extra code to detect that. However that code was only tested with newer objects which all have a standard _private array to store the real content. It wasn't tested with the clients and the tags which actually (under the hood) still use the old awful.tag.setproperty I mentioned earlier. They still do that to limit the amount of breakage people had to migrate from 3.5 to 4.0.

Then comes the fun part. The awful.rules do this:


-- Apply the properties
for property, value in pairs(properties) do
    if type(obj[property]) == "function" then
        -- It is one of those crusty old properties!
        obj[property](obj, value)
    else
        -- A normal modern property!
        obj[property] = value
    end
end

But, in your case (and many other), this explodes. The buttons are still using the old gears.table.join result with the 4xnumber_of_buttons raw button objects. Not the number_of_buttonsxawful.button objects. Now that buttons is no longer detected as a function because it is using a magic metatable __call on a temporary table, it calls the new property codepath which expects the awful.buttons and get the button objects instead. The code did detect that and called the fallback code, but it wasn't tested and client and tag and turned out to be broken for those 2 class.

Edit:

-- old format
obj:buttons(gears.table.join(
    awful.button(),
    awful.button(),
    awful.button()
))

-- new format
obj.buttons = {
    awful.button(),
    awful.button(),
    awful.button(),
}
vp1981 commented 5 years ago

Hello, thank you Emmanuel (Elv13). I tried to follow the syntax of new rc.lua but didn't get good results (before your fix). Do I understand correctly, that mix between old and new format could be not in my rc.lua but in some awesome lua files?

Also, your fix helped with error messages from mouse movement (the part

client.connect_signal("focus",
  function(c) c.border_color = beautiful.border_focus  end
)
client.connect_signal("unfocus",
  function(c) c.border_color = beautiful.border_normal end
)

in rc.lua).

Elv13 commented 5 years ago

Do I understand correctly, that mix between old and new format could be not in my rc.lua but in some awesome lua files?

In the case of the original error from the header post, yes, it was awful.rules which caused the "wrong" code path to be used because it was checking the type of the property and that changed.

I tried to follow the syntax of new rc.lua but didn't get good results (before your fix)

Can you clarify what went wrong so I can make it better?

vp1981 commented 5 years ago

Hello Emmanuel, unfortunately after your fix the error message is gone. I'll try to downgrade the awesome and reproduce the problem but will have to wait the weekend.

wimstefan commented 5 years ago

Thank you so much Emmanuel for such a detailed and thoughtful explanation of what's going on behind the scenes!!! Even though it is way above my intellectual capabilities to comprehend what you're explaining it still is a wonderful gesture from a very talented developer to take the time and explain patiently the issue in such a depth 🙏

psychon commented 5 years ago

I agree with @wimstefan. Thanks to the explanation, I feel like I understood what happened. :-)