Bug 422042 - KNotification uses transient=true hint incorrectly
Summary: KNotification uses transient=true hint incorrectly
Status: RESOLVED FIXED
Alias: None
Product: frameworks-knotifications
Classification: Frameworks and Libraries
Component: general (show other bugs)
Version: 5.70.0
Platform: Arch Linux Linux
: NOR normal
Target Milestone: ---
Assignee: kdelibs bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-05-25 09:56 UTC by Zauberfisch
Modified: 2020-11-30 23:25 UTC (History)
8 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Zauberfisch 2020-05-25 09:56:41 UTC
SUMMARY

KNotification turns transient=true if persistent=false. But according to the specs, those 2 things are completely different and should not be connected in this way.

The spec https://developer.gnome.org/notification-spec/ has 2 hints:
resident -> information how to display a notification (basically saying timeout = infinity)
transient -> information how to handle a notification (eg to log it or to discard it)

I haven't looked into it to much, but as far as I can tell from https://invent.kde.org/frameworks/knotifications/-/blob/master/src/notifybypopup.cpp#L297 KNotification will do the following:

if (persistent == true)
  timeout = 0 (infinite)
else
  transient = true

I believe a notification framework should expose an api for applications to set transient and resident directly and independent of each other.


I have observed this behavior in applications that use KNotification on non KDE plasma desktops (xfce+xfce-notifyd, i3+dunst). If and how a plasma desktop is effected I have not tested.

STEPS TO REPRODUCE
1. use xfce-notifyd or dunst as notification daemon
2. trigger a notification via NotifyByPopupPrivate::sendNotificationToServer (for example the quassel irc client does that for private messages if the irc client is not focused)
3. observe the dbus messages. 

OBSERVED RESULT

The application does not set transient, but the dbus message contains a transient=true hint.

Here the output of dbus-monitor when receving a notification in the quassel irc client:
http://paste.zauberfisch.com/t/5ecb18a069345/quassel-notify.txt

EXPECTED RESULT

The application should not set transient=true, unless the application has specified to do so.


SOFTWARE/OS VERSIONS
OS: arch linux
DE: xfce, i3
KDE Frameworks Version: 5.70.0
Qt Version: 5.14.2

ADDITIONAL INFORMATION

I believe this issue on the xfce-notifyd bugtracker is related: https://gitlab.xfce.org/apps/xfce4-notifyd/-/issues/25
Comment 1 Ulrich Norbisrath 2020-07-17 02:36:32 UTC
I confirm that this makes it very hard to use kdeconnect on a non kde desktop as described in https://gitlab.xfce.org/apps/xfce4-notifyd/-/issues/25.
Comment 2 Zauberfisch 2020-07-20 01:45:59 UTC
@albertvaka I found the following commit from you in 2017:

https://invent.kde.org/frameworks/knotifications/-/commit/579b75aac8e2323e3b16fc7f114efd10d4a77e07

The commit message says "Mark non-persistent notifications as transient", but to my understanding the code is the opposite:

    if (!(notification->flags() & KNotification::Persistent)) {
        hintsMap[QStringLiteral("transient")] = true;
    }

can you confirm that this was a typo, and that it should be & !KNotification::Persistent instead?
Comment 3 Zauberfisch 2020-07-20 01:46:16 UTC
@albertvaka I found the following commit from you in 2017:

https://invent.kde.org/frameworks/knotifications/-/commit/579b75aac8e2323e3b16fc7f114efd10d4a77e07

The commit message says "Mark non-persistent notifications as transient", but to my understanding the code is the opposite:

    if (!(notification->flags() & KNotification::Persistent)) {
        hintsMap[QStringLiteral("transient")] = true;
    }

can you confirm that this was a typo, and that it should be & !KNotification::Persistent instead?
Comment 4 Ulrich Norbisrath 2020-10-09 14:04:45 UTC
Just confirming that this is still an issue. We are currently trying to discuss this problem at https://gitlab.xfce.org/apps/xfce4-notifyd/-/issues/25#note_17084, but maybe should move the discussion here. However, I assume that nobody in kde will notice this problem as the notifications do show up in kde's notification log and stay there.
Comment 5 Nicolas Fella 2020-10-24 17:59:28 UTC
(In reply to Zauberfisch from comment #3)
> @albertvaka I found the following commit from you in 2017:
> 
> https://invent.kde.org/frameworks/knotifications/-/commit/
> 579b75aac8e2323e3b16fc7f114efd10d4a77e07
> 
> The commit message says "Mark non-persistent notifications as transient",
> but to my understanding the code is the opposite:
> 
>     if (!(notification->flags() & KNotification::Persistent)) {
>         hintsMap[QStringLiteral("transient")] = true;
>     }
> 
> can you confirm that this was a typo, and that it should be &
> !KNotification::Persistent instead?

The code works as it was intended by the author (a single & is bitwise or, not boolean or).

However I agree that setting transient here is questionable according to the spec.
The commit message indicates that this was done for reasons though so before we go and remove it I'd like to check how it affects Gnome though
Comment 6 Zauberfisch 2020-10-24 18:21:25 UTC
Yeah, you are right, I've misread that when I skimmed the code. I missed the ! in front. 

But meanwhile I actually had a chat with "kbroulik" in #kde-devel and he actually mentioned he previously already considered outright removing transient support because it was used incorrectly and because kde notifications handle the transient usecase differently and have no built-in mechanism to specify something as transient.

So he and I agreed that the short time solution is to just remove that.
Longtime he may introduce a new API to knotifications and the whole of KDE to allow applications to specify if they want transient or not.

I've further offered to do a PR for this fix, but I do want to do some testing before I submit it, I just haven't gotten around to that yet.
Comment 7 Kai Uwe Broulik 2020-10-24 18:51:11 UTC
Thanks for getting in touch and looking forward to your merge request :)
Comment 8 Nicolas Fella 2020-10-24 19:28:17 UTC
I gave it a try on Gnome. Removing the transient flag makes the notification actually show up in their notification history thing, which is what we want(?).

IMO you can go ahead and make a patch
Comment 9 Zauberfisch 2020-10-24 19:32:21 UTC
excellent. Yes, showing up in history is what we want.

Hopefully I'll find time in the coming days to make that merge request.
Comment 10 Bug Janitor Service 2020-11-23 00:59:01 UTC
A possibly relevant merge request was started @ https://invent.kde.org/frameworks/knotifications/-/merge_requests/21
Comment 11 Nicolas Fella 2020-11-30 23:25:17 UTC
Git commit 2f65c6b1b233383188614efa4589cdab6dd3a7b4 by Nicolas Fella.
Committed on 30/11/2020 at 22:51.
Pushed by nicolasfella into branch 'master'.

Don't pass transient hint

Not persistent doesn not imply transient, that is an unrelated property.

There are vaild use cases for transient notifications, but it shouldn't
be set unconditionally.

M  +0    -4    src/notifybypopup.cpp

https://invent.kde.org/frameworks/knotifications/commit/2f65c6b1b233383188614efa4589cdab6dd3a7b4