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

[MM-42737] Use stream ids instead of track ids #38

Merged
merged 1 commit into from
Mar 24, 2022
Merged

Conversation

streamer45
Copy link
Collaborator

Summary

After some debugging (thanks @cpoile 🙌 ) and knowledge gathering I figured out that relying on local track ids was a dangerous move as they are not passed consistently. Stream IDs instead seem to be the way to go, so going with those this time around.

This solution does also remove the annoying browser specific check. The moral of the story is: I should have listened to Firefox when it told me something was clearly wrong with my intended logic.

Oh, and this is mostly why R&D call failed so emphatically.

Ticket Link

https://mattermost.atlassian.net/browse/MM-42737

@streamer45 streamer45 added the 2: Dev Review Requires review by a core committer label Mar 24, 2022
@streamer45 streamer45 requested a review from cpoile March 24, 2022 18:22
@streamer45 streamer45 self-assigned this Mar 24, 2022
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Awesome!
Do you think this will solve the widget vs popout problem we were seeing?

@streamer45
Copy link
Collaborator Author

Awesome! Do you think this will solve the widget vs popout problem we were seeing?

Yes, as far as I could see it wasn't related at all to where the sharing was initiated. I was able to reproduce locally by unmuting first and then starting to share in a call where someone else shared already.

@streamer45 streamer45 added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Mar 24, 2022
@streamer45 streamer45 merged commit 6e94eb6 into main Mar 24, 2022
@streamer45 streamer45 deleted the MM-42737 branch March 24, 2022 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants