-
Notifications
You must be signed in to change notification settings - Fork 369
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
Add priority for notifications #7530
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mojganii)
ios/MullvadVPN/Notifications/NotificationManager.swift
line 36 at r1 (raw file):
if $0.priority == $1.priority { // Older notifications are shown first with same priority return $0.timestamp < $1.timestamp
Nit: The idea is good, but should we show older notifs before new ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rablador)
ios/MullvadVPN/Notifications/NotificationManager.swift
line 36 at r1 (raw file):
Previously, rablador (Jon Petersson) wrote…
Nit: The idea is good, but should we show older notifs before new ones?
Agreed, this showed me the "new device created" notification before the "New version installed" one.
We should show newer ones before older ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet and @rablador)
ios/MullvadVPN/Notifications/NotificationManager.swift
line 36 at r1 (raw file):
Previously, buggmagnet wrote…
Agreed, this showed me the "new device created" notification before the "New version installed" one.
We should show newer ones before older ones.
The priority of NewDeviceNotificationProvider
is medium, while LatestChangesNotificationProvider
is low. Therefore, NewDeviceNotificationProvider
is shown first. A timestamp is used for notifications with the same priority to determine which one is shown first. However, if you both agree that the timestamp
is over-engineering, we can remove it.
a0747c6
to
c5a4d45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 6 of 9 files reviewed, all discussions resolved (waiting on @buggmagnet and @rablador)
ios/MullvadVPN/Notifications/NotificationManager.swift
line 36 at r1 (raw file):
Previously, mojganii wrote…
The priority of
NewDeviceNotificationProvider
is medium, whileLatestChangesNotificationProvider
is low. Therefore,NewDeviceNotificationProvider
is shown first. A timestamp is used for notifications with the same priority to determine which one is shown first. However, if you both agree that thetimestamp
is over-engineering, we can remove it.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 6 of 9 files reviewed, all discussions resolved (waiting on @buggmagnet)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 6 of 9 files reviewed, all discussions resolved (waiting on @rablador)
c5a4d45
to
d1bc47e
Compare
🚨 End to end tests failed. Please check the failed workflow run. |
This PR adds priority to notifications. In addition, notifications with same priorities are shown based on the creation date.
This change is