-
Notifications
You must be signed in to change notification settings - Fork 27
Fix/backlog grooming june 2025 #1287
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
Conversation
…ost db transaction
…ing to avoid duplicates
val existingContent = existingNotifications.find { it.id == notificationId }?.notification?.extras?.getString(CONTENT_HASH) | ||
val newContentHash = "${notifications[0].text}_${notifications[0].timestamp}".hashCode().toString() |
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.
So if we can trunk an arbitrary text as CONTENT_HASH
, maybe we don't actually need to use the collision possible hashCode
, but just use "${notifications[0].text}_${notifications[0].timestamp}"
as CONTENT_HASH
?
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.
Switched to a string
val notifications = notificationState.notifications | ||
val existingNotifications = ServiceUtil.getNotificationManager(context).activeNotifications | ||
val existingContent = existingNotifications.find { it.id == SUMMARY_NOTIFICATION_ID }?.notification?.extras?.getString("content_hash") |
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.
Should this use the constant instead?
val existingContent = existingNotifications.find { it.id == SUMMARY_NOTIFICATION_ID }?.notification?.extras?.getString("content_hash") | |
val existingContent = existingNotifications.find { it.id == SUMMARY_NOTIFICATION_ID }?.notification?.extras?.getString(CONTENT_HASH) |
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.
Fixed
val existingContent = existingNotifications.find { it.id == SUMMARY_NOTIFICATION_ID }?.notification?.extras?.getString("content_hash") | ||
|
||
// Create a hash based on the notification content (all messages + their timestamps) | ||
val contentForHash = notifications.map { "${it.text}_${it.timestamp}" }.joinToString("|") |
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.
Might be good to extract the way to construct the hash for reuse
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.
True, I moved it to a function
SES-3950 - Fixes to received and sent timestamps in message info
SES-2359 - Reworked video thumbnail creation to avoid race condition during db transaction
SES-4024 - Respecting the system's 12 vs 24h format
SES-2582 - Reworking notifications to allow emoji reaction notifications on outgoing messages.
Also cleaned up and reworked the overall notification logic to also avoid duplicates.