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

Fix: 1:1 call - screen sharing no longer works after pressing the cancel button in Element-Desktop #27301

Merged
merged 4 commits into from
Apr 9, 2024

Conversation

gumara-dev
Copy link
Contributor

@gumara-dev gumara-dev commented Apr 8, 2024

Fixes element-hq/element-desktop#1367

Signed-off-by: Stephan Raab [email protected]

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

@gumara-dev gumara-dev requested a review from a team as a code owner April 8, 2024 20:19
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Apr 8, 2024
t3chguy
t3chguy previously requested changes Apr 9, 2024
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

This needs a comment as it is non-obvious, as well as sign-off as per the checklist

Sign-off given on the changes (see CONTRIBUTING.md).

@gumara-dev
Copy link
Contributor Author

This needs a comment as it is non-obvious, as well as sign-off as per the checklist

Sign-off given on the changes (see CONTRIBUTING.md).

Signed-off-by: Stephan Raab [email protected]

Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

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

Hi, thanks for the contribution! It's not apparent to me how this fix works. Could you add a comment explaining why we need to always call the callback?

src/vector/platform/ElectronPlatform.tsx Outdated Show resolved Hide resolved
@gumara-dev
Copy link
Contributor Author

gumara-dev commented Apr 9, 2024

Hi, thanks for the contribution! It's not apparent to me how this fix works. Could you add a comment explaining why we need to always call the callback?

@robintown

However, the await function that was called by the matrix-js-sdk call.ts on line 1274

const stream = await this.client.getMediaHandler().getScreensharingStream(opts);

never returns. This only happens when a stream is selected.
If this return is now commented out and the this.ipc.call("callDisplayMediaCallback", source); is also executed on cancel, it works.

@gumara-dev gumara-dev requested a review from t3chguy April 9, 2024 18:20
@robintown
Copy link
Member

Okay, so in other words the getDisplayMedia promise will never resolve if we don't at least provide a dummy value? That's the kind of thing that should go in a comment in the code.

@gumara-dev
Copy link
Contributor Author

Okay, so in other words the getDisplayMedia promise will never resolve if we don't at least provide a dummy value? That's the kind of thing that should go in a comment in the code.

That’s correct 👌🏻

@gumara-dev
Copy link
Contributor Author

@robintown Thanks :)

@robintown robintown added this pull request to the merge queue Apr 9, 2024
Merged via the queue into element-hq:develop with commit 8ce46d3 Apr 9, 2024
23 of 24 checks passed
@gumara-dev gumara-dev deleted the fix_branch branch April 9, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1:1 call - screen sharing no longer works after pressing the cancel button
3 participants