awesome

Welcome to awesome bug tracking system.
Tasklist

FS#503 - awesome "steals" gnome systray even when its systray is disabled

Attached to Project: awesome
Opened by Stefano Zacchiroli (zack) - Tuesday, 21 April 2009, 12:40 GMT
Last edited by Uli Schlachter (psychon) - Thursday, 22 July 2010, 07:28 GMT
Task Type Bug Report
Category Core
Status Closed
Assigned To No-one
Operating System Linux
Severity Medium
Priority Normal
Reported Version 3.2.1
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 7
Private No

Details

I'm using awesome 3.2.1 (Debian/unstable) with GNOME. In GNOME, I'm using the legacy systray applet and hence I've disabled awesome' own systray by commenting out the corresponding line in rc.lua.

Nevertheless, the GNOME systray seems not to allow new applications to appear there, which in turn seems to block some application to start. For instance, running pigdin shows now window on the screen, while according to ps and strace pidgin is in fact running. Other applications which register to the systray (I've tried, for instance, with skype) exhibit a similar behaviour.

I'm able to reproduce the problem also with no user rc.lua, so I'm not attaching mine. The only other change I've in the system-wide rc.lua is to disable naughty, because I want to use GNOME's notifications instead of awesome's. I'm attaching my system wide rc.lua.



I don't know if it is related or not, but I haven't (yet) found the proper way of having GNOME run awesome as its window manager. The gconf tip reported on the wiki does not make gnome-session start awesome. Hence I've tried adding it as a startup program, but that plays badly with the systray applet (coincidence?) making systray-ed applications not register in it (mainly 2: GNOME's network manager and power manager). Hence my last attempt of using awesome with GNOME was stuck at not having any window manager started by gnome-session and starting awesome by hand at the end of the GNOME bootstrap procedure.

Also, no matter how I start awesome (by hand or from GNOME's session), it does not get killed when leaving GNOME.

Many thanks for your efforts!
Cheers.
   rc.lua (15.7 KiB)
This task depends upon

Closed by  Uli Schlachter (psychon)
Thursday, 22 July 2010, 07:28 GMT
Reason for closing:  Fixed
Additional comments about closing:  commit f47b81699617f23d8bcef74f714c9a52ea6550b5
Author: Daniel Graña <dangra@gmail.com>
Date: Fri May 21 16:18:12 2010 -0300

Register systray only if systray widgets are attached. ( FS#503 )

Signed-off-by: Daniel Graña <dangra@gmail.com>
Signed-off-by: Uli Schlachter <psychon@znc.in>
Comment by Stefano Zacchiroli (zack) - Tuesday, 21 April 2009, 14:53 GMT
Some more info. I'm able to reproduce the problem not only using gnome-session, but also with the attached ~/.Xsession .

A "workaround" that seems to work is to have awesome start *after* the systray spawned by the gnome-panel is started
(and that's the reason of the sleep in the attached .Xsession) *and* remove the systray from the panel to add a new one.
In the new one, apps will be able to appear.

If awesome starts before the GNOME's systray, apps like gnome-power-manager and network manager get "lost" and do not
appear in the systray not even if that is removed and readded to the panel.
   Xsession (0.2 KiB)
Comment by Julien Danjou (jd) - Monday, 27 April 2009, 11:57 GMT
You can't totally disable the awesome systray. And it assumes generally it is the only one to run.
Comment by Stefano Zacchiroli (zack) - Thursday, 21 May 2009, 09:40 GMT
Is that by design?

I would personally be very interested in having a way to use the GNOME's one, and I believe other users would be interested.
Of course I'm willing to help, but I wouldn't enroll in the task if you consider this a "feature" that will not be changed in the future.

Thanks for your feedback,
Cheers.
Comment by Angel Olivera (redondos) - Wednesday, 08 July 2009, 00:22 GMT
Stefano's workaround didn't work for me; in fact, I've been starting awesome much later than gnome-panel all this time.

jd, are there plans for implementing this?

Thanks.
Comment by Daniel Graña (dangra) - Thursday, 13 May 2010, 20:01 GMT
The attached patch adds a --nosystray command line option to disable systray.

I used instruction here http://awesome.naquadah.org/wiki/Quickly_Setting_up_Awesome_with_Gnome#Normally,
but modified ~/.config/autostart/awesome.desktop as:

[Desktop Entry]
Version=1.0
Type=Application
Name=Awesome
Comment=The awesome launcher!
TryExec=awesome
Exec=awesome --nosystray
Comment by Uli Schlachter (psychon) - Friday, 14 May 2010, 15:06 GMT
That patch doesn't really make sure that systray.c later on causes invalid "stuff" to happen. After all it's now working with uninitialized variables (globalconf.screen.tab[phys_screen].systray.window), isn't it?
Comment by Daniel Graña (dangra) - Wednesday, 19 May 2010, 20:34 GMT
This hacky patch does the job without leaving uninitialized variables around, but I don't know if passing a command line option is acceptable to disable systray in awesome.

diff --git a/systray.c b/systray.c
index 1f573ee..1e9cde3 100644
--- a/systray.c
+++ b/systray.c
@@ -85,7 +85,7 @@ systray_init(int phys_screen)
p_delete(&atom_systray_r);

xcb_set_selection_owner(globalconf.connection,
- globalconf.screens.tab[phys_screen].systray.window,
+ XCB_NONE, /* globalconf.screens.tab[phys_screen].systray.window, */
atom_systray,
XCB_CURRENT_TIME);

Comment by Daniel Graña (dangra) - Wednesday, 19 May 2010, 20:36 GMT
Was thinking at using command line option and a bool systray_is_active in globalconf. Doing xcb_set_selection_owner based on systray_is_active flag.

Sounds good?
Comment by Daniel Graña (dangra) - Wednesday, 19 May 2010, 21:32 GMT
here it is:
* adds command line option --nosystray
* adds globalconf.systray_is_active (much like xinerama_is_active and have_xtest)
* it disables systray by not selecting it to receive notifications
* patch at http://gist.github.com/406858
Comment by Daniel Graña (dangra) - Wednesday, 19 May 2010, 22:12 GMT
For future references, Gregor Best suggests registering the systray only if a systray widget is created.
seeawesome@naquadah.org/msg01437.html"> http://www.mail-archive.com/awesome@naquadah.org/msg01437.html
Comment by Daniel Graña (dangra) - Wednesday, 19 May 2010, 22:14 GMT
no idea why previous link got broken, here it is again
awesome@naquadah.org/msg01437.html"> http://www.mail-archive.com/awesome@naquadah.org/msg01437.html
Comment by Daniel Graña (dangra) - Thursday, 20 May 2010, 14:25 GMT
Citing email about the topic:

===============================================================================
On Thu, May 20, 2010 at 5:27 AM, Julien Danjou <julien@danjou.info> wrote:
Hi Daniel,

> I was trying to use awesome as wm behind gnome replacing metacity but found
> the bug better described by
>  FS#503 <http://awesome.naquadah.org/bugs/index.php?do=details&task_id=503>.
> I am trying to fix it now (or part of it at least).
>
> I cloned awesome source, prepared a patch and need someone to review it. My
> latests comments has some notes about the patch, see
> http://awesome.naquadah.org/bugs/index.php?do=details&task_id=503#comment1967

Well, the patch seems valid, unfortunately I'm not very comfortable with it.
The ideal patch would be to enable systray window only at systray widget
creation, and destroy it on widget destruction.

Sounds sensible, I was hanging around in #awesome to get direction to proper fix and now I got it.
Will try to submit a patch in the coming days.

thanks!
Daniel.
===============================================================================
Comment by Uli Schlachter (psychon) - Thursday, 20 May 2010, 15:34 GMT
Lots of new comments... Oo

For using xcb_set_selection_owner() with XCB_NONE: This will cause a BadWindow error from the server, wouldn't it make more sense to just not call that function at all, if it's not needed? :P
On the patch from github (http://gist.github.com/406858): Same comment, just move the call to xcb_set_selection_owner() inside of the if ().

Besides that, this patch seems like it does the job it was meant to do.

For committing this patch to awesome, it would be easier if you could append the result of 'git format-patch origin/HEAD..' to this bug. Jd can the just 'git am' those (if he likes them...).
Comment by Daniel Graña (dangra) - Thursday, 20 May 2010, 16:11 GMT
@psychon

Patch was rejected by jd but he suggested proper fix:
| The ideal patch would be to enable systray window only at systray widget
| creation, and destroy it on widget destruction.

>For using xcb_set_selection_owner() with XCB_NONE: This will cause a BadWindow
> error from the server, wouldn't it make more sense to just not call that function
> at all, if it's not needed? :P

I am learning xcb/X in the process of fixing this issue, so thanks for pointing the difference.
I choose between not doing any selection (not calling xcb_set_selection_owner) versus clear the selection
using XCB_NONE just like systray_cleanup does hoping that doing clearing the selection won't fail.

If calling with XCB_NONE fails at systray_init, it will fail at systray_cleanup too
but nobody will notice it because it's just leaving awesome.

But as said, patch needs to be reworked to register/select systray only if a systray widget is created.

thanks!
Comment by Uli Schlachter (psychon) - Thursday, 20 May 2010, 19:57 GMT
Ah, sorry seems like I mis-read the man page for XSetSelectionOwner.

Wouldn't setting it to XCB_NONE also break gnome since it effectively unsets gnome's selection? (Each selection is a shared thing on the X server, all apps access the same "stuff")
I don't know much about this either, but clearing it seems wrong.
Comment by Daniel Graña (dangra) - Friday, 21 May 2010, 02:04 GMT
thanks for pointing the manpage, something useful I extracted from it:

| If the client that is the owner of a selection is later terminated (that is, its connection is closed) or if
| the owner window it has specified in the request is later destroyed, the owner of the selection automatically reverts to None

Does it means that clearing the selection at systray_cleanup is not required?
Comment by Daniel Graña (dangra) - Friday, 21 May 2010, 05:02 GMT
Please, review patch that register systray only if a systray widget is created and attached to a wibox.
* Patch at http://gist.github.com/408472
* Adds a `registered` flag to screent_t.systray structure (commonly acceded by globalconf.screens.tab[phys_screen].systray)
* Splits systray_init in two functions, one that initialize screen_t.systray and the other that register systray with X (named systray_register)

feedback welcome!
Comment by Uli Schlachter (psychon) - Saturday, 22 May 2010, 07:48 GMT
+ screen_t screen = globalconf.screens.tab[phys_screen];

Doesn't that copy the screen_t? That would mean that all changes that are made to it will be lost.
Also, didn't you mean to move "screen.systray.registered = true;" into systray_register()? (I would also move the check of 'if(!screen.systray.registered = true)' into systray_register())
Also, is systray_init() still needed? I think creating the window can be moved into systray_register().
Comment by Daniel Graña (dangra) - Friday, 28 May 2010, 12:48 GMT
On Wed, May 26, 2010 at 3:50 PM, Uli Schlachter <psychon@znc.in> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> Am 24.05.2010 21:57, Daniel Gra=C3=B1a wrote:
> > http://gist.github.com/412326
>
> I wonder what happens if I have two wiboxes attached to the same screen..=
.?
> (First one got a systray, second one doesnt)
>
> When the first one updates, it will register the systray. When the second
> one
> updates it will unregister it again. Next time the first one updates...
>
> Did I miss sth?
>

Hi, it looks like a valid bug report to me :(

And based on it, another patch approach
http://github.com/dangra/awesome/commit/d872bf...<http://github.com/dangra/=
awesome/commit/d872bf46073fcda5bf68008b8d699bd0d780fe31>

The notable things about this new patch:
* `wibox_t` gains a new field named `systray_count` to track the number of
systray widgets attached per wibox
* `wibox_systray_refresh` updates `wibox->systray_count` but do not
register/unregister screen systray
* Added `systray_refresh` function that is called after `wibox_refresh` in
`awesome_refresh` cb
Comment by Daniel Graña (dangra) - Friday, 28 May 2010, 12:51 GMT
Need to complain, current FS fails parsing urls and emails, version running at FS site hasn't this bugs and it also allows to preview comments before submitting.
Comment by lotus (lotus) - Tuesday, 08 June 2010, 18:58 GMT
guys what have u done on me. now if i start fbpanel i get only the error : ""tray: another systray already running" in the console. this can not be.
Comment by Uli Schlachter (psychon) - Tuesday, 08 June 2010, 19:14 GMT
@lotus: Uhm, what does have this to do with this report?
Comment by lotus (lotus) - Wednesday, 09 June 2010, 06:20 GMT
this concern to this because since some changes in the git tree, i am not anymore possible to start fbpanel, because it only accept one systray.
Comment by Gregor Best (farhaven) - Wednesday, 09 June 2010, 12:56 GMT
The systray is allocated at startup since Awesome 3.0. That is almost 2 years ago and by no means happened recently. You can start fbpanel if you remove the systray from its configuration, and as for Awesome always grabbing the systray, we're working on that :)
Comment by Daniel Graña (dangra) - Wednesday, 09 June 2010, 16:00 GMT
@lotus: you can check this email thread about progress in this ticketawesome-devel@naquadah.org/msg05032.html"> http://www.mail-archive.com/awesome-devel@naquadah.org/msg05032.html

A shortcut, latest patch to fix this issue is http://github.com/dangra/awesome/commit/a513e321bd445d2dfc6dca4af9f22535b6b32324

Could be cool if you help testing it, thanks.
Comment by Uli Schlachter (psychon) - Friday, 11 June 2010, 16:22 GMT
Despite some grammar stuff ("Has wibox an attached systray") I feel like systray_refresh() seems bad. It needlessly walks all wiboxes on each update. Wouldn't it make more sense to call systray_register() directly from wibox_refresh() *only* when the has_systray value changes?
(Also, wtf @ the foreach loop in systray_refresh()? Are you really or-ing value into a boolean? Just set that one to true and "break;"?!)

Besides that, the code seems sane. Now let's do some testing...
Results:
- Alltray sucks. Never use it!
- Patch doesn't seem to break my config
- Alltray annoyed me enough so that I don't feel like adding/removing systrays dynamically, sorry

I'd vote for jd testing this himself and/or merging the next version of the patches (really, no need to walk all the wiboxes).

Oh and sorry if I sound harsh, I don't mean stuff as unfriendly as I write it.8
Comment by Daniel Graña (dangra) - Friday, 11 June 2010, 17:17 GMT
> Despite some grammar stuff ("Has wibox an attached systray")
yes, my english grammar sucks, suggestions? "Has systray attached" or just "Has systray"?

> I feel like systray_refresh() seems bad. It needlessly walks all wiboxes on each update.
> Wouldn't it make more sense to call systray_register() directly from wibox_refresh() *only* when the has_systray value changes?

True, it walks and register/unregistry at every awesome_refresh. Will work on fixing this.

> (Also, wtf @ the foreach loop in systray_refresh()? Are you really or-ing value into a boolean? Just set that one to true and "break;"?!)

Yeah, I found it pretty easy to use `foreach` to iterate the array but though that a plain `break` won't work as `foreach` expand to a nested `for` loop.
I will change it for a plain `for` loop and break there.

> Besides that, the code seems sane. Now let's do some testing...
> Results:
> - Alltray sucks. Never use it!

bad, but luckily not related to this patch.

> - Patch doesn't seem to break my config

I take that as a +1 that it works using builtin systray widgets.

> - Alltray annoyed me enough so that I don't feel like adding/removing systrays dynamically, sorry

I didn't understood this, you mean that you didn't tested adding/removing systrays dinamically like attaching/deattaching lua systray widget while awesome is running?

> I'd vote for jd testing this himself and/or merging the next version of the patches (really, no need to walk all the wiboxes).

Ok, but keep sending feedback, I will try to get it into acceptable form even if I don't reach the ideal of one-systray-per-screen.

> Oh and sorry if I sound harsh, I don't mean stuff as unfriendly as I write it.8

No prob, the coin is flipping. thanks for your feedback!
Comment by Uli Schlachter (psychon) - Friday, 11 June 2010, 17:35 GMT
Suggestion:
"Is there a systray in this wibox" or something like that.

Also, I didn't know that foreach() does some nested loops. Perhaps that code could be moved into its own static function so that one can use "return" as a replacement for "break"?

> I didn't understood this, you mean that you didn't tested adding/removing systrays dinamically like attaching/deattaching lua systray widget while awesome is running?
Yeah, exactly.

Loading...