Skip to content

Enable privacy notification tray for screen sharing#165

Open
shikhak wants to merge 3 commits intonwjs:nw83from
shikhak:enable-privacy-notification-tray-for-screen-sharing
Open

Enable privacy notification tray for screen sharing#165
shikhak wants to merge 3 commits intonwjs:nw83from
shikhak:enable-privacy-notification-tray-for-screen-sharing

Conversation

@shikhak
Copy link
Copy Markdown

@shikhak shikhak commented Jan 17, 2024

Filed bug: nwjs/nw.js#8156
To enable screen-sharing, these patches need to be undone.

@ayushmanchhabra
Copy link
Copy Markdown
Contributor

Just want to clarify that screen sharing was already working and this PR fixes and enables the stop sharing button which was previously missing?

@ayushmanchhabra
Copy link
Copy Markdown
Contributor

@rogerwang requesting your review, forgot to ping you here

#endif
std::unique_ptr<content::MediaStreamUI> ui;
return ui;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think you need to keep the return ui, or if not let's keep "#if 0" so up stream merge is easier.

@hthetiot
Copy link
Copy Markdown

hthetiot commented Apr 22, 2026

Hello @GnorTech,
Thx for maintener work you have been doing recently.

Would it be possible to review this one.
I do believe we might need to keep lines I highlighted in comment, but i may be wrong.
#165 (comment)

@GnorTech
Copy link
Copy Markdown
Member

The change to ShouldDisplayNotification is correct — removing is_nwjs_app() there will show the "Stop Sharing" button.

However, the change to GetDesktopCapturePermission should not be applied. That check auto-approves screen capture for NW.js apps, which is the expected behavior for desktop applications. Removing it would force users to click "Allow" on a permission dialog every time, which is unnecessary for a native desktop app.

@hthetiot
Copy link
Copy Markdown

Hello @shikhak, I hope you doing great.
Would it be possible to apply the review fixe above in you PR.
Alternatively if you wish I can make a separate PR.
Best regards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants