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.

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

Hand raise feature #2542

Open
wants to merge 45 commits into
base: livekit
Choose a base branch
from
Open

Hand raise feature #2542

wants to merge 45 commits into from

Conversation

mgcm
Copy link

@mgcm mgcm commented Aug 7, 2024

Initial draft to implement #130

This implementation initially used LiveKit's API that allows for participants to publish data.

It is now using reactions to call membership events and redactions of said event to toggle raising of hands.

Planned features:

  • show order of hand raises (1st, 2nd, etc)
  • add a timer that shows how long the hand is raised
ec-raise-hand.mp4

@mgcm mgcm changed the title Initial support for Hand Raise feature Initial support for Raise Hand feature Aug 7, 2024
@fkwp
Copy link
Contributor

fkwp commented Aug 7, 2024

Nice :-)

This implementation uses LiveKit's API that allows for participants to publish data. I'm interested in discussing if it would make more sense to use extensible matrix events instead.

MatrixRTC is designed to not rely on the actual RTC backend (today it's LiveKit; tomorrow the-next-best-thing) it makes sense to implement those kinds of features with native Matrix primitives.

Since the Raise-Hands feature would benefit a lot from being moderated we could start this journey first with a Reactions feature (👍️, 👏, 🤣, ❤️, ...). And here we could use the Matrix reactions feature or similar.

@mgcm
Copy link
Author

mgcm commented Aug 9, 2024

Since the Raise-Hands feature would benefit a lot from being moderated we could start this journey first with a Reactions feature (👍️, 👏, 🤣, ❤️, ...). And here we could use the Matrix reactions feature or similar.

Thanks for the feedback!

I'm also interested in reactions but not message reactions like in MSC2677 - something more like what Jitsi and Teams consider to be reactions: playful animation overlays (+ sound?) that transmit a ephemeral feeling (indeed like 👍️, 👏, 🤣, ❤️ but for video).

You mention moderation, which I understand could apply to stuff such as muting, spotlighting and lowering hands for other users - I assume getting people off the call can use existing kick / ban membership events as spec'ed.

Under this light, here are a few proposals how this could be done:

Option 1 - A new moderation state event

Since a raised hand is a binary state (either a call member has its hand raised or doesn't), we could introduce a room state event (m.call.moderate) which would have a list of MXIDs which have their hand raised:

{
	"type": "m.call.moderate",
	"sender": "@alice:wonderland.org",
	"content": {
		"raised_hand": [
			"@whiterabbit:wonderland.org",
			"@redqueen:wonderland.org"
		]
	},
	"state_key": "",
	"event_id": "$143273582443PhrSn:example.org",
	"origin_server_ts": 1432735824653,
	"room_id": "!jEsUZKDJdhlrceRyVU:example.org",
	"unsigned": {
		"age": 1234
	}
}

Clients would add / remove their user's MXID to the list and use it to render accordingly. It could be further used to include other moderation features, like:

{
	"type": "m.call.moderate",
	"sender": "@alice:wonderland.org",
	"content": {
		"raised_hand": [
			"@whiterabbit:wonderland.org"
		],
		"hard_mute": [
			"@whiterabbit:wonderland.org",
			"@redqueen:wonderland.org",
			"@madhatter:wonderland.org"
		],
		"spotlight": [
			"@alice:wonderland.org"
		]
	},
	"state_key": "",
	"event_id": "$143273582443PhrSn:example.org",
	"origin_server_ts": 1432735824653,
	"room_id": "!jEsUZKDJdhlrceRyVU:example.org",
	"unsigned": {
		"age": 1234
	}
}

Having an empty state key, means any user with an adequate power level can update this state event. Room creation should make sure call members have the adequate power level to send m.call.moderate state events, so they can raise their hand.

Main issue with this approach is that anyone that is allowed to raise a hand is now also able to interfere with any of these moderation options, which is can be problematic.

Option 2 - Reactions to membership events

A raised hand could be an m.reaction room event that is linked to the m.room.member state event for the user:

{
    "type": "m.reaction",
    "sender": "@alice:wonderland.org",
    "content": {
        "m.relates_to": {
            "rel_type": "m.annotation",
            "event_id": "$event-id-of-m.room.member-for-alice",
            "key": ""
        }
    }
}

Un-raising your hand would be achieved with a redaction. With this approach, only the sending user and room moderators would be able to redact the raised hands.

Main issue with this approach is how to handle users that join the call after these reactions have been sent, potentially being unable to retrieve them from the room timeline?

Option 3 - A new dedicated room state event

Similar to the first option, but used only for raised hands:

{
	"type": "m.call.raised_hands",
	"sender": "@alice:wonderland.org",
	"content": {
		"@whiterabbit:wonderland.org": {
			"order": 1,
			"raised_at_ts": 1432735824653
		},
		"@redqueen:wonderland.org": {
			"order": 2,
			"raised_at_ts": 1432736824653
		}
	},
	"state_key": "",
	"event_id": "$143273582443PhrSn:example.org",
	"origin_server_ts": 1432735824653,
	"room_id": "!jEsUZKDJdhlrceRyVU:example.org",
	"unsigned": {
		"age": 1234
	}
}

This has the same downside of Option 1 (anyone can add or remove himself and others from the list) but how critical is messing up the intended raised hands state by malicious actors? Such manipulation would be easily handled by kicking / banning them.

Anyway, hope this can get the conversation one step further.

@amshakal
Copy link

amshakal commented Sep 2, 2024

Thank you for your work on this feature! I have a few design suggestions that could help simplify its implementation:

  1. Emoji Usage: Instead of using a separate icon for the "Raise Hand" feature, could we consider using the hand emoji (✋) instead?
  2. Integration with Reactions: Would it be possible to integrate the "Raise Hand" feature within the existing reactions menu, allowing users to use the hand emoji as a reaction?

I believe these changes could help streamline the user interface by reducing the number of options available in the bottom action sheet. A quick competitive analysis also showed that similar products often combine these features. What do you think?

Looking forward to your thoughts!

@mgcm
Copy link
Author

mgcm commented Sep 4, 2024

@amshakal Great to hear your feedback. For the time being, my focus will be on the technical implementation of the feature and not worry much about the final UI/UX polish, as that is something that surely Element's Design team will have a strong opinon about.

@mgcm
Copy link
Author

mgcm commented Sep 4, 2024

So I'm drafting an implementation using annotations and reactions (Option 2) and was wondering if the reaction should be to the m.room.member event or to the m.call.member event.

Having it linked to the room member state event would open this feature up to non-call use cases - I've read about people being interested in having raised hands in text chat rooms... but maybe thats too much forward thinking?

Either way, it's a minor implementation detail.

@CLAassistant
Copy link

CLAassistant commented Sep 6, 2024

CLA assistant check
All committers have signed the CLA.

@mgcm mgcm force-pushed the raise-hand-button branch 2 times, most recently from 488fd43 to e84b0a4 Compare September 8, 2024 00:57
@mgcm
Copy link
Author

mgcm commented Sep 8, 2024

Updating to a refactored version that uses reactions to call membership events. It works :)

@amshakal I wasn't aware you were actually working for Element Design 🤦 I'm sure we can get something done along what you were suggesting.

I won't be focusing yet on the reactions feature but we can look at the raise hand emoji suggestion you mentioned. I'm fine with replacing the current SVG with the hand emoji (✋). Let's try that.

@mgcm
Copy link
Author

mgcm commented Sep 8, 2024

Here's two screenshots of replacing with the hand emoji. I think this needs some tweaking on:

  1. the bottom menu button active state: when the hand is raised, the contrast seems to be not optimal
  2. the top left corner raised hand indicator per each call member: it's using the --cpd-color-icon-secondary as the background - should it be using another design token?
hand-not-raised hand-raised

src/room/useRaisedHands.tsx Outdated Show resolved Hide resolved
@toger5
Copy link
Contributor

toger5 commented Sep 9, 2024

Main issue with this approach is how to handle users that join the call after these reactions have been sent, potentially being unable to retrieve them from the room timeline?

The relations api should solve this. joining a call gives you access to the room state and you can ask for the related raise hand events (even through the widget api)

@mgcm
Copy link
Author

mgcm commented Sep 10, 2024

The relations api should solve this. joining a call gives you access to the room state and you can ask for the related raise hand events (even through the widget api)

@toger5 it works 👯

I've tested this in Widget mode inside Element Web and seems to work as well, although I'm wondering how as I don't see an implementation for relations in the RoomWidgetClient class from the JS SDK...

Also, on mobile, it loads and shows reactions from other non-mobile clients but sending isn't working.. wondering how to debug that...

@toger5
Copy link
Contributor

toger5 commented Sep 10, 2024

I've tested this in Widget mode inside Element Web and seems to work as well, although I'm wondering how as I don't see an implementation for relations in the RoomWidgetClient class from the JS SDK...

That is surprising indeed. I also expected that to be a required change (to update the js-sdk matroska widget client to forward relations requests to the host client)
But the relations would only be needed on load right? So a widget still gets the up to date state by just subscribing to the raise hand room events?

Also, on mobile, it loads and shows reactions from other non-mobile clients but sending isn't working.. wondering how to debug that...

Probably you dont need to do too much debugging here. On mobile we are very strict with the permissions. The raise hand event most likely is just not part of that yet. I am wondering how you can receive them though.

src/room/InCallView.tsx Outdated Show resolved Hide resolved
@mgcm
Copy link
Author

mgcm commented Sep 10, 2024

That is surprising indeed. I also expected that to be a required change (to update the js-sdk matroska widget client to forward relations requests to the host client) But the relations would only be needed on load right? So a widget still gets the up to date state by just subscribing to the raise hand room events?

Yes, relations are queried only on the call's session first load.

Probably you dont need to do too much debugging here. On mobile we are very strict with the permissions. The raise hand event most likely is just not part of that yet. I am wondering how you can receive them though.

🤔

@mgcm mgcm force-pushed the raise-hand-button branch 2 times, most recently from 4e13c66 to 21d6c8d Compare September 10, 2024 23:04
src/tile/MediaView.tsx Outdated Show resolved Hide resolved
Copy link
Member

@robintown robintown left a comment

Choose a reason for hiding this comment

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

Hi as I mentioned earlier today, I'm really interested in seeing this reaction data eventually become part of the view models. (Maybe a natural time for that refactor would be when we start wanting to promote participants with raised hands to the start of the grid.) But for a first iteration here are the comments I had:

@@ -168,6 +174,11 @@ export const InCallView: FC<InCallViewProps> = ({
connState,
onShareClick,
}) => {
const { supportsReactions, raisedHands } = useReactions();
const raisedHandCount = Object.keys(raisedHands).length;
const [previousRaisedHandCount, setPreviousRaisedHandCount] =
Copy link
Member

Choose a reason for hiding this comment

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

We have a usePrevious hook which might work here (and cause fewer spurious renders than tracking this in component state)

Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find a usePrevious hook, but useDeferredValue seemed to do the trick.

Comment on lines 93 to 95
.raisedHandBorder {
border: var(--cpd-space-1x) solid var(--cpd-color-yellow-1200);
}
Copy link
Member

Choose a reason for hiding this comment

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

If the border were added in GridTile rather than the MediaView component, we could prevent the other tile borders from rendering on top of it (make it exclusive with the speaking and hover borders, that is). We also don't mean to be rendering borders on top of MediaViews shown in a SpotlightTile, I assume.

Copy link
Member

Choose a reason for hiding this comment

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

Done. Additionally followed the same gradient style that speaking does 👍

src/useReactions.tsx Outdated Show resolved Hide resolved
src/useReactions.tsx Outdated Show resolved Hide resolved
src/useReactions.tsx Outdated Show resolved Hide resolved
src/button/RaisedHandToggleButton.tsx Outdated Show resolved Hide resolved
src/button/RaisedHandToggleButton.tsx Outdated Show resolved Hide resolved
src/button/RaisedHandToggleButton.tsx Outdated Show resolved Hide resolved
src/reactions/RaisedHandIndicator.tsx Outdated Show resolved Hide resolved
src/useReactions.test.tsx Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants