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

[Push] Show notification on push notification (until sync is started) #1043

Conversation

ArnyminerZ
Copy link
Member

@ArnyminerZ ArnyminerZ commented Sep 25, 2024

Purpose

It can happen that a PUSH notification is received, but the app actually doesn't have access to the internet, so the synchronization is not started immediately. A notification will be shown to tell the user that the sync is pending. It should be hidden when the synchronization starts.

Short description

Added a new parameter to the enqueue functions, to tell them whether the schedule is from a push notification or not. If it is, a notification is shown using PushNotificationManager, which is shown until the sync is started.

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

Depends on: #1056

Signed-off-by: Arnau Mora Gras <[email protected]>
@ArnyminerZ ArnyminerZ added the enhancement New feature or request label Sep 25, 2024
@ArnyminerZ ArnyminerZ self-assigned this Sep 25, 2024
@ArnyminerZ ArnyminerZ changed the title [WIP] [Push] Show notification on push notification (until sync is started) [Push] Show notification on push notification (until sync is started) Oct 2, 2024
@ArnyminerZ ArnyminerZ requested a review from rfc2822 October 2, 2024 05:42
@ArnyminerZ ArnyminerZ marked this pull request as ready for review October 2, 2024 05:42
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Nice, will be cool to finally get the notifications :)

A few comments though…

Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
Signed-off-by: Arnau Mora Gras <[email protected]>
Copy link
Member Author

@ArnyminerZ ArnyminerZ left a comment

Choose a reason for hiding this comment

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

@rfc2822 I'm not sure how to get an instance of PushNotificationManager from here, since it's in the companion, should we pass it as an argument? Doesn't sound really good

Edit: here*

// Show notification if called by push
// FIXME: Inject PushNotificationManager
if (isPush) {
PushNotificationManager.notify(context, account, authority)
}

@rfc2822
Copy link
Member

rfc2822 commented Oct 5, 2024

We could have used a custom entry point for the companion object. However I think there shouldn't be too much logic in companion object anyway, so I have prepared a new class in #1056. That one is useable with DI.

…fication-until-sync-is-started

# Conflicts:
#	app/src/main/kotlin/at/bitfire/davdroid/push/UnifiedPushReceiver.kt
#	app/src/main/kotlin/at/bitfire/davdroid/sync/worker/OneTimeSyncWorker.kt
Signed-off-by: Arnau Mora Gras <[email protected]>
@ArnyminerZ ArnyminerZ requested a review from rfc2822 October 8, 2024 06:54
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Looks very promising – only minor comments.

@ArnyminerZ ArnyminerZ requested a review from rfc2822 October 10, 2024 08:03
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Great, looking forward to (not) see the notifications 👍🏻

@rfc2822 rfc2822 merged commit c805e54 into main-ose Oct 10, 2024
8 checks passed
@rfc2822 rfc2822 deleted the 1014-push-show-notification-on-push-notification-until-sync-is-started branch October 10, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Push] Show notification on push notification (until sync is started)
3 participants