-
Notifications
You must be signed in to change notification settings - Fork 4
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
Agora broadcasting #74
base: main
Are you sure you want to change the base?
Conversation
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.
Woohoo! Thanks for submitting this.
I'd be happy to get in there and resolve the merge conflicts soon, unless you're already on it.
That would be great! TY! |
b3c7776
to
1a31236
Compare
ac21bd7
to
0275c9f
Compare
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.
Looks really good overall!
Most of this feedback is for readability, though we definitely should factor out the dart:io
imports.
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.
@ChaAbby btw, let me know if I can help out with your checkout of this branch—I know that having someone else force-push to it can make things sort of wonky
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.
Thanks for the updates, and also thanks for your patience!
I just have a couple of concerns about the ThcUser
class structure, and I guess we still gotta take care of those dart:io
imports too.
if (isAudioEnabled != isAudioEnabled) 'isAudioEnabled': isAudioEnabled, | ||
if (isVideoEnabled != isVideoEnabled) 'isVideoEnabled': isVideoEnabled, | ||
if (view != view) 'view' : view, |
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.
A couple of questions here:
- It seems as though
if (x != x)
will always return false, do you know what the intended behavior is here? - I noticed that
view
has a theWidget?
type, and I'm not exactly sure how we're intending to represent it in JSON format.
(I also haven't seen anywhere that sets a value forview
.)
bool? isAudioEnabled; | ||
bool? isVideoEnabled; |
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.
Unfortunately, adding these non-final
values to an @immutable
class is triggering some static analysis problems.
In my opinion, it would make the most sense to store these values in LocalStorage
. What are your thoughts?
With this PR.
Directors can start live streams and participants can join the channel.
I am currently not seeing the video on my end -- with the app simulator on my mac it is not compatible with the laptop camera, so not sure if I haven't gotten it set up correctly or its not showing up because of that.