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

[WEB] add UI for transcription #3213

Merged
merged 11 commits into from Jul 26, 2018

Conversation

Projects
None yet
3 participants
@nikvaessen
Contributor

nikvaessen commented Jun 29, 2018

This adds a button to invite and kick the transcriber, as well as a label indicating a transcriber is currently in the room.

It does not currently play a sound when the transcriber joins or leaves, and it nothing is currently displayed on mobile.

@virtuacoplenny virtuacoplenny self-requested a review Jul 2, 2018

Show outdated Hide outdated react/features/toolbox/components/web/Toolbox.js
Show outdated Hide outdated react/features/transcribing/components/TranscribingLabel.web.js
Show outdated Hide outdated react/features/transcribing/middleware.js
Show outdated Hide outdated react/features/transcribing/middleware.js
&& action.displayName === TRANSCRIBER_DISPLAY_NAME) {
store.dispatch(transcriberJoined(action.id));
} else {
store.dispatch(potentialTranscriberJoined(action.id));

This comment has been minimized.

@virtuacoplenny

virtuacoplenny Jul 2, 2018

Contributor

What conditions cause potentialTranscriberJoined to be needed?

@virtuacoplenny

virtuacoplenny Jul 2, 2018

Contributor

What conditions cause potentialTranscriberJoined to be needed?

...state,
potentialTranscriberJIDs:
[ action.transcriberJID ]
.concat(state.potentialTranscriberJIDs)

This comment has been minimized.

@virtuacoplenny

virtuacoplenny Jul 2, 2018

Contributor

What's the scenario in which there can be more than 1 potential subscriber jid? Is it possible for multiple people to start transcribing?

@virtuacoplenny

virtuacoplenny Jul 2, 2018

Contributor

What's the scenario in which there can be more than 1 potential subscriber jid? Is it possible for multiple people to start transcribing?

This comment has been minimized.

@nikvaessen

nikvaessen Jul 2, 2018

Contributor

Because Jibri and Jigasi will both use the recorder domain (hidden domain), both Jibri and Jigasi joining the conference will result in a HIDDEN_PARTICIPANT_JOINED event. We check for Jigasi instance by looking for the DisplayName, which should be Transcriber, but the presence causing the event to be emitted does not yet contain the displayname. Thus I store the potential JID's and only start transcribing when the JID is matched to the correct DisplayName

@nikvaessen

nikvaessen Jul 2, 2018

Contributor

Because Jibri and Jigasi will both use the recorder domain (hidden domain), both Jibri and Jigasi joining the conference will result in a HIDDEN_PARTICIPANT_JOINED event. We check for Jigasi instance by looking for the DisplayName, which should be Transcriber, but the presence causing the event to be emitted does not yet contain the displayname. Thus I store the potential JID's and only start transcribing when the JID is matched to the correct DisplayName

@nikvaessen

This comment has been minimized.

Show comment
Hide comment
@nikvaessen

nikvaessen Jul 3, 2018

Contributor

How could I test if the analytic event is working?

Contributor

nikvaessen commented Jul 3, 2018

How could I test if the analytic event is working?

if (!(isDialing || isTranscribing)) {
store.dispatch(showPendingTranscribingNotification());
conference.room.dial(TRANSCRIBER_DIAL_COMMAND).catch(

This comment has been minimized.

@virtuacoplenny

virtuacoplenny Jul 3, 2018

Contributor

.catch takes in one function, which is the error handler. It's sugar over .then(undefined, errorCallback).

@virtuacoplenny

virtuacoplenny Jul 3, 2018

Contributor

.catch takes in one function, which is the error handler. It's sugar over .then(undefined, errorCallback).

@virtuacoplenny

This comment has been minimized.

Show comment
Hide comment
@virtuacoplenny

virtuacoplenny Jul 3, 2018

Contributor

How could I test if the analytic event is working?

When I remember to test it I use the dev proxy server pointed to beta and check if AnalyticsAdapter#sendEvent sends the information I expect. @bgrozev or @hristoterezov very likely know where the events are going and how to access them.

Contributor

virtuacoplenny commented Jul 3, 2018

How could I test if the analytic event is working?

When I remember to test it I use the dev proxy server pointed to beta and check if AnalyticsAdapter#sendEvent sends the information I expect. @bgrozev or @hristoterezov very likely know where the events are going and how to access them.

@nikvaessen

This comment has been minimized.

Show comment
Hide comment
@nikvaessen

nikvaessen Jul 5, 2018

Contributor

Event on button press worked on Splunk.

Contributor

nikvaessen commented Jul 5, 2018

Event on button press worked on Splunk.

@@ -75,6 +75,8 @@ import {
getAvatarURLByParticipantId,
getLocalParticipant,
getParticipantById,
hiddenParticipantJoined,

This comment has been minimized.

@saghul

saghul Jul 5, 2018

Member

Drive-by observation: we should not need these. A participant can have a hidden property if that's needed. Or alternatively we could filter transcribers before dispatching the participantJoined action.

@saghul

saghul Jul 5, 2018

Member

Drive-by observation: we should not need these. A participant can have a hidden property if that's needed. Or alternatively we could filter transcribers before dispatching the participantJoined action.

This comment has been minimized.

@nikvaessen

nikvaessen Jul 5, 2018

Contributor

I disagree; currently a PARTICIPANT_JOINED event triggers whatever logic is required for a new participant, such as displaying it in the filmstrip. All this code should then first check if it's not actually a hidden participant?

@nikvaessen

nikvaessen Jul 5, 2018

Contributor

I disagree; currently a PARTICIPANT_JOINED event triggers whatever logic is required for a new participant, such as displaying it in the filmstrip. All this code should then first check if it's not actually a hidden participant?

This comment has been minimized.

@nikvaessen

nikvaessen Jul 5, 2018

Contributor

And we can't filter for transcriber because the displayname will not be set until a presence which comes later...

@nikvaessen

nikvaessen Jul 5, 2018

Contributor

And we can't filter for transcriber because the displayname will not be set until a presence which comes later...

@nikvaessen

This comment has been minimized.

Show comment
Hide comment
@nikvaessen

nikvaessen Jul 24, 2018

Contributor

Force pushed to the PR... messed up with fixing a merge conflict.

Contributor

nikvaessen commented Jul 24, 2018

Force pushed to the PR... messed up with fixing a merge conflict.

Merge remote-tracking branch 'upstream/master' into pr_ui_transcription
# Conflicts:
#	fonts/jitsi.eot
#	fonts/jitsi.svg
#	fonts/jitsi.ttf
#	fonts/jitsi.woff
#	fonts/selection.json
#	react/features/base/font-icons/jitsi.json

@virtuacoplenny virtuacoplenny merged commit b8daf0a into jitsi:master Jul 26, 2018

1 check passed

default 775 tests run, 634 skipped, 0 failed.
Details

@nikvaessen nikvaessen deleted the nikvaessen:pr_ui_transcription branch Jul 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment