feat: Implement BrowserWindow.getMediaSourceId() and BrowserWindow.moveAbove(mediaSourceId) #18926

CapOM · 2019-06-21T18:43:25Z

Description of Change

BrowserWindow.{focus,blur,moveTop}() are not enough in some
situations. For example when implementing an overlay that
follows another window that can lose focus. In that case
it is useful to move the overlay above the tracked window.

sourceId is a string in the format of DesktopCapturerSource.id,
for example "window:1869:0".

Resolves #18922.

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR title follows semantic commit guidelines
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Added BrowserWindow.getMediaSourceId() and BrowserWindow.moveAbove(mediaSourceId)

CapOM · 2019-06-21T21:40:19Z

CC @MarshallOfSound @alexeykuzmin , Hi I am not sure who to add as reviewer, thx.

shell/browser/native_window_views.cc

CapOM · 2019-06-24T23:06:46Z

I have added 5 unit tests

CapOM · 2019-06-25T22:09:42Z

I am fighting with the unit tests (flakiness of API I am using in the tests but not the new API itself).
But the code is ready to be re-reviewed. Please take a look, thx!

spec-main/api-browser-window-spec.ts

spec/api-browser-window-spec.js

CapOM · 2019-06-26T20:52:18Z

I addressed remaining issues. The test failure seem unrelated to this change. Please take a look, thx!

CapOM · 2019-07-02T16:32:06Z

Hi, gentle ping ?

codebytere

The general structure and mac-specific implementation of this looks ok to me but i'd also like thoughts about linux and windows before merging this.

cc @zcbenz @ckerr @nornagon

shell/browser/api/atom_api_top_level_window.cc

spec-main/api-browser-window-spec.ts

zcbenz

The implementation looks good to me.

shell/browser/native_window_views.cc

CapOM · 2019-07-03T23:25:45Z

Hi, I have addressed all the remaining remarks, please take a look, thx!

CapOM · 2019-07-09T16:54:14Z

Hi @nornagon , gentle ping ?

nornagon

generally lgtm, just a few concerns with the tests.

shell/browser/native_window_views.cc

spec-main/api-browser-window-spec.ts

spec/api-browser-window-spec.js

spec/api-desktop-capturer-spec.js

CapOM · 2019-07-10T23:34:34Z

Hi @nornagon I think I addressed all your remarks, let me know if I forgot something, thx!

spec/api-browser-window-spec.js

codebytere · 2019-07-16T15:14:14Z

BrowserWindow module window.getMediaSourceId() returns valid source id - returns valid source id

still seems to be failing @CapOM

codebytere · 2019-08-05T16:15:54Z

@CapOM would you still like to move forward with this? it needs a rebase as well.

CapOM · 2019-08-08T16:07:04Z

Back from vacations, I just rebased the 2 commits for now.

codebytere · 2019-08-09T02:24:23Z

@CapOM test failure on macOS:

BrowserWindow module window.getMediaSourceId() returns valid source id - returns valid source id

AssertionError: expected 'window:-1:0' to match /^window:\d+:\d+$/

CapOM · 2019-08-09T03:51:49Z

Thx, I will have a look.

Return the Window id in the format of DesktopCapturerSource's id. For example "window:1234:0". electron#16460 Notes: Added BrowserWindow.getMediaSourceId

BrowserWindow.{focus,blur,moveTop}() are not enough in some situations. For example when implementing an overlay that follows another window that can lose focus. In that case it is useful to move the overlay above the tracked window. sourceId is a string in the format of DesktopCapturerSource.id, for example "window:1869:0". Notes: Added BrowserWindow.moveAbove(mediaSourceId) electron#18922

CapOM · 2019-08-13T19:09:58Z

@codebytere I fixed the test. The -1 was because [NSWindow windowNumber] returns the max value of uint64_t which is 18446744073709551615 (2^64 - 1) until the window is ordered to the screen for the first time. So the test now wait that the window is shown.

(the reason why it is translated to -1 in the string is because content:DesktopMediaID cast it to intptr_t instead of uintptr_t, so the conversion to string through content:DesktopMediaID.ToString selects the long long signature of the converting function. Instead of selecting the one that takes unsigned long long as parameter)

codebytere · 2019-08-13T19:14:15Z

@CapOM great detective work 🕵

i'll get this merged as soon as CI is ✅

CapOM · 2019-08-14T17:11:23Z

CI looks green, thx!

release-clerk · 2019-08-15T06:51:20Z

Release Notes Persisted

Added BrowserWindow.getMediaSourceId() and BrowserWindow.moveAbove(mediaSourceId)

bynho · 2020-01-15T14:21:27Z

Has this been removed? I cant find the API to get the media source id mentioned in this PR

CapOM · 2020-01-15T17:27:46Z

Commits are there:

9ca6bc0
64131e3

And it is shipped in electron starting from 8.0

bynho · 2020-01-15T23:58:33Z

Fantastic. Thank you @CapOM

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 21, 2019

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 22, 2019

codebytere reviewed Jun 24, 2019

View reviewed changes

shell/browser/native_window_views.cc Outdated Show resolved Hide resolved

CapOM force-pushed the implement_moveAbove branch 2 times, most recently from fff8470 to 1141d12 Compare June 24, 2019 23:04

CapOM changed the title ~~feat: Implement BrowserWindow.moveAbove(mediaSourceId)~~ feat: Implement BrowserWindow.getMediaSourceId() and BrowserWindow.moveAbove(mediaSourceId) Jun 24, 2019

CapOM force-pushed the implement_moveAbove branch 4 times, most recently from 485d2ce to 1e24458 Compare June 25, 2019 21:32

CapOM force-pushed the implement_moveAbove branch from 1e24458 to 50e07bf Compare June 25, 2019 22:40

codebytere reviewed Jun 26, 2019

View reviewed changes

spec-main/api-browser-window-spec.ts Show resolved Hide resolved

spec-main/api-browser-window-spec.ts Outdated Show resolved Hide resolved

spec-main/api-browser-window-spec.ts Show resolved Hide resolved

spec/api-browser-window-spec.js Outdated Show resolved Hide resolved

CapOM force-pushed the implement_moveAbove branch 2 times, most recently from cf62392 to b5dc201 Compare June 26, 2019 18:24

codebytere approved these changes Jul 2, 2019

View reviewed changes

nornagon reviewed Jul 2, 2019

View reviewed changes

shell/browser/api/atom_api_top_level_window.cc Outdated Show resolved Hide resolved

nornagon reviewed Jul 2, 2019

View reviewed changes

spec-main/api-browser-window-spec.ts Show resolved Hide resolved

CapOM force-pushed the implement_moveAbove branch from b5dc201 to 8188541 Compare July 2, 2019 19:42

zcbenz approved these changes Jul 3, 2019

View reviewed changes

shell/browser/native_window_views.cc Outdated Show resolved Hide resolved

CapOM force-pushed the implement_moveAbove branch 5 times, most recently from 02953ce to 24dc332 Compare July 3, 2019 23:24

nornagon requested changes Jul 9, 2019

View reviewed changes

CapOM force-pushed the implement_moveAbove branch from 24dc332 to 0580552 Compare July 10, 2019 23:28

CapOM force-pushed the implement_moveAbove branch from 0580552 to 1f23b2f Compare July 10, 2019 23:37

nornagon approved these changes Jul 15, 2019

View reviewed changes

spec/api-browser-window-spec.js Outdated Show resolved Hide resolved

nornagon added the semver/minor backwards-compatible functionality label Jul 15, 2019

CapOM force-pushed the implement_moveAbove branch 2 times, most recently from 9b55504 to 09b779b Compare July 15, 2019 16:58

CapOM force-pushed the implement_moveAbove branch from 09b779b to 8814f44 Compare August 8, 2019 16:06

Julien Isorce and others added 2 commits August 13, 2019 11:47

feat: Implement BrowserWindow.getMediaSourceId

64131e3

Return the Window id in the format of DesktopCapturerSource's id. For example "window:1234:0". electron#16460 Notes: Added BrowserWindow.getMediaSourceId

CapOM force-pushed the implement_moveAbove branch from 8814f44 to 9ca6bc0 Compare August 13, 2019 18:47

zcbenz merged commit 680399f into electron:master Aug 15, 2019

deepak1556 mentioned this pull request Aug 20, 2019

ci: add macOS debug builds #19831

Merged

3 tasks

ghost mentioned this pull request Aug 1, 2020

BrowserWindow.getMediaSourceId() fails if window created with show: false #24809

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement BrowserWindow.getMediaSourceId() and BrowserWindow.moveAbove(mediaSourceId) #18926

feat: Implement BrowserWindow.getMediaSourceId() and BrowserWindow.moveAbove(mediaSourceId) #18926

CapOM commented Jun 21, 2019 •

edited

CapOM commented Jun 21, 2019

CapOM commented Jun 24, 2019

CapOM commented Jun 25, 2019

CapOM commented Jun 26, 2019

CapOM commented Jul 2, 2019

codebytere left a comment

zcbenz left a comment

CapOM commented Jul 3, 2019

CapOM commented Jul 9, 2019

nornagon left a comment

CapOM commented Jul 10, 2019

codebytere commented Jul 16, 2019

codebytere commented Aug 5, 2019 •

edited

CapOM commented Aug 8, 2019

codebytere commented Aug 9, 2019

CapOM commented Aug 9, 2019

CapOM commented Aug 13, 2019

codebytere commented Aug 13, 2019

CapOM commented Aug 14, 2019

release-clerk bot commented Aug 15, 2019

bynho commented Jan 15, 2020

CapOM commented Jan 15, 2020 •

edited

bynho commented Jan 15, 2020

feat: Implement BrowserWindow.getMediaSourceId() and BrowserWindow.moveAbove(mediaSourceId) #18926

feat: Implement BrowserWindow.getMediaSourceId() and BrowserWindow.moveAbove(mediaSourceId) #18926

Conversation

CapOM commented Jun 21, 2019 • edited

Description of Change

Checklist

Release Notes

CapOM commented Jun 21, 2019

CapOM commented Jun 24, 2019

CapOM commented Jun 25, 2019

CapOM commented Jun 26, 2019

CapOM commented Jul 2, 2019

codebytere left a comment

Choose a reason for hiding this comment

zcbenz left a comment

Choose a reason for hiding this comment

CapOM commented Jul 3, 2019

CapOM commented Jul 9, 2019

nornagon left a comment

Choose a reason for hiding this comment

CapOM commented Jul 10, 2019

codebytere commented Jul 16, 2019

codebytere commented Aug 5, 2019 • edited

CapOM commented Aug 8, 2019

codebytere commented Aug 9, 2019

CapOM commented Aug 9, 2019

CapOM commented Aug 13, 2019

codebytere commented Aug 13, 2019

CapOM commented Aug 14, 2019

release-clerk bot commented Aug 15, 2019

bynho commented Jan 15, 2020

CapOM commented Jan 15, 2020 • edited

bynho commented Jan 15, 2020

CapOM commented Jun 21, 2019 •

edited

codebytere commented Aug 5, 2019 •

edited

CapOM commented Jan 15, 2020 •

edited