Tray D-Bus Menu #6249
Tray D-Bus Menu #6249
Conversation
Yes, this PR has been looking for a new owner for a while. Thanks for taking over! |
|
@FlexW Thank you for doing this. I would like to support you however I can. Please see my branch from #5161 work; https://github.com/nmschulte/sway/tree/fix-tray-updates. It has improved message handling to resolve some the standing issues of #5161; it is based upon current This is all I have for now; I will be following closely. Later I will check your list and my notes and share any new items I've thought about to incorporate. |
Thank you for your support. I'll check your branch out. I think I had the same scaling issue that you described, and I think that issue is resolved in my branch. There was an issue with not specifying the correct size of the buffer when scaling is activated. If you want you can check out my branch and see if the scaling issue is gone for you too. |
|
👍 I hope to dive through the diff later this week.
…On Sun, May 2, 2021, 1:41 PM Felix Weilbach ***@***.***> wrote:
@FlexW <https://github.com/FlexW> Thank you for doing this. I would like
to support you however I can.
Please see my branch from #5161 <#5161>
work; https://github.com/nmschulte/sway/tree/fix-tray-updates. It has
improved message handling to resolve some the standing issues of #5161
<#5161>; it is based upon current
master/master, and I rebase it locally about once a week to continue
using it. Since 1.6 / wlroots 1.2/1.3, something has broken for me w/re:
non-1.0 DPI scaling and popups, such that most apps no longer work to show
menus in this scenario.
This is all I have for now; I will be following closely. Later I will
check your list and my notes and share any new items I've thought about to
incorporate.
Thank you for your support. I'll check your branch out. I think I had the
same scaling issue that you described, and I think that issue is resolved
in my branch. There was an issue with not specifying the correct size of
the buffer when scaling is activated. If you want you can check out my
branch and see if the scaling issue is gone for you too.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6249 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACBFAT6TYWVL7JLF63C5XS3TLWMENANCNFSM437SF2HQ>
.
|
Does #6147 match what you're describing? If so and you're experiencing it with other apps, would be worth leaving a note in that thread. |
@Xyene I don't think #6147 describes the issue, but I can try with external displays; experiencing this on a laptop with 4K display, no external monitors. Zoom does strange things with its menus under Wayland such that the menus popup on the wrong output or don't show at all, depending which output they're parented, on a top-bottom-plus-(rotated-)straddling-sides 4-output DPI==1 stationary desktop setup though. Also I see #6148 with this same setup. |
😃 All of the popups seem to be working on this branch ( |
Thats great to hear. I'm aware of that crash, it's because I don't handle the hotspots correct fow now. I'll let you know when I fixed it. |
FlexW
force-pushed
the
feature/tray-dbus-menu
branch
from
3357c6b
to
a9e77fe
Compare
|
@nmschulte The crash that you experienced is gone now. You may experience other crashes or bugs. It's WIP. Just wanted to let you know :) |
FlexW
force-pushed
the
feature/tray-dbus-menu
branch
from
a9e77fe
to
8914389
Compare
|
@FlexW after merging |
|
uneducated question: is there a possibility that this might work with |
|
It should, yes. |
FlexW
force-pushed
the
feature/tray-dbus-menu
branch
4 times, most recently
from
c56a8cb
to
e212e13
Compare
|
@nmschulte Finally I found some time to work on it. Should now work better. So far I couldn't crash it anymore and it works as I would expect it with all my tray icons (nmapplet, nextcloud, udiskie, blueman, gammastep-indicator). I would appreciate it if you could do some tests too (I rebased against the latest master). Maybe you also know some more applications with which I can test. |
|
They don't run "in parallel", there's only one thread. The event loop will dispatch D-Bus events and Wayland events one after the other. No need for any kind of locking or race condition prevention. |
|
I finally managed to build this (caved and built I managed to crash Both Sub-menus are placed a bit off for me (2x scale), and there's an odd looking "bar"/line/border at the bottom of the sub-menus.
Finally, and I'm not confident in my sway-term/understanding here, there's still something odd with focus, popups, and xwayland vs xdg_shell (backends?). |
FlexW
force-pushed
the
feature/tray-dbus-menu
branch
5 times, most recently
from
5944eb9
to
f641e58
Compare
|
Thank you for your feedback @nmschulte. I would like to get this pr reviewed, because I think it works well enough and provides everything to do basic work with tray menus. There are some things that could be better, but I think it is usable. I can not crash it anymore, Maybe one of the maintainers can help with getting the last flickering and focus problems away. |
FlexW
force-pushed
the
feature/tray-dbus-menu
branch
from
83cd807
to
37cd0d0
Compare
|
@kennylevinsen I resolved all review comments. I appreciate another review round. |
FlexW
force-pushed
the
feature/tray-dbus-menu
branch
from
37cd0d0
to
ce789fe
Compare
|
FYI, you can squash when the PR is merged so you don't have to force-push and lose the history. |
|
I've been using this commit for a few weeks now at it's working great, but just noticed that the tray menus only display on my leftmost screen, but not on my right screen. |
|
I noticed that right clicking the QOwnNotes icon (among others that I forgot) have the ability to crash the bar (I use this with i3status-rust). |
Same here. I've played around with it a bit and found that the menu only shows on the display rooted at coordinates 0 0. So with the following in sway config it shows only on eDP-1: With this it shows on DP-1 and not on eDP-1: It doesn't matter whether the coordinates are negative or not. With this it still only shows on eDP-1: With this config it shows on neither display: With this config it shows on DP-1 but offset to the right by what could well be 300 pixels: With this config swaybar crashes when clicking on any tray icon on any display: With "shows" I mean that the icons are there on all displays but only on one display clicking on them actually shows the menu. On the other nothing happens when clicking the icon. The click does seem to reach swaybar though since in both cases it logs: (In this case blueman tray just as an indicator that the action is received and the actual error very likely totally unrelated to showing the menu or not as it happened with older commits as well which showed the menu on both displays.) |
|
@michaelweiser @bim9262 This seems the same as the positioning issue I mentioned on #6249 (comment) |
|
@joanbm: Duh, yes. I can confirm that the change from your comment fixes the issue. Thanks! |
FlexW
force-pushed
the
feature/tray-dbus-menu
branch
from
ce789fe
to
69e2fe7
Compare
Co-authored-by: Ian Fan <ianfan0@gmail.com> Co-authored-by: Nathan Schulte <nmschulte@gmail.com> Signed-off-by: Felix Weilbach <felix.weilbach@t-online.de>
FlexW
force-pushed
the
feature/tray-dbus-menu
branch
from
69e2fe7
to
2f304ef
Compare
|
I have replicated @TimB87 crash with
Notes:
Logs: I had debug symbols installed so here is the correct stacktrace from the core dump: The EDIT: added proper stack track since I actually had debug symbols available and a coredump |
| struct swaybar_dbusmenu_menu *focused_menu = tray->menu_pointer_focus; | ||
| if (!(tray && tray->menu && focused_menu)) { |
There was a problem hiding this comment.
I've started testing this branch out, and I just got a segmentation fault here due to tray being NULL, so this !tray check may need to occur sooner.
However, the context was that I was running a bar without a tray. I'm not entirely sure why this callback even runs in that case, it would be good to check whether it makes sense to try to disable it.
There was a problem hiding this comment.
Could you test if my patch below alleviates the issue? The null check should probably still be fixed, otherwise it serves no purpose.
|
Hey, I was watching this PR frequently and happy about the activity but it seems to stagnate a bit, is there something I / we can support on to get this further? Personally it's an important feature to me :) |
|
I had to fix some uninitialized variables and add some check to not invoke the dbus menu when the tray is disabled and while it works mostly well, I still get some random crashes when interacting with the tray with some inside You can find my changes below.diff --git a/swaybar/input.c b/swaybar/input.c
index 4ee4915991..8487eeb74a 100644
--- a/swaybar/input.c
+++ b/swaybar/input.c
@@ -118,8 +118,9 @@ static void wl_pointer_enter(void *data, struct wl_pointer *wl_pointer,
update_cursor(seat);
#if HAVE_TRAY
- if (dbusmenu_pointer_enter(data, wl_pointer, serial, surface, surface_x,
- surface_y)) {
+ struct swaybar_config *config = seat->bar->config;
+ if (!config->tray_hidden && dbusmenu_pointer_enter(data, wl_pointer, serial,
+ surface, surface_x, surface_y)) {
return;
}
#endif
@@ -127,13 +128,14 @@ static void wl_pointer_enter(void *data, struct wl_pointer *wl_pointer,
static void wl_pointer_leave(void *data, struct wl_pointer *wl_pointer,
uint32_t serial, struct wl_surface *surface) {
+ struct swaybar_seat *seat = data;
#if HAVE_TRAY
- if (dbusmenu_pointer_leave(data, wl_pointer, serial, surface)) {
+ struct swaybar_config *config = seat->bar->config;
+ if (!config->tray_hidden && dbusmenu_pointer_leave(data, wl_pointer, serial,
+ surface)) {
return;
}
#endif
-
- struct swaybar_seat *seat = data;
seat->pointer.current = NULL;
}
@@ -143,7 +145,9 @@ static void wl_pointer_motion(void *data, struct wl_pointer *wl_pointer,
seat->pointer.x = wl_fixed_to_double(surface_x);
seat->pointer.y = wl_fixed_to_double(surface_y);
#if HAVE_TRAY
- if (dbusmenu_pointer_motion(data, wl_pointer, time, surface_x, surface_y)) {
+ struct swaybar_config *config = seat->bar->config;
+ if (!config->tray_hidden && dbusmenu_pointer_motion(data, wl_pointer, time,
+ surface_x, surface_y)) {
return;
}
#endif
@@ -185,8 +189,9 @@ static void wl_pointer_button(void *data, struct wl_pointer *wl_pointer,
uint32_t serial, uint32_t time, uint32_t button, uint32_t state) {
struct swaybar_seat *seat = data;
#if HAVE_TRAY
- if (dbusmenu_pointer_button(seat, wl_pointer, serial, time, button,
- state)) {
+ struct swaybar_config *config = seat->bar->config;
+ if (!config->tray_hidden && dbusmenu_pointer_button(seat, wl_pointer, serial,
+ time, button, state)) {
return;
}
#endif
@@ -308,7 +313,8 @@ static void wl_pointer_axis(void *data, struct wl_pointer *wl_pointer,
}
#if HAVE_TRAY
- if (dbusmenu_pointer_axis(data, wl_pointer)) {
+ struct swaybar_config *config = seat->bar->config;
+ if (!config->tray_hidden && dbusmenu_pointer_axis(data, wl_pointer)) {
return;
}
#endif
@@ -330,7 +336,8 @@ static void wl_pointer_frame(void *data, struct wl_pointer *wl_pointer) {
struct swaybar_output *output = pointer->current;
#if HAVE_TRAY
- if (dbusmenu_pointer_frame(data, wl_pointer)) {
+ struct swaybar_config *config = seat->bar->config;
+ if (!config->tray_hidden && dbusmenu_pointer_frame(data, wl_pointer)) {
return;
}
#endif
diff --git a/swaybar/tray/dbusmenu.c b/swaybar/tray/dbusmenu.c
index 8821cacaec..909730bea5 100644
--- a/swaybar/tray/dbusmenu.c
+++ b/swaybar/tray/dbusmenu.c
@@ -592,7 +592,7 @@ static void swaybar_dbusmenu_draw_menu(struct swaybar_dbusmenu_menu *menu,
cairo_surface_destroy(recorder);
return;
}
- int surface_x, surface_y, surface_width, surface_height;
+ int surface_x = 0, surface_y = 0, surface_width = 0, surface_height = 0;
draw_menu_items(cairo, menu, &surface_x, &surface_y, &surface_width,
&surface_height, open);
diff --git a/swaybar/tray/tray.c b/swaybar/tray/tray.c
index b0545f4a70..e4a680342b 100644
--- a/swaybar/tray/tray.c
+++ b/swaybar/tray/tray.c
@@ -110,7 +110,7 @@ static int cmp_output(const void *item, const void *cmp_to) {
uint32_t render_tray(cairo_t *cairo, struct swaybar_output *output, double *x) {
struct swaybar_config *config = output->bar->config;
- if (config->tray_outputs) {
+ if (config->tray_outputs && !config->tray_hidden) {
if (list_seq_find(config->tray_outputs, cmp_output, output) == -1) {
return 0;
}@FlexW Are you planing to continue your work on this? |
|
@FlorianFranzen ASAN+UBSAN should help diagnose the crashes. Afterwards, I recommend fuzzing the D-Bus API to ensure it is robust against e.g. malicious sandboxed flatpak apps. |
|
It’s quite possible that some of the crashes are due to libdbusmenu being buggy — it is unmaintained for several years now (since 2017, IIUC) and Waybar also has problems with it. |
|
Any news on this? |
swaywm/sway#7226 is merged swaywm/sway#6249 looks dead :( also simplified ./overlays
|
This patch no longer cleanly applies on newer versions of sway. |
|
Hello @NickHu,
Would you mind submitting an alternative PR? Thanks in advance! |
|
@NickHu would you mind fuzzing this to ensure it is safe and secure against malicious D-Bus clients? |
|
No thank you, I don't have either the expertise nor the willingness to undertake any development in sway. Anyone else is free to do whatever they like with my tiny contribution of rebasing. |
|
I've been daily driving this for a few days now, and besides one issue that I fixed here (llyyr@84481c2), it seems mostly stable now across the clients I use. fwiw I also rebased it on master here in case there's other people that want to test it out. https://github.com/llyyr/sway/tree/dbus-tray I don't really know what's needed to push this further besides some style nits I noticed |
Can you make a PR with all of the fixes? |
This pull request implements D-Bus menus for the tray icons. @ianyfan @nmschulte It seems like #5161 is dead, and I would like to take over. Please tell me if this is not the case, then I will close this pull request.
This pull request is based on #5161 and is a draft for now. Please don't test it yet. When the pull request is ready for testing, I will say so.
@ianyfan @nmschulte I created this early pull request to let you know I'm working on it. Just in case that we do not double the work. Maybe you could add some other points that should be addressed before this pull request can be merged. I would say keyboard navigation and touch support can be done in a second pr.