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-62241] Fix screen sharing from Calls popout window #3258

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

streamer45
Copy link
Contributor

Summary

As of #3174, some of the functionality in the Calls popout window (e.g. screen sharing) stopped working since desktopAPI is missing on the window object. Using the local preload seems to be the way to go.

Since we are here, I am also adding an extra dynamic menu item to access the developer tools of the popout, which can be helpful in debugging future issues like this one.

/cc @matthewbirtch

Ticket Link

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

Release Note

NONE

@streamer45 streamer45 added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Dec 16, 2024
@streamer45 streamer45 self-assigned this Dec 16, 2024
@amyblais amyblais added this to the v5.11.0 milestone Dec 16, 2024
@@ -362,6 +362,12 @@ describe('main/windows/callsWidgetWindow', () => {
urlUtils.isCallsPopOutURL.mockReturnValue(false);
expect(callsWidgetWindow.onPopOutOpen({url: 'http://localhost:8065/notpopouturl'})).toHaveProperty('action', 'deny');
});

it('should pop out and make sure preload is set', () => {
Copy link
Member

Choose a reason for hiding this comment

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

🎉

@streamer45 streamer45 removed the 2: Dev Review Requires review by a core committer label Dec 17, 2024
@streamer45
Copy link
Contributor Author

@DHaussermann You should be familiar with this since you hit it before while testing unrelated changes.

@DHaussermann
Copy link

/update-brach

@streamer45
Copy link
Contributor Author

@DHaussermann Updated

@devinbinnie
Copy link
Member

@DHaussermann Can we prioritize QA review on this? Need to cut the RC on Monday.

@DHaussermann
Copy link

@devinbinnie Yes, happy to make this a priority.

Perhaps you can assist? I tried testing this yesterday and can't get a Mac build to install.

When I unzip the build for ARM64 and try to launch it Mac OS tells me the app is damaged.
image

Is there another way to do this? I feel like this ☝️ is exactly what I did in the past for desktop PRs

@devinbinnie devinbinnie added the Build Apps for PR Builds signed builds for testing label Jan 17, 2025
@devinbinnie
Copy link
Member

@DHaussermann You need to add the "Build Apps for PR" label, that creates a signed app that you should be able to run. I've added it for you and you should be able to grab the build from the check when it is finished building.

@DHaussermann
Copy link

Thanks @devinbinnie I may have tried the label in the past at some point but was not sure where the build would be posted.

Taking a look now on Community the Desktop Builds channel does not show any new posts for a couple days. https://community.mattermost.com/core/channels/desktop-builds

Do they get posted elsewhere?

@devinbinnie
Copy link
Member

@DHaussermann They're in the artifacts for the GitHub Action: https://github.com/mattermost/desktop/actions/runs/12831481292

@DHaussermann
Copy link

@streamer45 I don't see this issue solved.

  • Install build from the pipeline that is now in the build-for-pr and confirm I'm running a dev build `Version 5.11.0-develop.1 - Start a call and open the expanded view
  • Click the screen share option
    Observed: The share button does not seem responsive and does not open the view were I can select what I want to share.

Can you please advice if you can repro this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: QA Review Requires review by a QA tester Build Apps for PR Builds signed builds for testing release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants