-
Notifications
You must be signed in to change notification settings - Fork 342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement x-canonical-private-synchronous hint #552
Implement x-canonical-private-synchronous hint #552
Conversation
Hi! Thanks a lot for the contribution. From #159
Has this changed? Is Notify-OSD (and this hint) still being used in newer versions of ubuntu? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Code looks good and I couldn't find anything really wrong with the implementation. Really good work!
Before I merge this I want to setup an ubuntu VM to see how this hint and capability are handled by the rest of the OS. I'm a bit hesitant to have the capability on by default since canonical might expect any implementation that has it to be theirs and send even more non-standard notifications/calls that dunst might not be able to deal with - Will have to double check it.
I'm also requesting a review from @bebehei here since he's worked a lot with the queues code so something might catch his eye.
IIRC this didn't hold up. It's actually not GNOME's behavior, and inherited by Unity. But I personally hadn't been able to run dunst successfully in a GNOME environment, because it will get killed by gnome. But that's all I can remember so far. Code looks quite good. Have to fire up my Ubuntu VMs and test it. |
Also I'd definitely give the concept of notification bubbles a read: https://wiki.ubuntu.com/NotifyOSD @Gravemind What client do you use to make this feature important? |
I don't actually know about ubuntu either, yet... I'm on arch+i3-wm, and I was looking to get neater volume/brightness/redshift notifications (own scripts bash and python), and I found "x-canonical-private-synchronous" more than a few of times in projects and Q&A code. Adding this "known" hint to dunst seemed the cleaner solution to me, here. But I agree it could be bad elsewhere, more testing needed... |
I'd say you're taking the wrong approach here: Adding support of an implementation-specific detail of a program that you aren't using or that familiar with. Id replace solutions have been talked about before, but as you said most of them are hack-ish with many edge cases. I agree with your rationale that this would be a good addition but lets leave the canonical specific parts out until someone using ubuntu implements the gnome specific hint properly. I doubt there are many dunst users on ubuntus default setup - I tried setting it up and looks like gnome-shell takes over the notification address which I couldn't figure out how to disable. Maybe rename the hint to |
@bebehei Any elaboration on why you think this is such a bad idea? :p Sure, we're going down the implementation-specific rabbit hole but there is no similar standardized hint so I can't see any better solution (other than relying on canonicals hints) |
Ok. As you said, adding support for a hint This should be enough to discard this feature. You don't have to implement it in the server (dunst), you also have to implement it in the client. Looking realisticly on dunst's "market share", requesting implementation in common clients is a silly job. But instead of asking "How can we solve this?", the question should rather be "Do we actually need it?"! NotifyOSD introduces two types of notifications: "confirmation bubbles" and "notification bubbles". Both sent over the notification wire. And to mark those notifications, which should show up as confirmation bubbles, these are marked with the Does dunst currently have any possibility to handle such "confirmation bubbles" (or speaking in dunst terms "confirmation notifications") in the way they are meant? No. There's neither a place, nor an algorithm to sort the notifcations properly. So, introducing such a hint would require at first a concept of handling these.
And while you read this sentence, you @tsipinakis as an alert reader might see the contradiction here. You send two types of "notifications" with different semantics over the same wire. And the Gnome team found out about this. They do not send anymore these confirmations over the FDN DBus socket, they use a different scheme. (Disclaimer: I found out about this right now, while researching this PR.) So it's not even, that Ubuntu changed away. Gnome doesn't even support a similar hint. They even turned their backs. So, I would love to implement such a thing. But without seeing the possibility on the horizon to later upstream the semantics into the notification spec, it'll stay forever a dunst specific rabbithole. And as we haven't got a significant market share, this won't find any wide spread. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestingly, my own volume manager sends a synchronous:volume
hint. https://github.com/graysky2/pulseaudio-ctl/blob/26a9ee5/common/pulseaudio-ctl.in#L205
So at least I should give a review.
src/notification.c
Outdated
n->summary = n->summary ? n->summary : g_strdup(""); | ||
n->body = n->body ? n->body : g_strdup(""); | ||
n->category = n->category ? n->category : g_strdup(""); | ||
n->synchronous = n->synchronous ? n->synchronous : g_strdup(""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pointless. You'll free the string later directly.
src/notification.c
Outdated
@@ -356,6 +359,8 @@ void notification_init(struct notification *n) | |||
/* Sanitize misc hints */ | |||
if (n->progress < 0) | |||
n->progress = -1; | |||
if (STR_EMPTY(n->synchronous)) | |||
g_clear_pointer(&n->synchronous, g_free); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the above comment.
src/notification.h
Outdated
@@ -74,6 +74,7 @@ struct notification { | |||
bool transient; /**< timeout albeit user is idle */ | |||
int progress; /**< percentage (-1: undefined) */ | |||
int history_ignore; /**< push to history or free directly */ | |||
char *synchronous; /**< x-canonical-private-synchronous */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please give in a more meaningful comment.
src/notification.c
Outdated
@@ -66,6 +66,7 @@ void notification_print(const struct notification *n) | |||
printf("\tframe: %s\n", n->colors[ColFrame]); | |||
printf("\tfullscreen: %s\n", enum_to_string_fullscreen(n->fullscreen)); | |||
printf("\tprogress: %d\n", n->progress); | |||
printf("\tsynchronous: %s\n", n->synchronous); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will print out none
always.
printf("\tsynchronous: %s\n", n->synchronous); | |
if (n->synchronous) | |
printf("\tsynchronous: %s\n", n->synchronous); |
src/queues.c
Outdated
bool queues_notification_replace_synchronous(struct notification *new) | ||
{ | ||
GQueue *allqueues[] = { displayed, waiting }; | ||
for (int i = 0; i < sizeof(allqueues)/sizeof(GList*); i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (int i = 0; i < sizeof(allqueues)/sizeof(GList*); i++) { | |
for (int i = 0; i < sizeof(allqueues)/sizeof(GQueue*); i++) { |
But a lot of dunsts market share is more power level users with i3 or other window managers that wish to implement something similar. This is not something new even, this has been mentioned again[#114], and again[#229], and again[#341], and again[#384] and we mostly point users to dunstify's Unless someone is running a daemon it's unlikely they'll opt for a so they will settle for b. I think its worth it as a feature in one form or another, and since @Gravemind already went through the trouble of implementing it as a hint I thought it's as good as any.
My point in a sentence is that everything you said is true but a lot dunst users have tried and are hacking their way to something similar with it, adding a mechanism to support this without weird edge cases wouldn't hurt. I'm going to bed for now, let me sleep on it and see if I have an idea in mind tomorrow. Any more ideas appreciated as usual. |
e38abd4
to
81a4219
Compare
I tested xubuntu: Xfce doesn't seem to react to the x-canonical-private-synchronous hint from notify-send (EDIT: nor "synchronous"). I greped many notification server source codes, and I only found references to a x-canonical-private-synchronous in notify-osd. And I didn't find any "synchronous" hint reference (@bebehei, but interesting...). Given how easy it is to find references to
I cannot disagree. Maybe I could add the hint name as setting in I thought of using
But if there is a better way, I don't mind dropping mine and wait for it, rather than clutter the code now. |
I added it in dunstrc, disabled by default. Explicitly documented it as non-standard. I'm not pushing for a merge, just experimenting, and it felt a bit better to me: less "down the rabbit hole", so what do you think ? |
I'd like to prove this as a bad idea. Just to make this a configuration option, won't solve the things. You will have different clients and those clients will use different namespaces for the synchronous hints. I've got a client, which uses So, with a skalar option, you won't find any improvement. Implementation wise we would have to use at least an array for that. And still it wouldn't justify to create a new configuration option (there is no point in not hardcoding the stuff). @Gravemind Sorry for being so harsh to you. I understood, that we need such a feature. It's ultra useful and has got much more power than But just blindly copying the behavior of @tsipinakis The points you outlined in your comment are very true, but I'd love to create software, which has no hacky solutions on either side. Neither the client and neither the server. Personally, this is my list of things, I would change these things:
|
@Gravemind I wanted to give it another review. Instead, I pushed some code improvements. I don't want to have them lying around on my copy: Gravemind/dunst@x-canonical-private-synchronous...bebehei:x-canonical-private-synchronous Most important bit: We have to send a signal when we close the old "synchronous" notification. |
5b34997
to
21a44dd
Compare
@bebehei: Fixes (reviewed) integrated. My take on the changes:
2./3. When I said "but feels overkill...", I was thinking about something like that: [global]
# comma separated list of hint name
# only used if replace_by = hints
replace_by_hints = "replaces_name,synchronous,x-canonical-private-synchronous"
# enum: never|summary|body|category|appname|hints|...
replace_by = "hints"
[my_custom_rule]
category = "network"
# replace if other also matched a replace_by=appname and has the same appname
replace_by = "appname" I think this is a way that keeps it not too disruptive for the code, I'm ready to give it a shot if it seems good ?
|
I agree with @bebehei that this is going a bit too far by making the name customizable, we can hardcode the most used hints and if a user finds an unsupported one in the wild they can either change it in the client or send a PR to add it as a synonym.
Breaking down what this adds is
As I said above I believe option 1 is redundant and we could do with out it. I'd like to avoid adding options like that to the config. I'm ambivalent to option 2 but I believe this is also going a bit too far and we could do this simpler by simply giving access to the Example: (Going off @bebehei 's pulseaudio-ctl example as if it didn't send the synchronous hint) [merge-volume]
appname="pulseaudio-ctl"
set_synchronous="volume" I'm using the Edit: How about |
@tsipinakis Full ACK. The only point, which is left: How to name it? I'd love to use a name, which says more than synchronous.
Well, "merging" is already occupied with completely different semantics. See the NotifyOSD docs. It merges notifications together but doesn't replace them. Some random ideas:
Well, this was right now just a current braindump. I won't pick any name right now. I'll go to bed. But comments requested and appreciated. Edit: Maybe something like |
21a44dd
to
5596c2c
Compare
Ok, I re-implemented it with a Works for me. Note on the new implem:
I kept the old version in Gravemind:x-canonical-private-synchronous-bak |
17e94b9
to
3bfb2e8
Compare
@Gravemind Well, the implementation might be the easiest thing here. We (all three of us discussing in this PR) have pretty good C skills to write the final implementation down in less than an hour. But the point is, what do we actually want? And I'm waiting currently for @tsipinakis' feedback first. He'll come up with a good idea. |
You give me way too much credit :p I took a quick look around by doing a github search with variations of the hints name and, as expected, the world of unstandardised hints is a mess. Identical hint names found:
Clients I looked at check for capabilities for
I haven't found any one that looks for the So we should at least implement these. Going back on the naming issue I'm unsure of the usefulness of picking a better name. It removes confusion for the user trying to use it solely inside rules, but given that the name synchronous has been used a lot it can also cause confusion for users that are looking how to control the hint. Maybe consider having the rule have the same name as the hint? |
It still helps me understand the architecture and subtleties better. And I find it easier judge hands-on (good or bad), so I thought I'd share. But I'll let you guys work on it then, I'll be happy with any implem in the end. I was wondering if "synchronous" should close the old notif and insert the new with a new id, or, replace the old using the same id (like So, I find "synchronous" confusing enough to be more comfortable with a properly named, "well" defined, dunst-specific feature, where the "synchronous" feature can piggyback on it. It would be easier to justify implementation choices. (of course, because there is no official synchronous spec) |
I would pick a new name, and name the rule, the Why? We currently don't provide any behavior in dunst, which has to do something with "synchronous" (and I'm not eager to implement such behavior). Even the code of this PR doesn't provide it and for example the adjective unique would match much better. Those synchronous notifications are named synchronous, because they show up immediately (synchronous to the action you made). That they replace each other is just a byproduct. I'd love to have this byproduct and I don't care about canonical's main goal. So, why not creating a feature in dunst, which covers this byproduct and maps the old hints as best as possible? |
1 similar comment
I would pick a new name, and name the rule, the Why? We currently don't provide any behavior in dunst, which has to do something with "synchronous" (and I'm not eager to implement such behavior). Even the code of this PR doesn't provide it and for example the adjective unique would match much better. Those synchronous notifications are named synchronous, because they show up immediately (synchronous to the action you made). That they replace each other is just a byproduct. I'd love to have this byproduct and I don't care about canonical's main goal. So, why not creating a feature in dunst, which covers this byproduct and maps the old hints as best as possible? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go on gradually. At least these two bits have to get removed.
ef9c07d
to
3fe352b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Gravemind FYI Github doesn't send notifications for force pushes, didn't notice you had addressed the review comments.
@bebehei I can't think of a better name than stack_tag
got anything else? Otherwise I'd say this is ready to be merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just minor nitpicks.
3fe352b
to
433d3d3
Compare
Implements "x-dunst-stack-tag", "x-canonical-private-synchronous", "private-synchronous", and "synchronous" hints via the "stack_tag" feature.
433d3d3
to
d879d70
Compare
Merged! Thanks for implementing this @Gravemind. Sorry this PR turned out to be more controversial than usual. |
FYI, a standardization proposal is being discussed here: https://gitlab.freedesktop.org/xdg/xdg-specs/-/issues/77 |
I took a shot at implementing a trivial "x-canonical-private-synchronous" ( #159 ).
I implemented it as simple string equivalent to a "--replace-id". I'm not
sure if there should be more to it (according to the canonical spec ?).
I did not handle
x-canonical-
private-synchronous
because I didn't want to add more non-standardized hints than necessary.I couldn't wait for an official implem, just happy to share my take on it.