Skip to content

Conversation

@JeroenvdV
Copy link

This makes adding notifications much faster by avoiding parsing JSON unnecessarily. Further improvements to the encoding of the SharedPreferences will also improve the speed of cancelling specific notifications.

Set<String> jsonNotificationsToSave = new HashSet<String>(jsonNotifications);

Gson gson = buildGson();
jsonNotificationsToSave.add(gson.toJson(notificationDetails));

Choose a reason for hiding this comment

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

notifications should have unique ids not unique json strings

Copy link
Author

Choose a reason for hiding this comment

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

I ignored this explicitly, although we can solve this by using a HashMap instead of ArrayList in loadScheduledNotifications.

*/
private static Set<String> getScheduledNotificationsJsonSet(SharedPreferences sharedPreferences) {
Set<String> jsonNotifications = sharedPreferences.getStringSet(SCHEDULED_NOTIFICATIONS_SET, null);
if (jsonNotifications != null) {

Choose a reason for hiding this comment

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

This check implies that we cannot switch back to the old implementation and again to this one, the migration will always happen only once

Copy link
Author

Choose a reason for hiding this comment

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

This is true, in an attempt to reduce the number of extra queries to Shared Preferences made during normal use.

When switching back to the old implementation, the notifications will be lost. When re-switching to the new implementation, the lost notifications will be restored and any new ones will be lost.

Preventing the initial loss is only possible by additionally saving the alarms the old way, which would defeat the purpose of the change.
Doing a second migration is only possible by always checking for presence of an old data object, which would incur a performance penalty each time.

My suggestion to solve the second migration problem is to make it the responsibility of the app and not the plugin. App developers can ensure notifications are explicitly re-scheduled by the app if such a rare situation occurs.

@JeroenvdV JeroenvdV force-pushed the 10.0.0-medapp branch 2 times, most recently from f546ad0 to 251114b Compare February 14, 2022 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants