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
ce789fe to
69e2fe7
Compare
Co-authored-by: Ian Fan <[email protected]> Co-authored-by: Nathan Schulte <[email protected]> Signed-off-by: Felix Weilbach <[email protected]>
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? |
|
I have been keeping these patches on my own fork for a while now and I building a version of this package on Arch without any issues. What is preventing this from being merged? |
The tray-dbus-menu-1.9 branch on my fork contains all the patches and fixes from this branch. However, I'm not sure about the testing part. |
Feel free to open a PR then to drive this forward. |
Sure, I'll create a new branch, because that one is targeting the latest release, not main. |
|
Created #8405. Rebased it against master. |
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.