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

👥 Multiple sessions 👥 #5194

Merged
merged 41 commits into from
Mar 9, 2021
Merged

👥 Multiple sessions 👥 #5194

merged 41 commits into from
Mar 9, 2021

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Feb 17, 2021

spiderman

  • 😿 For now only multiple Talk sessions in different PHP sessions will be supported. So you can be in the same conversation with mobile apps, SIP, browser, etc at the same time, but not in the same chat in 2 tabs of the same browser. The additional burden is currently too much to handle
  • Transform the unique attendee key on the sessions table to a normal index
  • Make sure that we always select the Talk session of the current PHP session because there could be more than 1
  • In places where the session is not used, the parameter should be set to false/null, to avoid left joining an unneeded database table
  • Move integration tests to API v4
  • Move UI to API v4
  • Remove special handling of lower API versions as the routes don't match anymore
  • Add v4 capability and remove others

@nickvergessen nickvergessen added 2. developing feature: api 🛠️ OCS API for conversations, chats and participants labels Feb 17, 2021
@nickvergessen nickvergessen added this to the 💖 Next Major (22) milestone Feb 17, 2021
@nickvergessen
Copy link
Member Author

Rebase on #5215 for compatibility

@nickvergessen

This comment has been minimized.

@nickvergessen

This comment has been minimized.

@nickvergessen nickvergessen force-pushed the techdebt/noid/multi-session branch 2 times, most recently from ece8735 to d7507e5 Compare March 3, 2021 12:23
@nickvergessen nickvergessen marked this pull request as ready for review March 4, 2021 20:52
@nickvergessen
Copy link
Member Author

Ready to review. Don't be scared about the size, it's mostly integration tests bumping the required version of the API from none (v1) or v2 or v3 to the new one-and-only v4

docs/call.md Show resolved Hide resolved
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
Copy link
Member Author

nickvergessen commented Mar 8, 2021

Added 3 commits and rebased.
Works as expected now with the HPB from my POV.

Only thing is guests see "Conversation not found" after their previous session gets kicked because of duplication, but lets fix that later.

@PVince81
Copy link
Member

PVince81 commented Mar 8, 2021

HPB tests

  • TEST: could join and call twice with same user
  • TEST: video/muting is correctly linked to a session, doesn't apply to both
  • TEST: raising and lowering hand from both sessions properly updates the indicator in participant list for others (aggregated) 🚫 the participant list seems tied only to the first session so the hand only appears when first session raises/lowered, might need some tweaks for session id matching / aggregation => raise for separate fixing ?
  • TEST: kicking out user from conversation terminated both sessions
  • TEST: self-joined twice in separate browsers

@PVince81
Copy link
Member

PVince81 commented Mar 8, 2021

I've tested with HPB and all seemed to work fine.

There's only a minor issue with raise hand because we don't aggregate HPB style session id in the participant list, but perhaps we can fix this separately.

Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81
Copy link
Member

PVince81 commented Mar 8, 2021

some integration tests are failing still: https://drone.nextcloud.com/nextcloud/spreed/1816/4/3

/drone/server/apps/spreed/tests/integration/features/conversation/one-to-one.feature:121
    /drone/server/apps/spreed/tests/integration/features/conversation/one-to-one.feature:130
    /drone/server/apps/spreed/tests/integration/features/conversation/remove-participant.feature:10
    /drone/server/apps/spreed/tests/integration/features/conversation/remove-participant.feature:18
    /drone/server/apps/spreed/tests/integration/features/conversation/remove-participant.feature:30
    /drone/server/apps/spreed/tests/integration/features/conversation/remove-participant.feature:67
    /drone/server/apps/spreed/tests/integration/features/conversation/remove-participant.feature:79
    /drone/server/apps/spreed/tests/integration/features/conversation/remove-participant.feature:94

@nickvergessen
Copy link
Member Author

Ah yeah... that is what the fixme was about 😅

Signed-off-by: Joas Schilling <coding@schilljs.com>
@PVince81
Copy link
Member

PVince81 commented Mar 9, 2021

Also, I still have some features in my mind like "ask to unmute" or "mute from sidebar". Now since we only show a participant once during a call there the UX for such would be more tricky as we'd need to either ask the user which session they want to mute (maybe only one is unmuted by mistake) or do it for all of them.

Also not sure if it's a good idea to temporarily show one participant as multiple entries in the participant list during a call. Or group them somehow with an expandable arrow. Feels like it's going too far. (increased code complexity and UX complexity for not that much benefit)

@nickvergessen
Copy link
Member Author

Also, I still have some features in my mind like "ask to unmute" or "mute from sidebar". Now since we only show a participant once during a call there the UX for such would be more tricky as we'd need to either ask the user which session they want to mute (maybe only one is unmuted by mistake) or do it for all of them.

Yeah, let's discuss this separatly. For now muting works on the call view and its okay for now.

@nickvergessen
Copy link
Member Author

raising and lowering hand from both sessions properly updates the indicator in participant list for others (aggregated) no_entry_sign the participant list seems tied only to the first session so the hand only appears when first session raises/lowered, might need some tweaks for session id matching / aggregation => raise for separate fixing ?

#5349

Only thing is guests see "Conversation not found" after their previous session gets kicked because of duplication, but lets fix that later.

#5350

@PVince81 PVince81 merged commit 2a2a43a into master Mar 9, 2021
@PVince81 PVince81 deleted the techdebt/noid/multi-session branch March 9, 2021 08:15
@PVince81
Copy link
Member

PVince81 commented Mar 9, 2021

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review feature: api 🛠️ OCS API for conversations, chats and participants
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants