-
Notifications
You must be signed in to change notification settings - Fork 760
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
ignore push for a thread if it's currently visible to user #7641
ignore push for a thread if it's currently visible to user #7641
Conversation
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.
LGTM, thanks!
Just a few remarks, but no blockers.
@@ -142,11 +143,6 @@ class VectorPushHandler @Inject constructor( | |||
pushData.roomId ?: return | |||
pushData.eventId ?: return | |||
|
|||
// If the room is currently displayed, we will not show a notification, so no need to get the Event faster | |||
if (notificationDrawerManager.shouldIgnoreMessageEventInRoom(pushData.roomId)) { |
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.
I guess we do not have the threadId in the pushData
, true?
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.
We don't :(
* Should be called when the application is currently opened and showing timeline for the given threadId. | ||
* Used to ignore events related to that thread (no need to display notification) and clean any existing notification on this room. | ||
*/ | ||
fun setCurrentThread(threadId: String?) { |
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.
Maybe merge with the method setCurrentRoom
, just adding the parameter threadId: String?
to it?
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.
user can switch between different threads in the same rooms, so i think it's better to separate changing room and changing thread
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.
OK
SonarCloud Quality Gate failed. |
Type of change
Content
we don't show push notification for a room which is currently observed by the user, but we should also take in account which thread timeline user observes
Motivation and context
closes #7634