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

AUTOGEN_awful_widget_layoutlist_bar.svg is unreproducible #2629

Open
psychon opened this issue Jan 30, 2019 · 3 comments
Open

AUTOGEN_awful_widget_layoutlist_bar.svg is unreproducible #2629

psychon opened this issue Jan 30, 2019 · 3 comments
Assignees

Comments

@psychon
Copy link
Member

psychon commented Jan 30, 2019

There was also a difference in AUTOGEN_awful_widget_layoutlist_bar.svg that didn't look time-related.

Was it something like this? awesomeWM/apidoc@ab926e9#diff-1c97ba2002b724e67f79d5225c015ce3 It seems that the IDs that cairo generates in the SVG have a random offset...?

In the end, the IDs that cairo uses some from here: https://gitlab.com/cairo/cairo/blob/ef8c379e0f560ced63c2a07566a3a7d608e3996c/src/cairo-surface.c#L264-285
This is just a counter that is incremented each time a cairo surface is allocated. So... something (no idea what, usual suspect is pairs iteration order) causes us to create cairo surfaces in different order / a different number of cairo surfaces. Yay, this will be lots of fun to debug.

Originally posted by @psychon in #2626 (comment)

@Elv13 Elv13 self-assigned this Jan 30, 2019
@Elv13
Copy link
Member

Elv13 commented Jan 30, 2019

It's also a big issue when doing the same in Qt QML and the vast majority of CSS implementations.

Some deep orders are fundamentally undefined:

https://github.com/awesomeWM/awesome/blob/master/lib/wibox/widget/base.lua#L478

https://github.com/awesomeWM/awesome/blob/master/lib/wibox/widget/base.lua#L424

Note that I didn't really try to figure this one out. Neither awful.widget.common or awful.widget.layoutlist have pairs in them.

@psychon
Copy link
Member Author

psychon commented Jan 30, 2019

Found it (boy was this ugly).
For debugging, I wrote out "random empty SVGs". That way I could look at them and see what their internal ID number was. That way I did something like "okay, at this point the numbers stay stable... but at this point, I already have a jump on re-runs, so the problem must be somewhere in between".

Way too much search later: It's the call to gears.table.crush in the constructor of the layoutlist widget. I added print("Setting key", k) to gears.table.crush and this is the order on one run:

Setting key	widget_template
Setting key	source
Setting key	screen
Setting key	base_layout

and this is the order in another run which generated different IDs:

Setting key	base_layout
Setting key	source
Setting key	widget_template
Setting key	screen

Both set_base_layout and set_widget_template call update.

The following patch just survived around 20 runs without producing different output:

diff --git a/lib/awful/widget/layoutlist.lua b/lib/awful/widget/layoutlist.lua
index bb43c7799..ff82d82e9 100644
--- a/lib/awful/widget/layoutlist.lua
+++ b/lib/awful/widget/layoutlist.lua
@@ -150,6 +150,10 @@ local function reload_cache(self)
 end
 
 local function update(self)
+    if self._private.update_frozen then
+        return
+    end
+
     assert(self._private.layout)
     awcommon.list_update(
         self._private.layout,
@@ -400,13 +404,12 @@ local function new(_, args)
 
     gtable.crush(ret, layoutlist, true)
 
+    ret._private.update_frozen = true
     ret._private.style   = args.style or {}
     ret._private.buttons = args.buttons
     ret._private.source  = args.source
     ret._private.data = {}
 
-    reload_cache(ret)
-
     -- Apply all args properties
     gtable.crush(ret, args)
 
@@ -414,6 +417,8 @@ local function new(_, args)
         ret:set_base_layout()
     end
 
+    reload_cache(ret)
+
     assert(ret._private.layout)
 
     -- Do not create a connection per instance, if used in a "volatile"
@@ -427,6 +432,7 @@ local function new(_, args)
     -- Add to the (weak) list of active instances.
     table.insert(instances, ret)
 
+    ret._private.update_frozen = nil
     update(ret)
 
     return ret

Avoiding needless updates could be considered performance optimisation. ;-)

I'll leave it to @Elv13 to figure out what to do.

@Elv13
Copy link
Member

Elv13 commented Jan 31, 2019

I'll leave it to @Elv13 to figure out what to do.

I will when I get some time (soon). Given more widgets start to use args and also given the previously mentioned other pairs in wibox.widget.base, I think the mitigation should be available to add widgets. Sorting is out of the question for startup time considerations, but the "locking" proposed above could be generalized.

Debian is probably better off using @psychon patch for now.

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

No branches or pull requests

2 participants