awesome

Welcome to awesome bug tracking system.
Tasklist

FS#611 - beautiful race condition when setting wallpapers

Attached to Project: awesome
Opened by koniu (koniu) - Monday, 31 August 2009, 02:37 GMT
Last edited by koniu (koniu) - Thursday, 29 October 2009, 23:56 GMT
Task Type Bug Report
Category Lua libraries → beautiful
Status Closed
Assigned To No-one
Operating System All
Severity Medium
Priority Normal
Reported Version git/master
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 1
Private No

Details

I'm having a pretty strange issue in which awesome "ignores" my wallpaper_cmd, or more precisely, changes it back to the default as soon as it loads. I do get all colors/fonts/etc from my custom theme.lua.

To reproduce it with default config:
1. make a copy of the default theme and replace the wallpaper_cmd with something else than the default (eg "xsetroot -solid red")
2. make a copy of the default rc.lua and near the top add
beautiful.init(path_to_modified_theme)
3. load the config and watch the red for a split second and then back to the default bg

I've added io.stderr:write(theme) at the beginning of beautiful.init() and the output has the commands in right order - first the default then the custom.

Adding beautiful.init(path_to_modified_theme) to the end of the rc.lua changes nothing. but once awesome is running, I can run beautiful.init(path_to_modified_theme) from the prompt or awesome-client and the background is set correctly.

I must be doing something wrong.
This task depends upon

Closed by  koniu (koniu)
Thursday, 29 October 2009, 23:56 GMT
Reason for closing:  Fixed
Additional comments about closing:  Fixed by: 67e53469563882526ebf2b36c401266ba40a935b

Revert "beautiful: init default theme by default" and "awesomerc: stop handling beautiful"

This reverts commit 42c47eeccee804c138a8d7baed01e893196e7873 .
This reverts commit 4823a1254108b3bd5ec086861080ba97018a3868 .
Signed-off-by: Julien Danjou <julien@danjou.info>

thanks
Comment by Julien Danjou (jd) - Monday, 31 August 2009, 08:42 GMT
That's totally normal: xsetroot does not set a wallpaper, it just paint the default root window with a color. It should not work also in command line. So it's not a beautiful bug as it is.
Comment by koniu (koniu) - Monday, 31 August 2009, 14:28 GMT
jd, this is really lame, it happens regardless if its xsetroot or awsetbg some.jpg, from your response it looks like you didnt even bother tryint to reproduce even tho there's clear info on how to do it.
Comment by Julien Danjou (jd) - Monday, 31 August 2009, 15:01 GMT
Of course I did not try to reproduce, since you say yourself that beautiful does it job:
"the output has the commands in right order - first the default then the custom"

I do not say the bug does not exist, I just say this is not a beautiful bug, so not our bug. Beautiful calls the provided command. If they fails to change the wallpaper, we cannot do anything about that.

FWIW I tried to reproduce (I'm nice) and it works perfectly. awsetbg uses Esetroot here to set the display, and then xsetroot works like a charm (so another call to awsetbg does anyway). Maybe your WPSETTER used by awsetbg is different than mine and cannot change a second time the wallpaper and/or disallow changes by xsetroot, I really don't know.

But this is definitively not a bug in awesome.
Comment by koniu (koniu) - Monday, 31 August 2009, 15:38 GMT
> I do not say the bug does not exist, I just say this is not a beautiful bug, so not our bug

Whether this is our bug or not, I'm having issues since you forced loading of the default theme in beautiful (which I'm personally surprised with and I heard the word 'gnome' on the irc channel :p).

> If they fails to change the wallpaper

It does change the wallpaper but its overwritten by the default one.

> Maybe your WPSETTER used by awsetbg is different than mine and cannot change a second time the wallpaper and/or disallow changes by xsetroot, I really don't know.

Once again, this has nothing to do with xsetroot, it was just a simplistic example.

The fact is that the same WPSETTER (feh in my case) is started by awsetbg twice - first with the default background, and then with one in my custom theme but somehow the custom bg is set first (i can see it for a split sec) and the default one overwrites it.

I suspect there is some form of "race condition" and perhaps you're not seeing it for whatever reason but maybe on other systems (like here) there would be problematic effects of spawning of two commands nearly in parallel - first on require("beautiful") and shortly after on beautiful.init(custom_theme) in rc.lua - whilst not knowing which one will finish first.

Having said that, I will try to investigate a bit more later so please dont kill this bug just yet.
Comment by Julien Danjou (jd) - Monday, 31 August 2009, 15:48 GMT
Okay, now that you say "race condition", that sounds more like our bug to me.

In that case we should probably add some sort of wait() in beautiful when setting the wallpaper.
Comment by koniu (koniu) - Wednesday, 02 September 2009, 23:08 GMT
I can't think of a way of tackling this without hassle. To be honest, I'd just revert 4823a1254108b3bd5ec086861080ba97018a3868 and 42c47eeccee804c138a8d7baed01e893196e7873. I see no advantage to these commits apart from the fact that awesomerc.lua.in can be loaded straight as a config file without worrying about @blabla@.

Another disadvantage of implicit theme loading is when someone sets their X background in their display manager and just wants to leave it at that - they'll see flicker (at the best). In plain words it's just too obtrusive - I want choice.

On the contrary, it was better prior to those commits when the user could instantly see how to load a theme, where the default one is etc. The educational value of the default rc is important imo.

Worst case scenario maybe we could put the default theme setting function in some kind of startup signal so that we can do sth like:
awesome.remove_signal("startup", beautiful.load_default_theme)
I have to admit I have no clear idea of how this could work.
Comment by Julien Danjou (jd) - Thursday, 03 September 2009, 08:11 GMT
Simplifying the default rc.lua is a mid-term goal and is also very important. That commit is part of that.
Providing configuration example and documentation is another goal, both are not incompatible.

To me, the easy fix is to disable the wallpaper in the default theme. And that would make him less intrusive.
Comment by koniu (koniu) - Thursday, 03 September 2009, 12:42 GMT
I can't see how removing two lines of obvious, intuitive code contributes to simplifying anything. Like I said before I think it has an opposite effect forcing users to wonder 'how do I change these ugly colors' and have to resort to api docs just for that.

As for the 'easy fix' of disabling it in the def theme - do it each time I update and rebuild, etc? Less intrusive?

Anyway, I think I brought up enough arguments for this really simple matter and further discussion doesn't make much sense - the problem remains and is up to you. However, if you're just gonna close this ticket please acknowledge that I think leaving users no choice but to hack built-in libs is a disappointing direction for awesome and there's a couple of people who share this viewpoint.
Comment by Julien Danjou (jd) - Thursday, 03 September 2009, 13:05 GMT
Well, you don't fix the missing of documentation by adding more code to a default configuration. Loading module asserting that they they have a sane default behaviour is something normal. Loading the default theme by default is done that way.

Asking to revert a commit to fix a problem you have is definitively not a constructive solution. There *is* a bug as you pointed out, and there's probably 2 differents bugs actually:

1. When loading 2 themes one after the other, the wallpaper_cmd execution is put in background so their order of execution is undefined;
2. When loading beautiful, it changes the default background with the one from the default theme.

While I admit the controversed commit has provocated 2., 1. is independant and exists since a while.

So far nobody (nor me) has proposed a fix, but that does not mean going backward is the *right* solution.

I really don't have a simple fix for 1. For 2., I can suggest to totally drop beautiful usage from the default configuration and let use awful default color, which should produce the same result. Untested, but it should work and fix 2.
Comment by Uli Schlachter (psychon) - Thursday, 03 September 2009, 13:15 GMT
Here he go, now someone has proposed even two fixes.

First one adds an explicit beautiful() call for loading the default theme, second one just doesn't apply the wallpaper from the default theme.

Alternatively we could (idea 3:) create an ugly 1sec timer for executing the wallpaper command and abort the timer if a new theme is loaded in this time.

Anyone likes either of these ideas?
Comment by Julien Danjou (jd) - Thursday, 03 September 2009, 13:42 GMT
idea1 and 2 are both OK IMHO, but does not fix point 1 I described (except that I think idea2 patch is not working).

And it's better for you if I ignore idea 3. :-)

As quickly discussed on IRC, I think there's a bigger problem behind all of that, being beautiful required to be loaded and required() by many awful stuff.

Loading...