Skip to content
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

fix(Notify): Use notification categories on Linux #427

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Pigpog
Copy link

@Pigpog Pigpog commented Jan 12, 2025

With this change, events will notify using distinct categories.

Incoming calls use call.incoming
Friend and conference messages use im.received
Friend requests and group invites use the generic im
File transfer requests use the generic transfer

Since call notifications used the same exact code paths as friend message notifications, I had to change some logic in the newFriendMessageAlert function. It uses the notification body (text) being empty to determine that it is an incoming call.

Solves #424


This change is Reviewable

@Pigpog
Copy link
Author

Pigpog commented Jan 12, 2025

After more testing, I think more work is needed for notifications in general. If multiple notifications are generated, qTox simply changes the existing notification to say "3 message(s) from user". I imagine this is an artifact of snore-notify.

Additionally, incoming call notifications do not clear if the call has been cancelled.
And lastly, even after disabling "Play sound" in qTox notification settings, it still plays the qTox ringtone.

I think it would be best to delay merging this until these other issues can be addressed.

@Pigpog
Copy link
Author

Pigpog commented Jan 12, 2025

Ok so I went a bit out of scope here (meant to start a new branch, oops), but so far I've managed to fix most of the issues I listed before.
A new notification is sent for every event now. The notification titles and bodies have been reformatted accordingly. Disabling "Play sound" in settings truly stops all notification sounds from playing.
Still need to make it retract notifications in certain situations (call ring cancelled, chat window focused, maybe other cases).

@iphydf
Copy link
Member

iphydf commented Jan 12, 2025

The main reason I used a single notification and made it update is the situation where you receive a lot of messages all at once. What happens with your change when someone spams a chat? In any case I think what we had was quite broken, so I'm happy to take a step back and simplify for now.

@iphydf iphydf added this to the v1.18.3 milestone Jan 12, 2025
@iphydf iphydf changed the title Use notification categories on Linux fix(Notify): Use notification categories on Linux Jan 12, 2025
@Pigpog
Copy link
Author

Pigpog commented Jan 12, 2025

It would be kind of cool to group the messages from one contact into single notifications (without mixing categories). The notification generator would need to track the notification IDs in use for each contact. Perhaps we could use the contact number to make it super simple. I might have a poke at that.

I think we have a few options here:

  1. Go back to grouping/summarizing all notifications
  2. One notification per contact, appending new messages
  3. One notification per event (my changes currently)

I dislike the first one because I am unable to read the messages in the notifications. One notification shared for everything is too conservative in my opinion.
The second one is a more modern behaviour and is nice for reducing clutter.
The third one is fine by me, telegram desktop acts this way and it causes no issues. Spam attacks can be vicious on Tox though, and this one adds to the impact. Perhaps we could rate limit notifications?

@nurupo
Copy link
Member

nurupo commented Jan 12, 2025

What about making this a settings option - updating the existing notification on an event vs adding a new notification per event? Might not be possible to do on every platform, but sounds like it might be useful for the ones that support it.

@iphydf
Copy link
Member

iphydf commented Jan 12, 2025

We're really focussing on Linux here. Other platforms use the Qt built-in thing, which doesn't allow notification fusing, categories, etc. at all. I do believe our best UX is going to be on Linux (and I'm very OK with that). For that, I think option 2 is best, but we can go ahead with 3 for the moment, and make 2 as an improvement on top of that (unless you want to do it now, I'm happy with that as well).

@Pigpog
Copy link
Author

Pigpog commented Jan 12, 2025

It might be best to keep this PR somewhat on track here and stick with individual notifications for now as you say.
I'm in for a git headache already with the mess I made

@Pigpog Pigpog closed this Jan 12, 2025
@Pigpog Pigpog force-pushed the notification-categories branch from d18ac6b to 127e0cb Compare January 12, 2025 19:26
@github-actions github-actions bot added the bug Bug fix for the user, not a fix to a build script label Jan 12, 2025
@Pigpog
Copy link
Author

Pigpog commented Jan 12, 2025

I'm just redoing all my commits at the moment, not very git-savvy

@Pigpog
Copy link
Author

Pigpog commented Jan 12, 2025

Reopening since I didn't mean to close

@Pigpog Pigpog reopened this Jan 12, 2025
@Chiitoo
Copy link

Chiitoo commented Jan 12, 2025

I'm just redoing all my commits at the moment, not very git-savvy

Depending on the mess, probably most of the time you can use git rebase --interactive HEAD~11 (where 11 is a random number of commits I used as an example) to clean things up. :]

@Pigpog Pigpog force-pushed the notification-categories branch from e7b753a to f2f80f1 Compare January 12, 2025 20:31
@Pigpog
Copy link
Author

Pigpog commented Jan 12, 2025

Ok that should be all of my changes back in place.
To recap, new notifications are sent for every event, the "Notify sound" setting now disables the ringtone and any other notification sound, and each type of notification is categorized per FreeDesktop's spec.

@Pigpog
Copy link
Author

Pigpog commented Jan 12, 2025

Looks like other platforms use notificationgenerator.. Should I go ahead and modify them to do single notifications as well?

EDIT: Looking at it again, the windows build test is failing due to an unused variable, but it is used in the very next line.

@iphydf
Copy link
Member

iphydf commented Jan 12, 2025

Looks like other platforms use notificationgenerator.. Should I go ahead and modify them to do single notifications as well?

EDIT: Looking at it again, the windows build test is failing due to an unused variable, but it is used in the very next line.

That's because it's only used in assert, which gets removed in release builds. You can put a std::ignore = ... for unused parameters.

@iphydf iphydf force-pushed the notification-categories branch 2 times, most recently from 89fe35d to 53304ed Compare January 14, 2025 12:58
@iphydf
Copy link
Member

iphydf commented Jan 14, 2025

/qtox/src/platform/desktop_notifications/desktopnotifybackend_noop.cpp:18:28: error: out-of-line definition of 'showMessage' does not match any declaration in 'DesktopNotifyBackend'
bool DesktopNotifyBackend::showMessage(const QString& title, const QString& message,
                           ^~~~~~~~~~~

@iphydf iphydf force-pushed the notification-categories branch from 53304ed to 8d2eb69 Compare January 15, 2025 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug fix for the user, not a fix to a build script
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants