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

Implements MSC3773 (Thread Notifications) #7424

Merged
merged 16 commits into from
Oct 26, 2022
Merged

Conversation

ericdecanini
Copy link
Contributor

@ericdecanini ericdecanini commented Oct 20, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Implements MSC3773

Motivation and context

Closes #6997

Thread notifications now come with sync response. This must be opted into and should also be able to fall back to previous homeserver versions that don't support the new unread thread notifications api.

Screenshots / GIFs

N/A

Tests

  • Sign into account on threads-dev.lab.element.dev
  • Get tagged in a thread using another account
  • See notification is highlighted in the timeline screen top-right icon

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 13

Checklist

@ericdecanini ericdecanini marked this pull request as ready for review October 20, 2022 23:50
@ericdecanini ericdecanini changed the base branch from feature/nfe/threads_notifications_and_receipts to develop October 20, 2022 23:50
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks. 2 first remarks on the form.

val threadList = it.invoke()
val isUserMentioned = threadList?.firstOrNull { threadRootEvent ->
threadRootEvent.root.threadDetails?.threadNotificationState == ThreadNotificationState.NEW_HIGHLIGHTED_MESSAGE
}?.let { true } ?: false
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}?.let { true } ?: false
} != null

is maybe simpler

)
)
}
val threadNotificationsSupported = session.homeServerCapabilitiesService().getHomeServerCapabilities().canUseThreadReadReceiptsAndNotifications
Copy link
Member

Choose a reason for hiding this comment

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

Keeping if (room == null) return would simplify the rest of the code.

# Conflicts:
#	matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/RealmSessionStoreMigration.kt
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

@sonarcloud
Copy link

sonarcloud bot commented Oct 26, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

10.5% 10.5% Coverage
0.0% 0.0% Duplication

@ericdecanini ericdecanini merged commit 29d3856 into develop Oct 26, 2022
@ericdecanini ericdecanini deleted the feature/eric/msc3773 branch October 26, 2022 14:44
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.

Implement MSC3773 (notifications for threads)
2 participants