-
Notifications
You must be signed in to change notification settings - Fork 105
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
Add DeviceMute widget action io.element.device_mute
.
#2482
Conversation
d6e978a
to
537d93a
Compare
This allows to send mute requests ("toWidget") and get the current mute state as a response. And it will update the client about each change of mute states.
537d93a
to
71eb9ae
Compare
src/room/MuteStates.ts
Outdated
); | ||
}; | ||
} | ||
}, [audio, video]); |
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 might need an explanation on what this is doing: whenever the audio or video mute state changes, we add a new listener for the device mute widget action? The listener presumably also fires (once?) to set the mute states and reply?
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.
To my understanding the audio and video elements are not changing instead they mutate. So whenever they are set we connect the listener and when they are unset we remove it.
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 sure it's that simple. Why not just use a callback and then the problem goes away? Also it will make the code less nested.
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 updated this. So it is no creating the useCallback and recomputes the on/off when the callback changes.
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 general, comments would be helpful: I say this as someone who's knowledge of this codebase is now a little rusty :)
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 good as far as I can see, comments make it much clearer, thanks!
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.
lgtm
This allows to send mute requests ("toWidget") and get the current mute state as a response.
And it will update the client about each change of mute states.