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

Add priority for notifications #7530

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

mojganii
Copy link
Collaborator

@mojganii mojganii commented Jan 27, 2025

This PR adds priority to notifications. In addition, notifications with same priorities are shown based on the creation date.


This change is Reviewable

Copy link

linear bot commented Jan 27, 2025

@mojganii mojganii self-assigned this Jan 27, 2025
@mojganii mojganii added the iOS Issues related to iOS label Jan 27, 2025
rablador
rablador previously approved these changes Jan 27, 2025
Copy link
Contributor

@rablador rablador left a 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?

Copy link
Contributor

@buggmagnet buggmagnet left a 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.

Copy link
Collaborator Author

@mojganii mojganii left a 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.

@mojganii mojganii force-pushed the add-priority-for-notifications-ios-1036 branch from a0747c6 to c5a4d45 Compare January 28, 2025 11:37
Copy link
Collaborator Author

@mojganii mojganii left a 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, 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.

Done

Copy link
Contributor

@rablador rablador left a 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)

Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 6 of 9 files reviewed, all discussions resolved (waiting on @rablador)

@buggmagnet buggmagnet force-pushed the add-priority-for-notifications-ios-1036 branch from c5a4d45 to d1bc47e Compare January 29, 2025 09:24
@buggmagnet buggmagnet merged commit fafec04 into main Jan 29, 2025
10 of 11 checks passed
@buggmagnet buggmagnet deleted the add-priority-for-notifications-ios-1036 branch January 29, 2025 09:26
Copy link

🚨 End to end tests failed. Please check the failed workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants