feat(external_api) facilitate gDM Electron #15575
Conversation
conference.js
Outdated
| @@ -175,6 +175,7 @@ let room; | |||
| * lib-jitsi-meet to detect and invoke | |||
| */ | |||
| window.JitsiMeetScreenObtainer = { | |||
| gDMSupported: true, | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Are we ok with not including a version check here just in case? gDM has been supported since M72 but do we know if it will work as expected with electron starting 72? IMHO, it will be safer to stick to versions we know where it works as expected.
There was a problem hiding this comment.
Good point. I'll add an extra check to LJM.
This flag means JM has the necessary APIs to make it work via the iframe API but LJM should decide which flow to use.
There was a problem hiding this comment.
yeah makes sense to add it there.
There was a problem hiding this comment.
On a second thought, note that the new behavior will only happen in the Electron SDK 7 (currently unreleased).
I think supporting a > 2 year old electron with the new SDK is not in the cards.
There was a problem hiding this comment.
Why it is failing?
I'm also wondering isn't it better to pass this to LJM instead of having it as a global? Or for some reason we can't do that?
There was a problem hiding this comment.
-
About the passing - I guess init of LJM works or maybe as part of createLocalTrack.
-
I'm sorry for the question, I'm almost certain I'm missing something here. I kind of don't understand why do we need this flag at all. Is it to accommodate use cases where somebody will use only LJM without jitsi-meet?
What I'm confused is that for all new versions after this PR we will have this flag set to true and in LJM we will only execute the code from the if branch where the flag is true and for the versions before this PR we will just have the old code. I mean since jitsi-meet is tied to a specific LJM version what is the point of the flag. Do we check it somewhere else?
There was a problem hiding this comment.
Let me take another look.
There was a problem hiding this comment.
Alright, I took another approach. See: jitsi/lib-jitsi-meet#2634
Basically, we always try gDM first, and fallback to the previous flow if it's not supported (there is a specific exception that Electron throws).
Thus, we no longer need this flag (that's how I started but got lost along the way).
I also left comments both here and in LJM so we axe all the old code (and the global!) once an SDK supporting gDM has been out for a while.
|
Jenkins please test this please. |
conference.js
Outdated
| @@ -175,6 +175,7 @@ let room; | |||
| * lib-jitsi-meet to detect and invoke | |||
| */ | |||
| window.JitsiMeetScreenObtainer = { | |||
| gDMSupported: true, | |||
There was a problem hiding this comment.
Are we ok with not including a version check here just in case? gDM has been supported since M72 but do we know if it will work as expected with electron starting 72? IMHO, it will be safer to stick to versions we know where it works as expected.
| * after with the legacy SS suport was removed from the electron SDK. If we remove it now the SS for Electron | ||
| * clients with older versions wont work. | ||
| */ | ||
| _isNewElectronScreensharingSupported() { |
There was a problem hiding this comment.
Why are we removing this?
The support for this was recently removed in the electron-sdk (a3622a3). Shouldn't we wait for a bit the ^^^ commit to be released in the clients? If it is not released it seems that we are going to fallback to the old flow just because this function is missing....
There was a problem hiding this comment.
The old flow is gone, so there is no point in keeping the check any longer. Arguably this should have been made in a3622a3 because there are no longer 2 flows. It's just the new one.
There was a problem hiding this comment.
Alright, I restored this. We have a bit of a mess here so I got a bit confused. I think we need to bump the Electron SDK to 7 and then after a while to 8, where we only leave the new-new way of doing SS, using gDM and gDM only.
| @@ -1046,8 +1046,20 @@ function initCommands() { | |||
| callback(conference?.getMeetingUniqueId() || ''); | |||
| break; | |||
| } | |||
| case '_new_electron_screensharing_supported': { | |||
| @@ -159,7 +159,7 @@ async function _toggleScreenSharing( | |||
| } catch (error) { | |||
| dispatch(handleScreenSharingError(error, NOTIFICATION_TIMEOUT_TYPE.MEDIUM)); | |||
|
|
|||
| throw error; | |||
| return; | |||
There was a problem hiding this comment.
Why do we need this change?
There was a problem hiding this comment.
See the commit log. Nobody catches the exception so it's thrown into the void. We are already handling the error above and also logging it in LJM so there is nothing the throw here accomplishes.
| @@ -173,7 +173,7 @@ async function _toggleScreenSharing( | |||
| if (!desktopAudioTrack) { | |||
| dispatch(handleScreenSharingError(AUDIO_ONLY_SCREEN_SHARE_NO_TRACK, NOTIFICATION_TIMEOUT_TYPE.MEDIUM)); | |||
|
|
|||
| throw new Error(AUDIO_ONLY_SCREEN_SHARE_NO_TRACK); | |||
| return; | |||
There was a problem hiding this comment.
Why do we need this change?
There is nobody to catch it and we already show the error as a notification.
saghul
force-pushed
the
electron-gdm
branch
2 times, most recently
from
84639c3 to
c569890
Compare
In order to use gDM in Electron the flow is somewhat reversed. It starts from the Electron main process, so we need an API in the external_api that can trigger the builtin picker. The picker is still necessary.
In order to use gDM in Electron the flow is somewhat reversed. It starts from the Electron main process, so we need an API in the external_api that can trigger the builtin picker. The picker is still necessary.