-
Notifications
You must be signed in to change notification settings - Fork 166
Add support for multiple content shares #998
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
} from 'amazon-chime-sdk-component-library-react'; | ||
|
||
const App = () => { | ||
const { toggleContentShare } = useContentShareControls(); | ||
|
||
return ( | ||
<MeetingProvider> | ||
<ContentShare nameplate='Content share' /> | ||
<ContentShare nameplate="Content share" /> |
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.
Are we using " or ' in this repo?
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.
We use single quote for JS, but here it should be double quote because it's a property on JSX element. Like we should use double quote on HTML element
@@ -10,37 +10,49 @@ import { BaseSdkProps } from '../Base'; | |||
|
|||
interface Props extends BaseSdkProps { | |||
nameplate?: string; | |||
tileId?: number; |
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.
Note that since the VideoGrid
UI Component only supports one featured area/tile, when there's two content shares available, I split the featured area into two parts to display both content share tiles there. This change only exists in meeting demo (I implemented a custom VideoTileGrid component in demo app) for testing purpose.
For builders, they will need to create VideoTileGrid to handle multiple content share tiles based on their UX using updated ContentTile
component.
@@ -21,17 +21,58 @@ import { | |||
reducer, | |||
} from './state'; | |||
|
|||
// Define the supported range for content shares | |||
const MIN_SUPPORTED_CONTENT_SHARES = 1; | |||
const MAX_SUPPORTED_CONTENT_SHARES = 2; |
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.
If in the future we supports more than 2 content shares from backend, we simply just need to update this constant.
// The chime attendee ID of the user sharing | ||
sharingAttendeeId: string | null; | ||
// Whether the user can start a content share based on current limits and `isLocalUserSharing` | ||
canStartContentShare: boolean; |
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.
We might want to change this to hasReachedContentShareLimit
so it's decoupled from isLocalUserSharing
, let me know your thoughts
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.
+1 for decouple, feel like state should be independent from each other here, also, builder may have feature on hasReachedContentShareLimit
only.
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.
My initial thought is to add a convenient boolean flag that builders can use to disable the content share button. This is needed to handle two scenarios:
- When content sharing limits have been reached
- When the local attendee is already sharing content
For scenario 1, previously, with single content share, if a second attendee started sharing, the SDK would automatically stop the existing share (takeover). However, now that we support multiple (2) concurrent content shares, attempting a third share will fail due to media limits, and the takeover won't occur. Since error handling for this case isn't straightforward (the error can't be caught directly in the API call), it's better to proactively disable the content share button when limits are reached.
Even without canStartContentShare
builder can still achieve the same by using
const canStartContentShare = tiles.length < maxContentShares && !isLocalUserSharing
It's a comparison between below three options, and I think option 1 is most convenient and aligns with my intention.
const disabled = canStartContentShare; // option 1
const disabled = hasReachedContentShareLimit && !isLocalUserSharing // option 2
const disabled = tiles.length < maxContentShares && !isLocalUserSharing // option 3
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.
if there an use case that builder can implement to kick off the 1st sharing when the 3rd starts? In this case, even though it reaches the max limit, but it literally is allowed to start content share.
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.
Today we don't allow three content shares from media side. So they cannot start a 3rd one, but they can stop one content share first, then canStartContentShare
becomes true and then start the 3rd (which is in fact the second content share stream in the meeting)
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.
Hmm thinking option 1 is simplest for builders? Reason: The limit on whether a content share can start or not when its at max limit + local user sharing is decided by the library and not by the builders. If we add hasReachedContentShareLimit
instead, then it seems builder will have to make such a decision knowing what all applies for a content share to be not allowed.
Thus, current props sounds fine to me. But we have to explain this in the documentation.
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.
Cool, I'll go with option 1 now. We can always add hasReachedContentShareLimit
if we find it needed later.
@@ -11,6 +11,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|||
### Added | |||
|
|||
- Added support for multiple content shares in meetings through the `ContentShareProvider`. The provider now accepts a `maxContentShares` prop (default: 1, range 1-2) to specify the maximum number of concurrent content shares allowed. |
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.
nonblocker: maxConcurrentContentShare
may be a better name?
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.
I feel 'concurrent' is redundant here, since 'maxContentShares' itself is already clear and unambiguous.
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.
What if we dont have such a prop? Can we take Video tiles like route. For example, we have a max limit on video streams but we dont have a prop for the max limit for them limiting builders. Correct me if not here. Have to check how that is limited component usage wise.
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.
I'm not following your question, could you explain a bit more?
We need this props for two reasons:
- Since this is a breaking change and we're not planning a major version release, we need to implement an opt-in mechanism so builders can explicitly enable this feature.
- Builder might want to limit concurrent content share to 1 even when 2 is supported, this prop can handle this conveniently. Alternatively builder can check the number of number active content share via
tiles.length
, and have a flag to disable the button when ittiles.length <=1
by themselves.
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.
Right, just trying to see whether we adding a new prop vs using the tiles.length
is fine too. If the later is fine, why introduce a new one? Now still going through review, will understand and sync on the breaking change part. Are you saying that builders if today with existing library APIs try to start 2 content share (if they can), that would break content share components?
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.
If the later is fine, why introduce a new one?
Since this is a breaking change and we're not planning a major version release, we need to implement an opt-in mechanism so builders can explicitly enable this feature.
will understand and sync on the breaking change part. Are you saying that builders if today with existing library APIs try to start 2 content share (if they can), that would break content share components?
Before this change when the second attendee starts content share, sdk will stop existing content share (when it detects a second content share tile), so it ensures only one content share tile available. After this change, when allowing multiple content share, starting the second content share won't stop the existing one. This is the breaking behavior and user needs to explicitly opt-in this
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.
Right I get it now, its more like a feature flag in a way where its honored and backward compatible in the ContentShareProvider
.
<ContentTile | ||
objectFit="contain" | ||
className={className || ''} | ||
className={`ch-content-share--${tileIdToRender} ${className || ''}`} |
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.
is this a defined class name?
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.
No, this className is not used in fact, I added here to follow how we did for RemoteVideo
component, it serves as an identifier
amazon-chime-sdk-component-library-react/src/components/sdk/RemoteVideo/index.tsx
Line 50 in efb7f0f
className={`ch-remote-video--${tileId} ${className || ''}`} |
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.
The prefix classname is to make sure we can confidently override the styles on this one and is a way to let builders know that they should not change these I believe. ch
stands for chime
:) from those times.
attendeeIdToUpdate | ||
); | ||
|
||
const backwardCompatibility = updateBackwardCompatibility( |
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.
may have a better var and method name. maybe updatedState
and updateForBackwardCompatibility
?
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 is to handle backward compatibility for existing tileId
and sharingAttendeeId
.
const backwardCompatibility =
{
tileId: number | null; // updated value
sharingAttendeeId: string | null; // updated value
}
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.
I think const updatedStateForBackwardCompatibility
and updateForBackwardCompatibility()
best describe the purpose, but they are a little verbose.
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.
updated to a better naming in my opinion
|
||
If used within the `VideoGrid` component, it will automatically place the active tile in the featured grid slot. It takes precedence over the featured video tile. | ||
|
||
Once a meeting session has been started, a user can start and stop content sharing by using the `useContentShareControls` hook. | ||
|
||
## Multiple Content Shares | ||
|
||
With the support for multiple content shares, you can now specify which content share tile to render by providing the `tileId` prop. If no `tileId` is provided, the component will render the default content share tile from (`const { tileId } = useContentShareState()`). |
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.
So what will the tileId
default to in case one does not provide it as a prop? Suggest adding a clarification for builder to help get answer to this question in the docs.
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.
We should also link to ContentShareProvider
with a small explanation on how ContentShareProvider maxContentShares={2}
impacts the ContentShare
component.
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.
If no
tileId
is provided, the component will render the default content share tile from (const { tileId } = useContentShareState()
).
I think this is clear enough. It will use the existing tileId
property from useContentShareState()
hook
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.
We should also link to ContentShareProvider with a small explanation on how ContentShareProvider maxContentShares={2} impacts the ContentShare component.
To clarify, maxContentShares
doesn't impact ContentShare
.
...rest | ||
}) => { | ||
const audioVideo = useAudioVideo(); | ||
const { tileId } = useContentShareState(); | ||
const contentShareState = useContentShareState(); |
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.
Thinking from keeping the props limited with no new optional props idea. How about we maintain tiles
as an array in the content share state. Based on the number of tiles, we should show those many ContentTile
s?
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.
But yes, we would have to maintain support of contantShareState.tileId
.
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.
In short, I decide to let builder to decide how and where to render the multiple content share tiles via the new tileId
of ContentShare
component. Check code example in its documentation for example.
{...rest} | ||
ref={videoEl} | ||
data-content-share-attendee={attendeeId} |
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.
Is this for testing?
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.
yeah, this is to make it easier for builder to know which tile is for which attendee so they can locate the element quickly.
lgtm overall. |
### State | ||
|
||
```javascript | ||
{ | ||
// The tile ID of the active content share | ||
// The tile ID of the active content share (deprecated, maintained for backward compatibility) | ||
// When multiple content shares are present, this points to the most recently started content share |
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.
If there is a task for next major version release, we should have a point to rename this to activeTileId
:)
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.
No, we don't plan a major version release. When we do major version release in the future we will delete tileId
and sharingAttendeeId
and builder should use the new collections instead.
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.
Yes it seems now the active tiles will be under tiles
and current tileId -> tiles[0] where tiles will have a single content share to preserve backward compatibility.
@@ -22,8 +36,17 @@ The `ContentShareProvider` provides state and functionality for content sharing. | |||
// Whether or not the local user's content share is loading | |||
isLocalShareLoading: boolean; | |||
|
|||
// The chime attendee ID of the user sharing | |||
sharingAttendeeId: string | null; | |||
// Whether the user can start a content share based on current limits and `isLocalUserSharing` |
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.
We can expand a bit more on "current limits" for clarity here. Is this just the maxContentShares
?
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.
Yeah, let me update it, it should be maxContentShares
, current limits is not very clear.
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.
Seems like still needs an update?
@@ -63,7 +128,7 @@ import { ContentShareProvider } from 'amazon-chime-sdk-component-library-react'; | |||
|
|||
const App = () => ( | |||
<CustomAudioVideoProvider> | |||
<ContentShareProvider> | |||
<ContentShareProvider maxContentShares={2}> |
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.
Its fine to drop the drop from here as its covered under the example updated above.
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.
updated
@@ -13,6 +13,17 @@ The `useContentShareControls` hook returns the state and functionality around st | |||
// Whether or not the local content share is paused | |||
paused: boolean; | |||
|
|||
// Whether or not the local user is currently sharing content | |||
isLocalUserSharing: boolean; |
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.
Can these be accessed from useContentShareState
and we do not add them in this hook as well? The idea seems like this provides actions on content share vs the other one providing the state. I do see we have paused
here, but checking if the other hook works or we need them repeated at two places. I think we should unify the usage to what the naming suggests.
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.
These are already available in this hook, we simply forgot updating the type definition and documentation 5 years ago. The only new property is canStartContentShare
|
||
// Content is being shared | ||
const contentShareTileId = tiles[0] // Or deprecated `tileId` | ||
const attendeeId = attendeeIdToTileId[contentShareTileId.toString()] // Or deprecated `sharingAttendeeId` |
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.
Can the TileId's be number
s and the key in attendeeIdToTileId
be number
to avoid this toString()
explicit conversions?
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.
Yeah object key can be a number. But this how RemoteVideoTileProvider is doing it, I decide to follow the same.
<ul> | ||
<li>Active content shares: {tiles.length}</li> | ||
<li>Current content share ID: {contentShareTileId}</li> | ||
<li>Shared by: {attendeeId[]}</li> |
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.
Have to correct to -> attendeeId
?
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.
updated
}; | ||
``` | ||
|
||
### With Multiple Content Shares |
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.
For builders to understand documentation wise thinking over single vs 2 content share approach example how about we document like?
Deprecated approach -> just a single tileId
. Example shows just the current tileId
. That way they still see this supported but deprecating and newer way is like below.
New approach -> Supports upto 2 concurrent content shares. Single vs 2 comes under this.
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.
I think that's fine, we don't want to encourage builders to use deprecated properties after this change.
I improved the comment a little bit to make it more clear
// Get the content share tile ID
const contentShareTileId = tiles[0]; // Note: Previously known as `tileId` (deprecated)
// Attendee Id for content sharer
const attendeeId = attendeeIdToTileId[contentShareTileId.toString()]; // Note: Previously known as `sharingAttendeeId` (deprecated)
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.
Hmm, that makes sense, this would remove how one might try to use with existing tileId
. I was more caring towards explaining that in more details with explicit examples but this works too.
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.
| RemoveAction | ||
| DeniedAction | ||
| ResetAction; | ||
| { type: ContentActionType.STARTING; payload?: StartingPayload } |
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.
We are updating some of the types for the payload here. What if someone is depending on this component currently and it starts failing for them as the types updated. Is this a backward incompatible change?
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.
These are internal type, so it's fine. It's export
here but not export
in index.ts file. It's not public available.
db5e0dd
to
c2ba9c5
Compare
c2ba9c5
to
ba943b9
Compare
Issue #:
Description of changes:
ContentShareProvider
. The provider now accepts amaxContentShares
prop (default: 1, range 1-2) to specify the maximum number of concurrent content shares allowed.ContentShareState
to track multiple content shares:tiles
,tileIdToAttendeeId
, andattendeeIdToTileId
.tileId
prop to theContentShare
component to specify which content share to render.canStartContentShare
state to control when content sharing is allowed based on the current number of shares and configured maximum.tileId
andsharingAttendeeId
properties, which now point to the most recently started content share when multiple shares are present.Testing
npm run build:release
locally?Yes
Update meeting demo to enable multiple content shares and verify the hook values, ContentTile components are working as expected.
Yes.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.