lxqt / lxqt-notificationd

The LXQt notification daemon
https://lxqt.github.io
GNU Lesser General Public License v2.1
62 stars 38 forks source link

[Feature request] Add "Do not disturb" mode #270

Closed stefonarch closed 3 years ago

stefonarch commented 3 years ago

Sometimes it could be useful to postpone all notifications (work, movie ecc) for an amount of time (like ½h, 1h, 2h) It could also be triggered somehow by "pause idleness watcher" with fullscreen or when https://github.com/lxqt/lxqt-powermanagement/pull/260 is implemented. adding an checkbox "pause also notification".

Possible Solution
jmfernandez commented 3 years ago

Yes, it would be very useful in this "teleconferences" world, in order to avoid showing any notification for half an our or one hour, for instance, in order to avoid them when screen is being shared, or video or slideshow are in progress.

stefonarch commented 3 years ago

At the moment this can be achieved by stopping the notification module in "Session Settings", but there will be no "missed" notifications when reactivating.

jmfernandez commented 3 years ago

At the moment this can be achieved by stopping the notification module in "Session Settings", but there will be no "missed" notifications when reactivating.

An additional drawback is that it will not be automatically re-enable after a timeout.

tsujan commented 3 years ago

There's a design problem that should be solved before adding codes for the new mode:

We show the tray icon only when there are unattended notifications. So, unlike in power management, we can't add a pause menu there. On the other hand, if we always show the tray icon, it'll lose its usefulness as a reminder that there are unattended notifications.

Adding a pause setting in the notifications config dialog isn't the solution because it's hidden from the user.

Using the tray icon of power management for this is impossible. Even if it was possible, we shouldn't do it because notifications aren't related to power management — to say nothing of computers without battery.

Showing the icon permanently and using another icon when there are unattended notifications would be a solution if there was a good icon for the latter in all icon sets, but that's not the case. Should we add our own icon?

tsujan commented 3 years ago

A simple solution — as far as designing is concerned — is forgetting about pause, adding a permanent setting for the new mode to the config dialog and showing a startup notification about it, as KDE does.

stefonarch commented 3 years ago

I didn't quite understand the latter.

A possibility could be "Don't show notification popups" setting, and then always show the icon, with a counter. So this would not disturb.

tsujan commented 3 years ago

... and then always show the icon, with a counter.

The existence of the icon informs the user about the existence of unattended notifications by a glance. I think we shouldn't sacrifice this important functionality.

stefonarch commented 3 years ago

I've in mind to have all somehow:

I don't use the unattended notifications, as I hardly click away any notification - I just read it on the fly and let it expire, and doing so it will end up as "not seen".

tsujan commented 3 years ago

Always shows icon with unattended notifications,

This is the same as "Behavior as it is now:.."; isn't it?

Toggle popups (do not disturb). This is hard to activate when no icon is present, needs to be done in settings.

We can add an option for the "do-not-disturb" mode to settings dialog. But what if the user forgets about it? I mean how do we remind he/she that he'll see no notification because he's activated the do-not-disturb mode?

Some people don't like it when I say the do-not-disturb mode is at odds with the concept of notification. OK, we might add it but we should also make sure that the user won't forget about it. The question is "How?".

Showing number of last unattended notifications.

Why? What's the use of showing the number? If there are unattended notifications, there will be a tray icon and the user could click it and see the list. If there's no unattended notification, there will be no tray icon.

tsujan commented 3 years ago

but we should also make sure that the user won't forget about it... The question is "How?"

Is the following answer to my above question acceptable to you?

When "do-not-disturb" mode is activated, treat all notifications as unattended (because they aren't shown) and add an appropriate emblem to the tray icon, accompanied by an appropriate tooltip.

stefonarch commented 3 years ago

Always shows icon with unattended notifications,

This is the same as "Behavior as it is now:.."; isn't it?

No, as it disappears when they are cleared. This would be useful to toggle "do not disturb".

Is the following answer to my above question acceptable to you?

When "do-not-disturb" mode is activated, treat all notifications as unattended (because they aren't shown) and add an appropriate emblem to the tray icon, accompanied by an appropriate tooltip.

The downside is that is has to be activated in settings, right? Could be undone automatically with "clear all & stop "do not disturb mode".

tsujan commented 3 years ago

No, as it disappears when they are cleared.

The tray icon already disappears when the list is cleared.

EDIT: Probably, I still don't understand what you mean but, as I said above, IMO, it's important to show the tray icon only when there's an unattended notification. Clearing the list means that the user has paid attention to the notifications.

The downside is that is has to be activated in settings

IMHO, it isn't a downside because the user can't ignore that he has activated the mode as soon as the first notification is triggered (silently) — because of the emblem. Moreover, the tray menu already has an item for opening the config dialog.

There will be several important details that should be considered in the code. The implementation won't be easy but is doable in a clean way.

tsujan commented 3 years ago

The emblem can be like this, with a semi-transparent background for it to bee seen with all icons:

emblem

tsujan commented 3 years ago

EDIT: I don't know if I've made a mistake or the implementation was much easier than I thought because it took only 15 minutes; should test it for a day or two.

Used a pause emblem (like that of power management tray icon), instead of the above alarming icon:

icon

And this is the new option:

settings

stefonarch commented 3 years ago

Looks fine! In the icon menu an item "exit do not disturb" "show notifications" or similar?

tsujan commented 3 years ago

In the icon menu an item "exit do not disturb" "show notifications" or similar?

I didn't touch the menu. Since the "Options" menu-item is there, the user won't have any problem. The whole patch is as small as possible: it sees all notifications unattended with the do-not-disturb mode and saves them, instead of showing them. I let the app save up to 50 notifications in this mode, which is more than what's needed in practice. The settings of unattended notifications are ignored with it.

Will make a PR today.

stefonarch commented 3 years ago

As mentioned here https://github.com/lxqt/lxqt/discussions/2054#discussioncomment-1135839 to be sure I switched back to the master branch, and telegram icon is back. I've no idea why, but this was the only difference to before.

stefonarch commented 3 years ago

Note: I didn't saw this happen with the first commit.

tsujan commented 3 years ago

Note: I didn't saw this happen with the first commit.

I assure you that Telegram's icon doesn't have the slightest relation to this patch. But it makes me worry about Status Notifier (see the second paragraph of https://github.com/lxqt/lxqt-notificationd/pull/271#issuecomment-893550824).

tsujan commented 3 years ago

@stefonarch If you ran Telegram after installing it, the generic icon is quite possible. See https://github.com/lxqt/libqtxdg/issues/226. In that case, restarting the panel will restore the correct icon once for all.

stefonarch commented 3 years ago

Sure I restarted telegram, panel, session and even laptop - always generic icon with telegram. I can try repeat it, atm recompiling panel, but I saw only new translations, everything should be quite git master.

tsujan commented 3 years ago

If it persists, please open an issue at panel's bug tracker. Here isn't a good place.

stefonarch commented 3 years ago

Sure I restarted everything, from panel to session to laptop. Recompiled panel now (although I saw only new translations) and reinstalled the "do not disturb" *.deb package, and it didn't happen again. So for the moment all good but there is something sleeping with those generic icons.

by the way: maybe the volume change notifications should be always shown, as immediate feedback on actions.

tsujan commented 3 years ago

but there is something sleeping with those generic icons.

It may be just https://github.com/lxqt/libqtxdg/issues/226 but, since you said you restarted panel, there may be more.

by the way: maybe the volume change notifications should be always shown

"Don't disturb" is what it means ;) Frankly, I won't use it because, IMO, it's at odds with notification but if a user is disturbed by notifications, there's an option now. IMO, having another exception list for the do-not-disturb mode would be weird, if not an overkill.

stefonarch commented 3 years ago

I didn't think of another option, but volume change isn't actually a notification coming from elsewhere but an action feedback, triggered by the user. I see now that libqtxdg had an update 750380b..dcac08a so not all was up-to date with git master.

tsujan commented 3 years ago

but volume change isn't actually a notification coming from elsewhere

There's no distinction in the code. Volume change comes from Panel, while lxqt-notificationd doesn't depend on lxqt-panel. Moreover, some users might complain rightly ("It was supposed not to disturb...").

stefonarch commented 3 years ago

Today after clearing the notifications at the next notificationsI got again the generic icon, clearing them and doing an notify-send test -t 10 icon was ok. System was only suspended and all relevant components should be up-to-date, will check later.

tsujan commented 3 years ago

Do you have https://github.com/lxqt/lxqt-notificationd/pull/271/commits/d31eb110414d8a8770837d29b00624ed41166a93 too?

stefonarch commented 3 years ago

Yes.

tsujan commented 3 years ago

Yes.

Since you've seen the same thing with Telegram a few times and because Telegram also adds emblems to its tray icon, I suspect Panel's Status Notifier plugin.

There's nothing wrong about the icon in this code or in Telegrams's code (I checked Telegrams's source) — the "workaround" I added here seems redundant (but it's harmless).

stefonarch commented 3 years ago

It happened again, after suspending for a long time (~4h), same behavior. A short suspend doesn't trigger it. With telegram or others no issue atm, could try those other apps where this issue was seen with,

tsujan commented 3 years ago

I checked the code of Status Notifier and found no issue in it either. We might need reports by different users to compare them.

If you want to test with other apps, you'll need to find those, whose tray icons have emblems. Telegram is such an app (with unread messages); I'm sure about that.

stefonarch commented 3 years ago

I think also meteo-qt is one of those. schermata-08-09-22-34

Will report, until now it didn't happen again.

tsujan commented 3 years ago

I think also meteo-qt is one of those.

I'm not sure — not comfortable with python+Qt codes — but it may be.

Could you please be more specific about what you see? Are you saying that the icon+emblem is there, then you suspend the computer and, after resuming, the icon is changed to the generic icon?

stefonarch commented 3 years ago

Are you saying that the icon+emblem is there, then you suspend the computer and, after resuming, the icon is changed to the generic icon?

Exactly, this was twice the case now.

tsujan commented 3 years ago

Exactly, this was twice the case now.

Oh, that's different from what I thought and a lot stranger! Thanks! I didn't think icons could change after resuming from suspend — and I've never seen it. Have no idea how it's possible.

EDIT1: I usually reboot/shut down my laptop once in 1-2 weeks. So, it has long periods of suspension.

EDIT2: I'll test it on my old laptop, which I may suspend for days.

tsujan commented 3 years ago

I think I've found a weird way of reproducing it on my old machine. It isn't related to resuming from suspend. Keeping my fingers crossed, I hope it'll be still reproducible when I find the time to investigate it.

EDIT: I reproduced it 4 times by doing a ritual :)

stefonarch commented 3 years ago

Well, nice! I rebooted yesterday and didn't see it again for now.

tsujan commented 3 years ago

I found a little time to investigate it. Fortunately, it's 100% reproducible with a sequence of actions.

Most probably, the cause of this weird problem is deep inside Qt. It only happens with hiding and showing the tray icon. I'd encountered a similar problem years ago (about not showing the tray menu). The workaround is elementary but I prefer to get nearer to its root before adding a commit to the PR. That may need more time.

tsujan commented 3 years ago

OK, done!

A short explanation:

QSystemTrayIcon definitely has a bug (under X11, at least), which shows up when the tray icon isn't a theme icon (like the emblemized icon I used): as soon as the tray icon is hidden, the non-theme icon is lost. The workaround I had in mind was re-adding it before the tray icon was shown, and it worked. However, because of another nasty problem in QSystemTrayIcon, there was a small memory leak. Also, removing and recreating the tray icon caused the same memory leak.

Since I hate memory leaks even when they're small, I removed the emblem and used the icon "notifications-disabled" for the do-not-disturb mode. If it doesn't exist in a theme, the usual icon ("preferences-desktop-notification") will be used. In this way, the code is simplified too.

NOTE: The above-mentioned bug doesn't happen with the power manager's emblem ("Pause idleness checks") because its tray icon is either shown or removed — we never hide it.

stefonarch commented 3 years ago

Testing, looks like that now, I like the do-not-disturb icon more than the previous, but shouldn't be both the same (the bell)? Could be confusing a little.

schermata-08-11-08-11 schermata-08-11-08-14

tsujan commented 3 years ago

but shouldn't be both the same (the bell)?

You mean using "notifications" for the usual mode? I had the same idea but saw that "notifications" didn't exist in GTK icon themes.

Maybe we can use "notifications" and go to "preferences-desktop-notification" only if it doesn't exist.

tsujan commented 3 years ago

Added a commit to the PR for the last idea. You're right; the usual and no-disturb modes are more consistent with those icons.

If someone uses a GTK icon set, he'll only see "preferences-desktop-notification" but we know that GTK icon sets aren't good for Qt DEs in general.

jmfernandez commented 3 years ago

Added a commit to the PR for the last idea. You're right; the usual and no-disturb modes are more consistent with those icons.

If someone uses a GTK icon set, he'll only see "preferences-desktop-notification" but we know that GTK icon sets aren't good for Qt DEs in general.

Maybe my current question is very off-topic. Something very useful in gtk-3.0 is that it is possible to set up a fallback icon theme where the toolkit looks for icons, in case the main icon theme does not have some icon for any reason. Does LXQt (or Qt itself) have something similar?

Your work providing an alternate icon name in case first icon name is not found is another way, assuring a decent fallback.

tsujan commented 3 years ago

Does LXQt (or Qt itself) have something similar?

Yes, of course. LXQt also follows freedesktop standards and looks for "dash fallbacks". If no icon is found, the generic icon will be used.

In addition to that, each app can decide which icon to use when a specific icon isn't found.