[MV-391] Add screencast support #83

skyman503 · 2023-04-04T14:24:34Z

No description provided.

graszka22

In general the code in the Room screen is too complicated and too nested.
Consider doing it like this:
Move the participants part to another component.

Then calculate participantsWithTracks like you're doing it now and keep the focused participant+track in state.
Now you have two cases:

If there is a focused track then return the rendered focused track+non-focused participants (you can have multiple return statements in one component!)
Else return the participants in a grid.

And extract the logic of getting the tracks etc from the views to functions. That should take care of most of the nesting.

Also it would be nice if the RoomParticipant just took the whole available space and didn't care about aspect ratios etc.

About my Android device bug: the focused participant takes the whole width of the screen and the height according to aspect ratio. Then the not focused participants take the available height and they have aspect ration set to 1 so they take the same width. Probably my device is taller than yours. RN reports window size 411x936px, my model is xperia 10 III.
In my opinion the non focused participants should have 128x128px and the the focused participant takes the rest of available space. I'd fix that but right now the code is too complicated for me :D

example/src/screens/Room.tsx

example/src/components/StopScrencastingWithFocus.tsx

example/src/components/RoomParticipant.tsx

graszka22 · 2023-04-04T15:52:41Z

example/src/components/RoomParticipant.tsx

@@ -171,6 +190,13 @@ const styles = StyleSheet.create({
    aspectRatio: 1,
    alignSelf: 'center',
  },
+  videoTrackScreencast: {
+    flex: 1,
+    aspectRatio: 1,


can't we manage the aspect ratio in the parent component? that way would be simpler and we can get rid of the focused prop

The focused prop will have to stay either way since it is used to determine not only the style but also pin button behaviour and moving it to the parent would make it less readable in my opinion. Also managing aspect ratio in the parent isn't as simple as it may seem, since I want to keep the displayName / muted / pin icons in fixed positions whatever the aspect ratio is, and managing thet from the parent would affect that. That's also why I don't like the idea of room participant taking the entire space as it would depend on the parent aspect ratio.

skyman503 · 2023-04-05T11:03:44Z

In general the code in the Room screen is too complicated and too nested. Consider doing it like this: Move the participants part to another component.

Then calculate participantsWithTracks like you're doing it now and keep the focused participant+track in state. Now you have two cases:

If there is a focused track then return the rendered focused track+non-focused participants (you can have multiple return statements in one component!)

Else return the participants in a grid.

And extract the logic of getting the tracks etc from the views to functions. That should take care of most of the nesting.

Also it would be nice if the RoomParticipant just took the whole available space and didn't care about aspect ratios etc.

About my Android device bug: the focused participant takes the whole width of the screen and the height according to aspect ratio. Then the not focused participants take the available height and they have aspect ration set to 1 so they take the same width. Probably my device is taller than yours. RN reports window size 411x936px, my model is xperia 10 III. In my opinion the non focused participants should have 128x128px and the the focused participant takes the rest of available space. I'd fix that but right now the code is too complicated for me :D

I did a fair bit of refactoring and in my opinion Room.tsx is looking clean now. I don't like the idea of roomParticipant taking the whole available space, I'll write more on why in response to one of the other comments.

graszka22

Only a couple of simple code style fixes this time 👍

graszka22 · 2023-04-05T13:56:52Z

example/src/components/FocusedParticipant.tsx

+
+  return (
+    <View style={styles.focusedParticipantContainer}>
+      <View style={styles.focusedParticipant}>


again those two views can be merged?

Simple copy pasting won't work here, so I think it would be best to leave it as it is for now especially since MV-395 will be reworking it's styling anyway.

example/src/components/FocusedParticipant.tsx

example/src/components/Participants.tsx

example/src/components/RoomParticipant.tsx

graszka22 · 2023-04-05T14:17:28Z

example/src/components/RoomParticipant.tsx

+  const videoTrack = tracks.find((t) => t.id === trackId);
+  const videoTrackType = videoTrack?.metadata.type;
+  const audioTrack =
+    !videoTrack || videoTrackType === 'camera'


It's not clear what are you doing here: if there is no video track or the videotrack is from camera then search for audio track? Why? You already have a check if the track is screenshared

I never realised I could just that screenshared check from before, good point. Should be simpler now.

graszka22 · 2023-04-05T14:22:28Z

example/src/screens/Room.tsx

@@ -106,74 +113,25 @@ export const Room = () => {
          <View style={{ flex: 1 }}>
            {shouldShowParticipants && (
              <>


We no longer need the outer fragment (<>)

I don't think so. I tried removing that and it gives an error, which the editor constantly recommends to fix by wrapping in empty jsx tag.

huh, works for me :p

graszka22 · 2023-04-06T08:58:21Z

example/src/components/RoomParticipant.tsx

+  const videoTrack = trackId ? tracks.find((t) => t.id === trackId) : null;
+  const videoTrackType = videoTrack?.metadata.type;
+  const audioTrack =
+    videoTrackType !== 'screensharing'


I still think this check is unecessary (though it's simpler). You literally have the same one lower in the code (videoTrackType !== 'screensharing' &&)

skyman503 · 2023-04-06T09:49:09Z

I think useEffect in the Room screen solves the no videoTrack update bug we talked about, it should be all working now.

graszka22 · 2023-04-06T10:20:01Z

example/src/screens/Room.tsx

+      focusedParticipantData?.participant !== curretStateOfFocusedParticipant
+    ) {
+      setFocusedParticipantData({
+        participant: curretStateOfFocusedParticipant!,


what if pinned participant exits the call? I've got a crash

Good catch, thanks. I haven't realised that the participant that is being focused could disappear.

and get rid of this ! now, it's not necessary
and it caused the crash again, I'm considering adding a lint rule to prohibit using ! :D

graszka22

ok now it looks pretty good 👏
Maybe putting the whole focused participant instead of just id wasn't a good idea after all - now I see we now have kind of double state but we'll think that out and refactor this later. Sorry for that 😅

skyman503 added 4 commits April 4, 2023 15:37

init

055cc05

Add stop screencast screen

71c19c5

Fix user joining with no camera

a8a6e27

Remove console log

b6ba438

skyman503 changed the title ~~@skyman503/mv 391 v1~~ [MV-391] Add screencast support Apr 4, 2023

Add not focused user background color

899b9f0

graszka22 suggested changes Apr 4, 2023

View reviewed changes

Refactor, split into different components

31b8c88

skyman503 added 5 commits April 5, 2023 13:04

Cleanup

0da11bc

Remove not needed type

b219501

fix margin when self screencasting

06ab37f

fix screencast bug

7620acc

remove parameters from callback

66b3aad

skyman503 requested a review from graszka22 April 5, 2023 13:34

skyman503 marked this pull request as ready for review April 5, 2023 13:35

graszka22 suggested changes Apr 5, 2023

View reviewed changes

skyman503 added 2 commits April 5, 2023 19:51

resolve first batch of comments

715ea89

Simplify audio track detection

81ab88d

skyman503 requested a review from graszka22 April 6, 2023 05:48

graszka22 suggested changes Apr 6, 2023

View reviewed changes

skyman503 added 2 commits April 6, 2023 11:11

resolve comments

781c47e

Fix no camera bug

9b12076

skyman503 force-pushed the @skyman503/MV-391-v1 branch from 2f728ab to 9b12076 Compare April 6, 2023 09:46

skyman503 requested a review from graszka22 April 6, 2023 09:49

graszka22 suggested changes Apr 6, 2023

View reviewed changes

Fix crash when focused user exits the call

186f384

skyman503 requested a review from graszka22 April 6, 2023 10:39

Fix no autopin for screencast bug

4828f6c

graszka22 approved these changes Apr 6, 2023

View reviewed changes

Merge branch 'master' into @skyman503/MV-391-v1

447baf7

skyman503 merged commit 5916d14 into master Apr 6, 2023

skyman503 deleted the @skyman503/MV-391-v1 branch April 6, 2023 11:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MV-391] Add screencast support #83

[MV-391] Add screencast support #83

skyman503 commented Apr 4, 2023

graszka22 left a comment •

edited

Loading

graszka22 Apr 4, 2023

skyman503 Apr 5, 2023

skyman503 commented Apr 5, 2023

graszka22 left a comment

graszka22 Apr 5, 2023

skyman503 Apr 6, 2023

graszka22 Apr 5, 2023

skyman503 Apr 6, 2023

graszka22 Apr 5, 2023

skyman503 Apr 6, 2023

graszka22 Apr 6, 2023

skyman503 Apr 6, 2023

graszka22 Apr 6, 2023

skyman503 Apr 6, 2023

skyman503 commented Apr 6, 2023

graszka22 Apr 6, 2023

skyman503 Apr 6, 2023

graszka22 Apr 6, 2023 •

edited

Loading

skyman503 Apr 6, 2023

graszka22 left a comment

[MV-391] Add screencast support #83

[MV-391] Add screencast support #83

Conversation

skyman503 commented Apr 4, 2023

graszka22 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skyman503 commented Apr 5, 2023

graszka22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skyman503 commented Apr 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graszka22 Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graszka22 left a comment

Choose a reason for hiding this comment

graszka22 left a comment •

edited

Loading

graszka22 Apr 6, 2023 •

edited

Loading