-
Notifications
You must be signed in to change notification settings - Fork 15.9k
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: set display_id in desktop capturer on Linux #33861
fix: set display_id in desktop capturer on Linux #33861
Conversation
💖 Thanks for opening this pull request! 💖 We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of commit messages with semantic prefixes:
Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
16b8303
to
c63a155
Compare
I've "tested" ad-hoc, by running the built electron, but running |
c63a155
to
a1d24dc
Compare
I also understand that this fix is for "Linux", but is currently X11-only. Is there a build flag or some-such I should be using instead of |
a1d24dc
to
7f1766f
Compare
(I'm also quite sure that the code here should be moved elsewhere -- I originally wrote the helper functions in ui/base/x/x11_display_util.cc, (which I guess is actually Chrome?) and just moved it in here so it would be self-contained & all in electron-land.) |
I believe what you're looking for is electron/shell/browser/native_window_views.h Lines 19 to 24 in 160d692
electron/shell/browser/native_window_views.cc Lines 541 to 544 in 160d692
|
7f1766f
to
9210512
Compare
Thanks for the tip @RaisinTen! Updated the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a lint warning
$ node ./script/lint.js && npm run lint:clang-format && npm run lint:docs
--- a/shell/browser/api/electron_api_desktop_capturer.cc
+++ b/shell/browser/api/electron_api_desktop_capturer.cc
@@ -39,7 +39,7 @@
#include "ui/gfx/x/randr.h"
#include "ui/gfx/x/x11_atom_cache.h"
#include "ui/gfx/x/xproto_util.h"
-#endif // defined(USE_OZONE_PLATFORM_X11)
+#endif // defined(USE_OZONE_PLATFORM_X11)
#endif // BUILDFLAG(IS_WIN)
9210512
to
df57ad8
Compare
Oops, thank-you, fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks pretty sane. I had a few questions/comments but no fundamental change requests.
df57ad8
to
f7626c3
Compare
@jamesnvc can you rebase this PR with the latest from main? |
f7626c3
to
d6262b7
Compare
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GN failures need to be addressed
Previously, display_id was an empty string, pending Chrome support for sharing individual screens. Now that this has been added, it is desirable to have this property set correctly. Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
Previously, display_id was an empty string, pending Chrome support for sharing individual screens. Now that this has been added, it is desirable to have this property set correctly. Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
I have automatically backported this PR to "20-x-y", please check out #35834 |
I have automatically backported this PR to "19-x-y", please check out #35835 |
/trop run backport-to 21-x-y |
The backport process for this PR has been manually initiated - sending your PR to |
Previously, display_id was an empty string, pending Chrome support for sharing individual screens. Now that this has been added, it is desirable to have this property set correctly. Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
I have automatically backported this PR to "21-x-y", please check out #35836 |
fix: set display_id in desktop capturer on Linux (#33861) Previously, display_id was an empty string, pending Chrome support for sharing individual screens. Now that this has been added, it is desirable to have this property set correctly. Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org> Co-authored-by: James Cash <james.nvc@gmail.com> Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
fix: set display_id in desktop capturer on Linux (#33861) Previously, display_id was an empty string, pending Chrome support for sharing individual screens. Now that this has been added, it is desirable to have this property set correctly. Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org> Co-authored-by: James Cash <james.nvc@gmail.com> Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
fix: set display_id in desktop capturer on Linux (#33861) Previously, display_id was an empty string, pending Chrome support for sharing individual screens. Now that this has been added, it is desirable to have this property set correctly. Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org> Co-authored-by: James Cash <james.nvc@gmail.com> Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
Previously, display_id was an empty string, pending Chrome support for sharing individual screens. Now that this has been added, it is desirable to have this property set correctly. Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
Description of Change
Previously,
display_id
was an empty string on Linux, pending Chrome support forsharing individual screens. Now that this has been added, it is
desirable to have this property set correctly.
Addresses #27732.
Checklist
npm test
passesnotes: Provided display_id for desktopCapturer on Linux.