tray module enhancements #1145

alebastr · 2021-06-30T07:33:50Z

Assorted enhancements for tray module.

I had to drop the tooltip icon changes with intent to finish those later, once I have a complete grasp of a possible implementation.
Need to take another look at update_pending_ for possibility of races. Shouldn't be a blocker for the merge though, as I haven't found anything harmful so far. Only a minuscule chance of missing a property update.

Changes:

Try to optimize updates by assuming that signals don't lie and could be using to infer all modified properties. A somewhat risky assumption, given how broken is an average sni client implementation, but I've seen other trays working like that.
Support tooltips for tray items (text only).
Support Scroll SNI method.
Support Status property: hide Passive items by default; attach corresponding CSS classes to the icon.

Fixes #1138, #1150

alebastr · 2021-07-20T06:51:27Z

@12101111, do you mind checking if the CPU usage is acceptable with this PR?
I haven't managed to get a good data with perf, likely because most of my profiling experience is with other tools and I don't have access to any of those at home ☹️

Alexays · 2021-07-21T08:37:49Z

src/modules/sni/item.cpp

-      // TODO: tooltip
+      tooltip = get_variant<ToolTip>(value);
+      if (!tooltip.text.empty()) {
+        event_box.set_tooltip_markup(tooltip.text);


Alexays · 2021-07-21T08:38:42Z

src/modules/sni/item.cpp

+static const std::map<std::string_view, std::set<std::string_view>> signal2props = {
+    {"NewTitle", {"Title"}},
+    {"NewIcon", {"IconName", "IconPixmap"}},
+    // {"NewAttentionIcon", {"AttentionIconName", "AttentionIconPixmap", "AttentionMovieName"}},


No need of these mappings?

Commented mappings are things we don't support at the moment. I.e. no reason to process AttentionIconPixmap updates if we aren't capable of displaying it.

Alexays

Apart from some questions, LGTM
Thanks!

Comparing two GVariants is too expensive; let's collect the set of properties updated by each signal and apply them unconditionally.

On the `Passive` value of `Status` tray items would be hidden unless `show-passive-items` is set to true. On the `NeedsAttention` value of `Status` tray items will have a `.needs-attention` CSS class.

12101111 · 2021-07-26T02:49:21Z

@12101111, do you mind checking if the CPU usage is acceptable with this PR?
I haven't managed to get a good data with perf, likely because most of my profiling experience is with other tools and I don't have access to any of those at home frowning_face

Sorry for the late, the CPU usage is nearly to 0 and perf result looks great.

rieje · 2021-09-18T19:28:44Z

The issue that prompted #1150 was udiskie's --smart-tray option which only shows the icon if when actions are available. Prior to this fix, it will always show the icon. With this fix, the icon is appropriately hidden except on the first instance of waybar, where it will always show the icon. Killing and starting another instance correctly shows the icon as hidden.

I have waybar autostarted but even if disable autostart and start waybar for the first time at a later time, the tray icon is incorrectly showing until I kill the instance and start another. I am on latest version 0.9.8.

alebastr · 2021-09-19T01:13:01Z

@rieje can you please check waybar -l trace output when running the first time? Does it get the right Status from udiskie?

rieje · 2021-09-19T01:54:53Z

Ok, just did some quick testing and I was wrong--it happens randomly all the time. I run multiple waybars to see what happens (the status is passive on all these instances which is correct), and on each instance that starts, it may trigger show/hide of the udiskie icon on all other waybars independently. -l trace output of the typical instance.

alebastr · 2021-09-19T07:05:43Z

I believe it's not so random. I managed to reproduce the issue, and everything is right from our side: we're calling event_box.set_visible(false) for the udiskie tray item and subsequent event_box.get_visible() returns false as well.

Do you happen to notice if the issue is reproducible when udiskie is not the first icon in the tray? My suspicion is that the first item in a GtkBox cannot be hidden, but I'll need to check GTK sources to confirm that.

rieje · 2021-09-19T14:44:57Z

Yes, still happens if not the first (leftmost) icon. I see it as the second icon, and less frequently as the third (last) icon.

alebastr · 2021-09-20T09:45:31Z

So I got some time to debug the issue and found that it's our fault. I just never looked at the code dealing with updates on the whole tray module level and missed the right (or wrong in this case) line.

@rieje do you mind giving alebastr@dc67bcc a try? Worked for me, but I only did limited testing.

rieje · 2021-09-21T00:08:56Z

This works, I have not experienced any unexpected results at all. Looks fixed!

alebastr force-pushed the sni-enhancements branch from cf97482 to 8cbf722 Compare July 1, 2021 06:01

alebastr force-pushed the sni-enhancements branch from 8cbf722 to 3623811 Compare July 9, 2021 07:59

alebastr force-pushed the sni-enhancements branch from 3623811 to 58b511a Compare July 20, 2021 06:07

alebastr marked this pull request as ready for review July 20, 2021 06:43

Alexays reviewed Jul 21, 2021

View reviewed changes

Alexays approved these changes Jul 21, 2021

View reviewed changes

alebastr added 6 commits July 22, 2021 08:01

fix(tray): ignore unused WindowId property

929fc16

refactor(tray): infer changed properties from signal name

4b6253e

Comparing two GVariants is too expensive; let's collect the set of properties updated by each signal and apply them unconditionally.

feat(tray): implement tooltips (text only) for tray items

84a8f79

feat(tray): fallback to Title for items without ToolTip

1418f96

feat(tray): handle scroll events

245f7f4

feat(tray): handle Status property

a5fe6f4

On the `Passive` value of `Status` tray items would be hidden unless `show-passive-items` is set to true. On the `NeedsAttention` value of `Status` tray items will have a `.needs-attention` CSS class.

alebastr force-pushed the sni-enhancements branch from 58b511a to a5fe6f4 Compare July 22, 2021 15:07

Alexays merged commit b33be38 into Alexays:master Jul 23, 2021

alebastr deleted the sni-enhancements branch July 23, 2021 14:52

alebastr mentioned this pull request Oct 2, 2021

fix(tray): fix visibility of Passive items #1271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tray module enhancements #1145

tray module enhancements #1145

alebastr commented Jun 30, 2021 •

edited

Loading

alebastr commented Jul 20, 2021

Alexays Jul 21, 2021

Alexays Jul 21, 2021 •

edited

Loading

alebastr Jul 21, 2021

Alexays Jul 23, 2021

Alexays left a comment

12101111 commented Jul 26, 2021

rieje commented Sep 18, 2021

alebastr commented Sep 19, 2021

rieje commented Sep 19, 2021 •

edited

Loading

alebastr commented Sep 19, 2021

rieje commented Sep 19, 2021

alebastr commented Sep 20, 2021

rieje commented Sep 21, 2021

tray module enhancements #1145

tray module enhancements #1145

Conversation

alebastr commented Jun 30, 2021 • edited Loading

Changes:

alebastr commented Jul 20, 2021

Alexays Jul 21, 2021

Choose a reason for hiding this comment

Alexays Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

alebastr Jul 21, 2021

Choose a reason for hiding this comment

Alexays Jul 23, 2021

Choose a reason for hiding this comment

Alexays left a comment

Choose a reason for hiding this comment

12101111 commented Jul 26, 2021

rieje commented Sep 18, 2021

alebastr commented Sep 19, 2021

rieje commented Sep 19, 2021 • edited Loading

alebastr commented Sep 19, 2021

rieje commented Sep 19, 2021

alebastr commented Sep 20, 2021

rieje commented Sep 21, 2021

alebastr commented Jun 30, 2021 •

edited

Loading

Alexays Jul 21, 2021 •

edited

Loading

rieje commented Sep 19, 2021 •

edited

Loading