Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Added "Raise Hand" feature #4569

Merged
merged 20 commits into from Dec 16, 2020
Merged

Added "Raise Hand" feature #4569

merged 20 commits into from Dec 16, 2020

Conversation

PVince81
Copy link
Member

@PVince81 PVince81 commented Nov 9, 2020 •
edited

Fixes #4562

Requires

Tasks

  • Added hand and hand-off icons (need better fitting ones later)
  • Added "raise-hand" capability
  • State changes for raising/lowering hand is broadcast to other participants
  • Display notification (toast) to moderators participants when someone raised a hand
  • Inform newly joined participants of current raised hands => won't, see Added "Raise Hand" feature #4569 (comment)
  • Show hand raised icon to sidebar participant list (when raised)
  • Add chat message "XYZ raised their hand" in the history ? Only for call people or everyone ? (if possible) => probably not, would need backend state, and is likely not that useful
  • Disable hand raising in 1-1 calls (or if participants count <= 2)
    • => tried it, doesn't feel natural. and one could always be presenting something
    • one can also do a presentation to only one participant
    • the hand action is anyway now in the three dots menu, so doesn't disturb any more visually
    • implementing this means we need to automatically lower all hands when participant count is less than 3
    • sometimes one might want to raise the hand before the presenter joins, or the presenter just lost connection and is re-joining
  • Sort participant list by "raised hand time" first, and then non-raised-hand participants
  • Hide local hand button if there is no local stream (as we currently can only broadcast when there are streams) => not needed, there will be a fix for this soon
  • raising hand from SIP: 🤙 SIP Bridge #4496 (comment)
  • Test with internal signaling
  • Test with HPB
  • Test with phone users (SIP)
  • Tweak UX design

Issues

  • broadcasting is unreliable (tested with internal signaling)

    • only Firefox seems to receive messages
    • not sure if the way I send the message is correct and whether we need to pass along the peer id and verify it (if available, as peers require streams AFAIK) @danxuliu I'll need your advice here

  • clicking "raise hand" doesn't toggle the hand icon

  • current icon design not fitting / not consistent

The hand is striked out differently. This is the do not touch icon from Material Design. obsolete, see newer screenshots below

  • clicking directly on the pixels of the three dots of the new menu doesn't do anything in some layouts, but clicking the border does... in any case it seems the actions menu has some issues with hover, cursor, etc...

  • bug: when leaving with hand raised, rejoining should have it lowered again. Probably LocalMediaModel is not cleared when leaving (approach might be obsolete with Added "Raise Hand" feature #4569 (comment))

  • use store value in video bottom bar vue? it seems the unwatcher has a delay compared to the model attribute

  • fix hand icon alignment and spacing in VideoBottomBar

image

@PVince81 PVince81 added this to the 💚 Next Major (21) milestone Nov 9, 2020
@PVince81 PVince81 self-assigned this Nov 9, 2020
@PVince81 PVince81 mentioned this pull request Nov 9, 2020
Closed
@fancycode
Copy link
Member

Would be great to show that also in the participant list. For phone-only users there won't be a video component and they should be able to raise their hand, too.

@PVince81
Copy link
Member Author

PVince81 commented Nov 9, 2020

Would be great to show that also in the participant list

Indeed, forgot to add it. Added as checkbox now.

@PVince81
Copy link
Member Author

PVince81 commented Nov 9, 2020

silly idea: animate the hand icon, making it wave (CSS 3 rotate / skew), while the hand is raised

@PVince81
Copy link
Member Author

I've changed the message type from "control" to "raiseHand" and now the broadcasting work.

Now need to find out why the views are not updating correctly.

@PVince81
Copy link
Member Author

Okay, the basics work now (tested with internal signaling):

  • clicking hand icon will toggle it
  • peers will see a hand icon on the left of the display name whenever a remote hand is raised

marcoambrosini
marcoambrosini reviewed Nov 11, 2020
View reviewed changes
Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this action should be buried in 3-dot menu in the bottom right corner of the localvideo, together with a link to the global audio and video settings.

@PVince81
Copy link
Member Author

adding a new three dots menu in the local controls sounds like an idea.

I've seen variants where additionally to raising hand it was also possible to set a mood instead of the hand, like "smiling", "unhappy", "confused", "thumbs up", "thumbs down" to silently signal something to the presenter. Having a menu would make room for those.

@marcoambrosini
Copy link
Member

marcoambrosini commented Nov 11, 2020 •
edited

As per the icon, this would do

https://materialdesignicons.com/icon/hand-right

We don't need a "hand down" icon, we can just display this one when the hand is raised.

EDIT: hmm, the link doesn't seem to work.. Anyway the icon is hand-right

@marcoambrosini
Copy link
Member

adding a new three dots menu in the local controls sounds like an idea.

Plus this is a borderline niche feature, so having it explicitly displayed by default could be distracting

@nickvergessen
Copy link
Member

Inform newly joined participants of current raised hands

wouldnt do that, as this requires a global state or something, instead of being a signaling message. e.g. with sip dialin this wont work

@PVince81
Copy link
Member Author

Inform newly joined participants of current raised hands

wouldnt do that, as this requires a global state or something, instead of being a signaling message. e.g. with sip dialin this wont work

We already do that for the muted state so we can show the icon. But it requires every participant to detect whenever a new person joined and then send them their state. Could get messy with a lot of participants.

@PVince81
Copy link
Member Author

I've moved the hand action into a new three dots menu:
image

When the hand is raised, there will be an extra button in the controls to show you so, and clicking it will lower the hand again and make it disappear:
image

@PVince81
Copy link
Member Author

PVince81 commented Nov 26, 2020 •
edited

  • BUG: hand icon shortcut "h" key doesn't do anything, probably because it's in the menu
    • should we debounce it as well to avoid abuse ?

@fancycode
Copy link
Member

Can we somehow reuse the flags introduced for "talking" in the SIP integration to signal the "raised hand" state?
#4496 (comment)

Right now it seems to use datachannels which we try to get rid of (#397).

@PVince81
Copy link
Member Author

PVince81 commented Nov 27, 2020 •
edited

Note that having a boolean flag will make it difficult to add other symbols like thumbs up / down in the future. But maybe we can rework this once we walk that path.

@PVince81
Copy link
Member Author

PVince81 commented Nov 27, 2020 •
edited

  • raise enhancement: ability to follow a participant after clicking on them in the participant list.
    • would be useful so that when someone raises hand, people could click on that person to make their video visible

=> raised here #4755

@PVince81
Copy link
Member Author

PVince81 commented Nov 27, 2020 •
edited

hand icon is now displayed in the participant list as per 9c1db58:

here we see Bob raised their hand, left side is admin and right side is Bob:
Screenshot_20201127_115503

  • consolidate hand icons ? (<Hand> vs "icon-hand")

@PVince81
Show notification whenever someone raises hand in a call
aa5dfed 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Display hand icon in participant list
64ae8e9 
- added call store attribute for storing the current hand raised state,
  to expose it from the webrtc models to the participant list
- call icon is replaced with a hand when a hand is raised, also for
  oneself
- when leaving a call, clear raised hands from the store
- when a user leaves a call, clear that user's hand raised state

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Map raise hand SIP flag to model attribute
208570a 
Set "raisedHand" in call participant model based on the matching SIP
flag.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Replaced raise hand icon with material design icons
a0375f5 
Now using the "Hand" material design icon everywhere for the "raise
hand" feature.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Adjust hand icon style
9c9cd7c 
Remove fill color to make it work with light and dark themes.
Adjusted size to 16 as per nextcloud-vue recommendation.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Fix hand icon size in video button
174ebeb 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Added wave hand animation
59f9ac0 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Fix hand icon styles in dark/light mode
4ea7479 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Fix raise hand keyboard shortcut + focus style
edacef6 
Now using a button that is visually hidden to catch the short key. That
button is the one that will appear only when the hand is raised.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Changed shortcut for raise hand from H to R
fc5dda4 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Fix raise hand after review
f103806 
Remove notification of action.
Fix alignment a bit.
Fix raise hand keyboard shortcut.
Fix lower hand button to not trigger an action on the parent.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Align remote video hand icon vertically
c61a944 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Fix rendering delay for raised hand icon
6810c12 
Fixed setting or deleting the raised hand state in the store using Vue
methods so that the participant list can re-render immediately.

Before this fix it did appear but with a delay, where the re-render was
triggered by some other change.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Reset hand state when starting/leaving call
3fc0bdd 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Remove silly hand animation
be769ae 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Copy link
Member Author

I've removed the sorting thing and rebased.

Also adjusted the icon with decorative and empty title

@nickvergessen

This comment has been minimized.

Sign in to view
@PVince81
Make hand icon decorative
77d368e 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Add raise hand to capabilities test
b4f6d70 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@nickvergessen nickvergessen merged commit 79baea8 into master Dec 16, 2020
@nickvergessen nickvergessen deleted the feature/4562/raise-hand branch December 16, 2020 09:56
@PVince81 PVince81 mentioned this pull request Dec 16, 2020
Closed
@nickvergessen nickvergessen mentioned this pull request Dec 18, 2020
Merged
This was referenced Feb 19, 2021
Merged
Merged
This was referenced Mar 16, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@danxuliu danxuliu Awaiting requested review from danxuliu

@marcoambrosini marcoambrosini Awaiting requested review from marcoambrosini

Assignees

@PVince81 PVince81

Labels
3. to review enhancement feature: call 📹 Voice and video calls
Projects
None yet
Milestone
v11.0.0-alpha.1
Development

Successfully merging this pull request may close these issues.

Raise hand button
4 participants
@PVince81 @fancycode @marcoambrosini @nickvergessen