| Summary: | KNotification uses transient=true hint incorrectly | ||
|---|---|---|---|
| Product: | [Frameworks and Libraries] frameworks-knotifications | Reporter: | Zauberfisch <accounts.bugs.kde.org> |
| Component: | general | Assignee: | kdelibs bugs <kdelibs-bugs-null> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | accounts.bugs.kde.org, albertvaka, kde, kde, kdelibs-bugs-null, nate, nicolas.fella, njkevlani |
| Priority: | NOR | ||
| Version First Reported In: | 5.70.0 | ||
| Target Milestone: | --- | ||
| Platform: | Arch Linux | ||
| OS: | Linux | ||
| Latest Commit: | https://invent.kde.org/frameworks/knotifications/commit/2f65c6b1b233383188614efa4589cdab6dd3a7b4 | Version Fixed/Implemented In: | |
| Sentry Crash Report: | |||
|
Description
Zauberfisch
2020-05-25 09:56:41 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. @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? @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? 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. (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 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. Thanks for getting in touch and looking forward to your merge request :) 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 excellent. Yes, showing up in history is what we want. Hopefully I'll find time in the coming days to make that merge request. A possibly relevant merge request was started @ https://invent.kde.org/frameworks/knotifications/-/merge_requests/21 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 |