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

Invitation notifications are not dismissed automatically if room is joined from another client (#347) #583

Merged
merged 4 commits into from
Oct 9, 2019

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented Sep 24, 2019

Fixes #347

Also do some cleanup

Notification for an invitation is still incomplete, this is tracked here: #582

Pull Request Checklist

  • Changes has been tested on an Android device or Android emulator with API 16
  • UI change has been tested on both light and dark themes
  • Pull request is based on the develop branch
  • Pull request updates CHANGES.md
  • Pull request includes screenshots or videos if containing UI changes
  • Pull request includes a sign off

@bmarty bmarty added this to the Stabilization1 milestone Sep 24, 2019
@bmarty bmarty requested a review from BillCarsonFr September 24, 2019 07:09
@bmarty bmarty force-pushed the feature/invot_notification branch from ee90060 to 50a0660 Compare October 8, 2019 08:57
Comment on lines 43 to 48
interface PushRuleListener {
fun onMatchRule(event: Event, actions: List<Action>)
fun onRoomJoined(roomId: String)
fun onRoomLeft(roomId: String)
fun onEventRedacted(redactedEventId: String)
fun batchFinish()
Copy link
Member

Choose a reason for hiding this comment

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

This class is now handling more things that events that matched a push rules, could be renamed to something else? NoteworthyEventListener? ImportantEvents? or something more related to the fact that it will be used by the notification drawer? NotificationActionsListener?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, you are right, we will rename that later

fun dispatchRoomLeft(roomId: String) {
try {
listeners.forEach {
it.onRoomLeft(roomId)
Copy link
Member

Choose a reason for hiding this comment

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

should catch inside each iteration and continue?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -499,7 +507,8 @@ class NotificationDrawerManager @Inject constructor(private val context: Context
companion object {
private const val SUMMARY_NOTIFICATION_ID = 0
private const val ROOM_MESSAGES_NOTIFICATION_ID = 1
private const val ROOM_EVENT_NOTIFICATION_ID = 2
private const val ROOM_INVITATION_NOTIFICATION_ID = 2
private const val ROOM_EVENT_NOTIFICATION_ID = 3
Copy link
Member

Choose a reason for hiding this comment

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

This event was id 2 before. Maybe add the new one as 3 (potential minor migration issue)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, good catch, thanks

@bmarty bmarty merged commit 36c5f9a into develop Oct 9, 2019
@bmarty bmarty deleted the feature/invot_notification branch October 9, 2019 10:48
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.

Invitation notifications are not dismissed automatically
2 participants