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

Always notify in one2one rooms #1029

Merged
merged 1 commit into from
Aug 2, 2018

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Jul 9, 2018

Notification is only send, when you don't have an active session for the room.
Or should we still do this, so you beep-boop when you have the tab open somewhere in the background? @mario @jancborchardt

Fix #875

@mario
Copy link
Contributor

mario commented Jul 9, 2018

Few things conceptually before I look into the code:

a) We should probably kill mentions and just send general chat message, but up to you (e.g. if user is mentioned, do we only send mention and not a general chat message to THAT user?)

Based on your code, it seems we keep MENTIONS only for group/public chats, and always notify on one-to-one chats. Let me know if I'm wrong.

b) We should ALWAYS send notifications and the client should decide when to show it

My idea is the following:
- you receive a push notification for chat

  • are there already notifications for that chat?
    • YES - Show "There are more pending messages" notification
    • NO - Show the message in a notification

c) Is there a reason we do this only for one2one conversations?

@nickvergessen
Copy link
Member Author

Is there a reason we do this only for one2one conversations?

Yes, imagine your phone ringing 500 times during a company call.

@mario
Copy link
Contributor

mario commented Jul 9, 2018

@nickvergessen for chat notifications? If you're in a call, all notifications would play NO sound, of course.

@nickvergessen
Copy link
Member Author

Yeah well, but does your phone know you are active on the desktop or vice versa?

@mario
Copy link
Contributor

mario commented Jul 10, 2018

No (well, partly only). Unfortunately that's an issue, but not a huge one.

@GoetheG
Copy link

GoetheG commented Jul 15, 2018

Let the user decide.

@mario
Copy link
Contributor

mario commented Jul 16, 2018

@GoetheG this is not how it should work.

@jancborchardt
Copy link
Member

@nickvergessen yes, notification should also be sent when tab is open in background. But of course only for 1on1 calls or mentions to you in groups.

@nickvergessen nickvergessen force-pushed the feature/noid/always-notify-in-one2one-rooms branch from c796aec to bced868 Compare July 27, 2018 10:06
@nickvergessen
Copy link
Member Author

Rebased on #1067 to resolve conflicts

@mario
Copy link
Contributor

mario commented Jul 27, 2018

So will this be changed for every type of room? :)

@nickvergessen
Copy link
Member Author

See PR title for details

@mario
Copy link
Contributor

mario commented Jul 27, 2018

@nickvergessen we should definitely still do it when in the background.

But I want this to work in the group/public calls too.

@nickvergessen
Copy link
Member Author

Which we cant do with the current iOS notifications because it can't silence them... Step by step inti the right direction

@nickvergessen nickvergessen force-pushed the feature/noid/always-notify-in-one2one-rooms branch from bced868 to ad8afc6 Compare July 31, 2018 15:41
@nickvergessen
Copy link
Member Author

@Ivansss ready to review

@nickvergessen nickvergessen force-pushed the feature/noid/always-notify-in-one2one-rooms branch 3 times, most recently from 602f197 to b2a0c58 Compare August 2, 2018 09:19
@nickvergessen nickvergessen force-pushed the feature/noid/always-notify-in-one2one-rooms branch from b2a0c58 to b74f91b Compare August 2, 2018 09:30
@Ivansss Ivansss merged commit cd5cd44 into master Aug 2, 2018
@Ivansss Ivansss deleted the feature/noid/always-notify-in-one2one-rooms branch August 2, 2018 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants