draft: macOS notification integration#10381
Conversation
Signed-off-by: Rello <github@scherello.de>
|
We already have the capacity to emit interactive macOS notifications via |
is ist not not using the existing one? the issue with the current one was, that it did not have a unique id, so be able to perform an action across many accounts. The shared notification logic for all platforms was moved into [src/gui/notifications] to make the code modular like the other recent refactors.
Only the native presentation layer is platform-specific:
if i misses something, let me know |
|
This fails to build for me.
Besides, I am skeptical about the priority of this and also do not feel good about the platform-specific feature duplication for a little more fancy user interface. 😕(more than the new menu bar extra) While the horizontal action stack does not even look that good. |
i2h3
left a comment
There was a problem hiding this comment.
Review: macOS native notifications
Thanks for this — the direction is solid and pulling the notification logic out of User/UserModel into an account-scoped NotificationManager is a real improvement. Notes below; nothing here is a safety bug, and the two most important items are UX/product questions rather than change requests.
What's good
- Clean separation:
NotificationManager(fetch → dedup → present → dispatch) vs the thinMacNotificationCenterfaçade;User/UserModelshrink nicely. - Stable, content-addressed notification identifiers (
NextcloudServerNotification-<sha256(accountId:id)>) make properremove/reconcileof delivered banners possible — something master couldn't do. - Per-notification
responseActionsinuserInfokeyed to a shared, signature-derived category is a correct design (links are per-notification; categories dedupe by action shape).
I looked specifically for threading / lifetime / memory-safety issues and found none: callbacks marshal back to the main thread via performOnMainThread/performWithNotificationManager, the account registry uses QPointer, reconciliation is generation-guarded, and the manual retain/release is correct for this MRR (no-ARC) target.
Two questions to settle before merge (inline on macnotificationcenter.mm:590 and notificationmanager.cpp:285):
- Clearing/swiping a banner now issues a server
DELETE(removes it on all devices) — intended? - First fetch fires one banner + sound per pending notification instead of master's aggregated toast — aggregate above a threshold?
Cleanup (inline): dead macOS TALK_MESSAGE path, the platform-split gating in buildNotificationDisplay, the never-pruned category registry, and a sendTalkReply DRY nit.
Pre-existing (carried over from master, not introduced here — just FYI): canSendNotification() is still a TODO stub returning center != nil; requestAuthorization() is called on every send; and the ETag short-circuit in servernotificationhandler.cpp (if (!_preFetchEtagHeader.isEmpty() || …)) is effectively inert because _preFetchEtagHeader is always empty on a per-fetch handler. Probably a separate cleanup ticket.
|
FYI: I let Claude Opus 4.8 Ultracode review it (as explicitly requested by @Rello), I did not write the comments above myself. |
Signed-off-by: Rello <github@scherello.de>
|
I propose grouping of the notifications. @i2h3 ok or not a good idea for any reason?
|
Signed-off-by: Rello <github@scherello.de>
|
@Rello sounds reasonable. |

Uh oh!
There was an error while loading. Please reload this page.