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.

Sign up for GitHub

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

Jump to bottom

add Notification Service Extension #2105

Merged
merged 31 commits into from
Mar 22, 2024
Merged

add Notification Service Extension #2105

merged 31 commits into from
Mar 22, 2024

Conversation

r10s
Copy link
Member

@r10s r10s commented Mar 6, 2024 •
edited
Loading

this PR adds a "Notification Service Extension" that is called for every push notification received, allowing to modify the notification content, which is (localized) "You have new messages" otherwise.

as we do not know anything about the message the push notification belongs to (nor does apple or others then), we do a background-fetch over all accounts and change the notification accordingly, concretely setting title to chatname and title to message text beginning.

this also filters out reactions or other messages that do not come with DC_EVENT_INCOMING_MSG

if we get more than one message via IMAP (because of races or additional accounts), we use texts as "N messages" or "N messages in M chats".

because of that and because of muting, it may happen, that we do not want to display a notification at all. by default this is not possible (we can only downtune via sound, interruptionLevel, relevanceScore etc.), but there is a special entitlement that makes not displaying possible

nb: the "Notification Service Extension" is an own process (similar to DcShare), that is started independently of the main deltachat-ios app (which may or may not run at the same time). group-id makes it possible that the same data are accessible, however.

bigger todo parts:

  • make sure, NSE-fetch and main-app-io do not run at the same time (otherwise last-seen etc. gets issues, also double messages and double notifications) - EDIT: done with the flags mainAppRunning/nseFetching (cherry picked from #2107/#2110) - we did not find a real-time IPC check better than saving flags in shared memory
    • add some resilience when app is terminated unexpectedly and we cannot reset the flag -> this can go to a subsequent PR, current state is quite stable already - and as long we do not have the do-not-show-entitlement, sth. will be visible in any case -- EDIT: and, even with entitlement, things would not be stuck forever as also background fetch resets the flag, so it is not that bad at all
  • set real message content
  • handle mute
  • set badge counter accordingly
  • if a notification spans more than one chat, add all of them to the title (iOS will truncate and the summary be sth as "N messages in M chats")
  • switch accounts as needed when tapping notifications
  • for notifications referring exactly to one chat, open the chat instead of the chatlist (requires using eg. userInfo instead of notificationIdentifier)
  • remove PUSH notifications when the chat is opened; for unspecific notifications ("N messages"), remove them when the chatlist is opened
  • make localisation work, probably just some string files/references missing in the extension EDIT: needed to be re-added at 'Build Phases / Copy Bundle Resources' after rebase
  • force update of chatlist similar to DcShare (needed in case the app is suspended to ram)
  • stop showing images in notifications: the current implementation is slow and would need some more love - but the effort is questionable as showing images unexpectedly is also easily compromising. better just show a text "Image" as on android (in general "📷" instead of "Image" would be cool, btw - EDIT: this is added meanwhile)
  • sync threading and other data with normal notifications
  • "New Messages - No more relevant" on next background fetches and app starts

finally:

  • see what happens with entitlement - however, this is no blocker. even without entitlement, this PR is an improvement as unwanted messages do no longer sound nor turn on display. and, of course, wanted notifications get the correct text

before (showing generic "You have new messages" in german) / after (showing real message content):

successor of #2080

@r10s r10s mentioned this pull request Mar 6, 2024
Merged
@Simon-Laux
Copy link
Member

Simon-Laux commented Mar 6, 2024 •
edited
Loading

future question: can we also send/trigger local notifications from this function/extension?

@r10s r10s force-pushed the r10s/notification-service-extension branch 3 times, most recently from 6234b38 to 3687246 Compare March 7, 2024 13:55
@r10s r10s added the notify label Mar 7, 2024
@r10s r10s force-pushed the r10s/notification-service-extension branch 6 times, most recently from 570d07b to 1c1ebb5 Compare March 13, 2024 17:51
@r10s r10s added the enhancement actually in development, user visible enhancement label Mar 20, 2024
r10s added 17 commits March 20, 2024 17:02
@r10s
add DcNotificationService
9a4d8c3
@r10s
fix issue number one: different deployment targets stand in the way o…
2a1b747 
…f 'Notification Service Extension' to work
@r10s
fix compiler complaining about bad versions
fd7ec26
@r10s
assign DcNotificationService to group.chat.delta.ios so it can access…
98d0c52 
… the same files and assets as the main app
@r10s
remove dead code
314f49f
@r10s
make notifications discardable, use more reasonable fallback
c04fa76
@r10s
set body to 'N messages' as needed
33636dd
@r10s
skip String.localized() for now as not working in extension (but is w…
e546b3d 
…orking in DcShare, sth. is missing)
@r10s
link DcCore, to fix 'umbrella header missing' - not sure why that wor…
2717d9b 
…ked before without
@r10s
tune down unrelevant messages until we can discard them
8c08f43
@r10s
unify summary for groups
9c3c645
@r10s
update badge counter
8cb1f63
@r10s
close databases when fetch is done
c774325
@r10s
if a notification spans more than one chat, add all of them to the ti…
df48825 
…tle (iOS will truncate and the summary be sth as 'N messages in M chats')
@r10s
force UI updates in case app was suspended
8426bee
@r10s
a trigger seems not to be needed, therefore also no need to remove pe…
84a988c 
…nding notifications
@r10s
because of accounts not using PUSH, we do not have a notification for…
c004172 
… each message; group everything in the same thread to avoid confusion when 'Alice Chat' is also present in thread 'Alice Chat, Bob Chat'
r10s and others added 10 commits March 20, 2024 17:02
@r10s
threadIdentifier not needed if the same everywhere; this also groups …
020e98f 
…in notifications not hitting the extension or handled by serviceExtensionTimeWillExpire
@r10s
move 'future' code to a comment
bc171eb
@r10s
set all of account_id/chat_id/message_id for all notifications
9412997
@r10s
switch to correct account on tapping a notification; do not just dism…
c858303 
…iss all notifications on account switch, account switch is lightweight meanwhile
@r10s
respect account_id when removing notifications for a chat
99e75ea
@r10s
handle unspecific notifications
91252e3
@r10s
do not add images to notifications; this is easily compromising, just…
45526b8 
… saying 'Image' as on android is good enough. also, images may slow down things siginificantly as copied uncompressed in the existing implementation (nb: would be cool to show an emoji as '📷' instead of the string 'Image'
@r10s
create explicit silenceNotification() method
c271aef
@zeitschlag @r10s
Localize notification-strings
8ee77e2
@zeitschlag @r10s
Enable localization for NSE
c4a1316
@r10s r10s force-pushed the r10s/notification-service-extension branch from 1c1ebb5 to d928de7 Compare March 20, 2024 16:02
@r10s r10s force-pushed the r10s/notification-service-extension branch from 18db119 to 1449af8 Compare March 20, 2024 22:31
@r10s
remove irrelevant notification on app-start or background-fetch
bea2abb 
use a simple 'Tap to open' as the notification content -
usually, there was some message before/after or there are muted ones,
so that openes less questions in most cases
and makes at least a more sense than adding a Notification saying "No relevant things" :)
@r10s r10s force-pushed the r10s/notification-service-extension branch from 5a0da83 to bea2abb Compare March 20, 2024 23:07
@r10s r10s requested a review from zeitschlag March 20, 2024 23:33
@r10s r10s marked this pull request as ready for review March 20, 2024 23:33
@Simon-Laux
Copy link
Member

future question: can we also send/trigger local notifications from this function/extension?

if so we could send a notification for each message that was received, but maybe that would spam too much. so the current way of telling about "you have multiple new messages" is completely fine, maybe even better 👀

@r10s
Copy link
Member Author

r10s commented Mar 21, 2024

i did a small test at some point, one can get a pointer to UNUserNotificationCenter from NSE, however, basic functionality as removing notifications did not work for me (otherwise one would not need an entitlement :)

therefore, i did not investigate further, even if some hacks may work on some OS, this seems not to be the gist of the NSE, and there may be dragons now or in the future :)

zeitschlag
zeitschlag approved these changes Mar 22, 2024
View reviewed changes
Copy link
Collaborator

@zeitschlag zeitschlag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I added some questions here and there, but consider them feedback and feel free to just mark them as completed :)

@r10s r10s merged commit 59b9de4 into main Mar 22, 2024
@r10s r10s deleted the r10s/notification-service-extension branch March 22, 2024 10:45
@r10s r10s mentioned this pull request Mar 23, 2024
Merged
@r10s r10s mentioned this pull request Apr 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zeitschlag zeitschlag zeitschlag approved these changes

Assignees
No one assigned
Labels
enhancement actually in development, user visible enhancement notify
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@r10s @Simon-Laux @zeitschlag