-
Notifications
You must be signed in to change notification settings - Fork 120
UrlParams: Intent system update, split into configuration and propreties #3376
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
Conversation
Co-authored-by: Robin <robin@robin.town>
Co-authored-by: Robin <robin@robin.town>
ec4345c
to
586bbb8
Compare
src/UrlParams.ts
Outdated
controlledAudioDevices: platform === "desktop" ? false : true, | ||
skipLobby: false, | ||
returnToLobby: false, | ||
sendNotificationType: undefined, |
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.
This should be "notification"
, because it's possible to start a call under this intent too if the previous call ends while you're on the lobby screen.
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.
And the logic for checking this would run in EC right?
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.
sendNotificationType
is slightly wrong. More correct would be requestSendingNotificationtype
(I remember we discussed that we do not have a "should do sth if checks pass" var prefix)
src/UrlParams.ts
Outdated
intentPreset = { | ||
confineToRoom: true, | ||
appPrompt: false, | ||
preload: true, | ||
header: platform === "desktop" ? HeaderStyle.None : HeaderStyle.AppBar, | ||
showControls: true, | ||
hideScreensharing: false, | ||
allowIceFallback: true, | ||
perParticipantE2EE: true, | ||
controlledAudioDevices: platform === "desktop" ? false : true, | ||
skipLobby: true, | ||
returnToLobby: false, | ||
sendNotificationType: "notification", | ||
}; | ||
break; | ||
case UserIntent.JoinExistingCall: | ||
intentPreset = { | ||
confineToRoom: true, | ||
appPrompt: false, | ||
preload: true, | ||
header: platform === "desktop" ? HeaderStyle.None : HeaderStyle.AppBar, | ||
showControls: true, | ||
hideScreensharing: false, | ||
allowIceFallback: true, | ||
perParticipantE2EE: true, | ||
controlledAudioDevices: platform === "desktop" ? false : true, | ||
skipLobby: false, | ||
returnToLobby: false, | ||
sendNotificationType: undefined, | ||
}; |
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.
Non-blocking: Since the difference between these cases is pretty minute (only skipLobby
changes), it'd be nice to collapse these two branches into one, so it's clearer what changes
Needed for implementing #3372 more cleanly
This PR updates the intend system. We want to solve this before starting on the telephone usecase implementation to prohibit us needing to make too many rust-sdk PR's and to converge to a solution we think brings the best benefits to maintain the url paramters.
This solution tries to get us the most features from this list:
By implementing intends with a "preset" architecture:
platform
This PR changes:
UrlProperties
andUrlConfiguration
(configuration are the ones where you can skip with one intend, properties are always required)