tray: implement d-bus menu #5161
Conversation
|
OS: Arch Linux EDIT: |
|
@artizirk by nothing else, do you mean tested or works? If the latter, which ones? X embedded menus don't work with this. (probably) |
|
By works I mean that menus open and are functional. Other applications like Slack and Dropbox do nothing currently |
|
looks like you are correct and xembed-sni-proxy does not use the dbusmenu at all. I tried out nm-applet that is compiled with appindicator support but with that menus don't work. |
|
Dropbox appears to be very badly behaved and will require further treatment. I can't even get Slack show its icon, but I don't use it otherwise, so maybe someone can help me with this. |
|
Slack and maybe other Electron based apps might require |
|
What could I do to test this PR? I am running Arch. Could I install this branch through the AUR? |
|
None of the menus display where I would expect, even relative to each other.
|
|
@nmschulte I think I fixed it, but I don't know whether it's a good fix. @loligans You can't install this branch through the AUR, I'm afraid, but you can pull this branch directly using git (relevant StackOverflow link if it helps), and then follow the instructions here to compile it manually. Thanks for the help! |
|
Fails to build with new wlroots: swaywm/wlroots@6977f3a;
|
|
You just need to rebase against master. |
|
Menus appear in the correct place and click interaction works. For the record,
|
|
Do you call the AboutToShow method? I believe udiskie uses it to choose which items should be visible. |
|
That does work, thanks. However, this change, along with submenus, requires fixing some logic paths, so I can't push it yet. |
|
Hi, this is an interesting pull request, but didn't get updated for some time. |
|
Sorry, I'm not sure when I will be able to work on this. Also, it's been a while since I worked on this, so I don't have all the ideas in my head any more. However, I believe the main problem was that I had originally written everything with the intention of loading everything as lazily as possible, which meant that every step of the process of opening a popup had its own callback, which then (sometimes conditionally) called the next callback in the process. Unfortunately, I had also made several simplifications in the process in order to get something done, namely not including submenus by only having a single popup instance, and not using the AboutToCall method, which adds a surprising amount of complexity to opening the popup, and it was starting to look like a callback hell. So the main fix, which I had started working on (and left commented), was to allow popups to nest, but keeping the required singleton Wayland interfaces, which needs defining new structs (including good names) and fix the logic in places that open popups so that they can open a specific submenu. However, along with the logic paths that were introduced with using the AboutToCall method, I wasn't sure whether I could keep adding on to what I had written or whether I needed to rethink the code structure. I am more than happy (and sorry) for someone else to take this up, and I can try to help by answering questions about it, but again, I'm not sure when I can come back to writing the code for it. Also, I had basically given up on keyboard and touch events, so if nothing has changed, I'm happy to drop that goal. However, I'm reticent to drop submenus since it is such a relatively large change in both UX and the code. |
|
Thank you for your answer and your work so far! I agree with you about the dropping of keyboard and touch event goals and even submenus for now. I would prefer to have some basic functionality and then evolve from there. |
|
Works great for me. |
ianyfan
force-pushed
the
tray
branch
2 times, most recently
from
3855b0d
to
f6c9985
Compare
|
Hi all, sorry for taking so long, but I think I got submenus working somewhat. However, I haven't been able to test it much so let me know if you have any problems with it. Unless keyboard events have miraculously begun working since I last worked on this, this will be the last feature I plan on implementing., and I will now be focused on cleaning up code paths, though I'm not sure how long that will take. If anyone else has been working on this in the meantime, your input is still welcome. Thanks all for trying this out. |
ianyfan
force-pushed
the
tray
branch
2 times, most recently
from
3ce35d8
to
714f28a
Compare
for sd-bus macro SD_BUS_METHOD_WITH_NAMES introduced in v242
No complaint, just wondering whether it's just me: I still experience frequent crashes of swaybar with the traybar changes merged in. So far only using |
|
I receive a |
|
Another issue with this DBus implementation is that if the menu contains a checkbox that changes state, it will simply not redraw and always seem on/off even if the action is performed when pressing the checlbox. I am maintainer of ActivityWatch and have been using this patch for a couple of weeks and this is the only issue I've found (no crashes which others mention). |
|
Hi! Ayatana Indicators upstream maintainer here. The implementation in GTK and Qt also update the menus while open. E.g. network-manager applet (nm-applet) updates the seen WiFi network while the indicator's menu is open. This should be part of this implementation IMHO. |
|
Furthermore, AppIndicator support via SNI DBus interface is only part of the Indicators implemention. It would be really cool, to see sway support Ayatana System Indicators. A render implementation for GTK is available in libayatana-indicator: As reference for a renderer applet implementation you can take a look at mate-indicator-applet: The Ayatana System Indicators are actually the more generic approach. In Ayatana Indicators we have one indicator (ayatana-indicator-application) providing a widget toolkit indepdent implementation of an AppIndicator. So maybe, providing system indicator support in Sway would be the much more generic approach (because AppIndicators would come with that out-of-the box). |
|
This PR is about SNI menus only. If you want support for Ayatana System Indicators (or anything else), please create a separate issue. |
|
|
||
| struct swaybar_popup *popup = sni->tray->popup; | ||
| if (popup->sni == sni) { | ||
| close_popup(popup); // TODO enhancement: redraw instead of closing |
There was a problem hiding this comment.
Clicking an item on the first tray menu interaction with Zoom's icon consistently crashes.
The issue is due to ItemsPropertiesUpdated signalling during the first interaction; Zoom updates the "Choose a Language" submenu to indicate the chosen language.
What happens is that close_popup is called before the AboutToShow callback triggers, and so the popup remains open and clicking on it causes SIGSEGV, as close_popup has set sni = NULL.
There was a problem hiding this comment.
I took this change one step further and tried to make LayoutUpdated and ItemsPropertiesUpdated signals automatically re-open the popup. It seems to be working, but is rather hacky with the popup_open global variable. https://github.com/nmschulte/sway/compare/master...nmschulte:fix-tray-updates?expand=1#diff-8acf2d71366c39e6519fbc4b635110efdb128c74ee2e17a06d9e09e4ef2354d8R400
It's not clear, but perhaps popup->sni is supposed to indicate that the popup "is open" ("is open": has opened after signalling AboutToShow and should not signal again until actually closed, not just close_popup called). popup->sni's lifecycle seems similar/same to popup->popup_surface.
With all of this, there is the popup hierarchy to worry about still: popup_surface->child; I think this is the main reason popup->sni's lifecycle is not quite the same as the popup_open cycle.
| } | ||
|
|
||
| struct swaybar_popup *popup = sni->tray->popup; | ||
| if (popup->sni == sni) { |
There was a problem hiding this comment.
This additional constraint allows Zoom's menu to function; the ItemsPropertiesUpdated callback changes are successfully applied, although they are to a submenu that isn't drawn in the first popup. I think perhaps this will need to traverse popup_surface->child hierarchy and additional logic to work in all cases; input on how to do this @ianyfan?
| if (popup->sni == sni) { | |
| if (popup->sni == sni && popup->popup_surface) { |
Udiskie sends a Something strange is happening with Zoom; Icon's in popups are not scaled according to DPI: 4K with As well, focus doesn't seem correct: I can type into Firefox and switch containers and workspaces after opening a menu, by both keyboard and mouse (e.g. scrolling, or putting cursor over the container). Clicking on a container that does not have focus will close the menu, but does not give it input/keyboard focus (it already has active/focus by way of cursor movement). |
|
Thanks for looking into this. I haven't really looked at this PR for a while because I got really frustrated with it. I couldn't figure out why the Besides that is this problematic scenario: suppose the user has opened a sequence of submenus It's a very unfun exercise. I hate edge cases. Apologies to all for the hold-up. P.S. I'm not sure about the scaling thing but it sounds like a simple fix, not sure where though. |
|
@ianyfan give my branch a test if you have a chance: https://github.com/nmschulte/sway/tree/fix-tray-updates; it's Which application has (updating) multiple nested menus I could test with? Fixed icon scaling, but I wonder if the scale should happen after the icon search when using an icon by name. |
Also, maybe related: I initially thought this to be a rf-related hardware bug/flaw, specific to toggling bluetooth on and off; see the |
I feel like item ids can be re-used across layout updates; if an application wants to retain submenus open it needs to re-use the ids. Dealing with the UX is a different problem entirely; the application has some control of this too w/re: timing of updates. I guess it could even consider I think NetworkManager is intelligent in this regard; we'll have to test
I only know these protocols by name right now, but |
This is the behavior with my work atop 62ec528 / swaywm/wlroots@306cf11: You can see that after clicking the menu, if the cursor moves the focus changes. I think this is expected.
I am able to see my containers and workspaces switch by simply clicking on active menu items. It does not always occur, but it happens quite a lot, and especially so with blueman-applet's "turn bluetooth on/off," it still seems relegated to menu items near the top of the menu. |
|
I'd love to help, see this make its way into v1.6: #5970 (https://github.com/swaywm/sway/milestone/7). |
|
@nmschulte Have you considered making a PR with your changes on top of @ianyfan's? |
|
@kennylevinsen indeed that is my plan. Presently with latest masters, some of my branch is not working (though, not crashing). I haven't checked this branch alone yet but I suspect it will present the same problems. I'm hoping to see this branch, at least, make it to v1.6. If this branch needs more work (e.g. my efforts), then I'm happy to "take over" for @ianyfan, or push separately. |
| @@ -363,7 +363,7 @@ static void handle_global(void *data, struct wl_registry *registry, | |||
| } | |||
| } else if (strcmp(interface, zwlr_layer_shell_v1_interface.name) == 0) { | |||
| bar->layer_shell = wl_registry_bind( | |||
| registry, name, &zwlr_layer_shell_v1_interface, 1); | |||
| registry, name, &zwlr_layer_shell_v1_interface, 2); | |||
There was a problem hiding this comment.
IIRC xdg-popup support in layer-shell is only available since v2?
|
Closed to due being severely outdated, but development continues in #6249. |
See #5161 (comment)
Help test please.
Or just use to your heart's content because basic menu operations work (i.e. just opening and clicking), so probably like 98% of what normal users do.
Not sure whether to change rendering style or expose some configuration options.
Waiting on #4781 too, so that we don't have to change the bar layer in order to render the popup over other applications.TODO: