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

Show subtitles when Jigasi sends transcription results in JSON #1914

Merged
merged 14 commits into from Jul 17, 2018

apply feedback on initial PR

  • Loading branch information...
pvgupta24 authored and nikvaessen committed May 31, 2018
commit 48e23fee623c587c18ef76bf0f7033bbec7b9f42
View
@@ -109,6 +109,7 @@ import {
} from './react/features/overlay';
import { setSharedVideoStatus } from './react/features/shared-video';
import { isButtonEnabled } from './react/features/toolbox';
import { endpointMessageReceived } from './react/features/transcription';
const logger = require('jitsi-meet-logger').getLogger(__filename);
@@ -1899,6 +1900,12 @@ export default {
}
);
room.on(
JitsiConferenceEvents.ENDPOINT_MESSAGE_RECEIVED,
(...args) => {
APP.store.dispatch(endpointMessageReceived(room, ...args));
});
room.on(
JitsiConferenceEvents.LOCK_STATE_CHANGED,
(...args) => APP.store.dispatch(lockStateChanged(room, ...args)));
@@ -0,0 +1,14 @@
.transcription-subtitles{
position: absolute;
bottom: 10%;
width: 100%;
z-index: 2;

This comment has been minimized.

@virtuacoplenny

virtuacoplenny Jun 1, 2018

Contributor

You should try to use the css variables for z-index if possible. z-index get out of control and at least having only a handful prevents random z-indexes of...55 and 102 and 10000.

@virtuacoplenny

virtuacoplenny Jun 1, 2018

Contributor

You should try to use the css variables for z-index if possible. z-index get out of control and at least having only a handful prevents random z-indexes of...55 and 102 and 10000.

This comment has been minimized.

@pvgupta24

pvgupta24 Jun 1, 2018

Contributor

👍

@pvgupta24

pvgupta24 Jun 1, 2018

Contributor

👍

font-weight: 1000;
font-size: 16px;
// color: '#000000';

This comment has been minimized.

@virtuacoplenny

virtuacoplenny Jun 1, 2018

Contributor

This can probably be removed. Not sure it's needed.

@virtuacoplenny

virtuacoplenny Jun 1, 2018

Contributor

This can probably be removed. Not sure it's needed.

This comment has been minimized.

@pvgupta24

pvgupta24 Jun 1, 2018

Contributor

👍

@pvgupta24

pvgupta24 Jun 1, 2018

Contributor

👍

opacity: 0.80;
text-shadow: 0px 0px 1px rgba(0,0,0,0.3),
0px 1px 1px rgba(0,0,0,0.3),
1px 0px 1px rgba(0,0,0,0.3),
0px 0px 1px rgba(0,0,0,0.3);
}
View
@@ -72,5 +72,5 @@
@import 'modals/invite/add-people';
@import 'vertical_filmstrip_overrides';
@import 'deep-linking/main';
@import 'transcription-subtitles';
/* Modules END */
@@ -51,7 +51,7 @@ import {
_addLocalTracksToConference,
sendLocalParticipant
} from './functions';
import { endpointMessageReceived } from '../../transcription';
import type { Dispatch } from 'redux';
const logger = require('jitsi-meet-logger').getLogger(__filename);
@@ -87,6 +87,11 @@ function _addConferenceListeners(conference, dispatch) {
JitsiConferenceEvents.LOCK_STATE_CHANGED,
(...args) => dispatch(lockStateChanged(conference, ...args)));
// Dispatches into features/transcription follow:

This comment has been minimized.

@virtuacoplenny

virtuacoplenny Jun 1, 2018

Contributor

A comment like this probably isn't accomplishing much. If anything it reveals a bit of implementation that isn't necessary to leak.

@virtuacoplenny

virtuacoplenny Jun 1, 2018

Contributor

A comment like this probably isn't accomplishing much. If anything it reveals a bit of implementation that isn't necessary to leak.

conference.on(
JitsiConferenceEvents.ENDPOINT_MESSAGE_RECEIVED,
(...args) => dispatch(endpointMessageReceived(conference, ...args)));

This comment has been minimized.

@virtuacoplenny

virtuacoplenny Jun 1, 2018

Contributor

Edit: okay I see now, this is to update the redux store for mobile clients even though mobile doesn't show the subtitles.

So the flow to get here is
-> react/features/base/conference/middleware CONNECTION_ESTABLISHED
-> react/features/base/conference/middleware#_connectionEstablished
-> if typeof APP === 'undefined'
-> react/features/base/conference/actions#createConference
-> react/features/base/conference/actions#_addConferenceListeners
-> conference.on(...endpointMessageReceived(...))

Are you certain _addConferenceListeners gets called on web? On web, the typeof APP will be "object," no "undefined." Do you want it called on web? Do you want to avoid having it called on mobile for now?

@virtuacoplenny

virtuacoplenny Jun 1, 2018

Contributor

Edit: okay I see now, this is to update the redux store for mobile clients even though mobile doesn't show the subtitles.

So the flow to get here is
-> react/features/base/conference/middleware CONNECTION_ESTABLISHED
-> react/features/base/conference/middleware#_connectionEstablished
-> if typeof APP === 'undefined'
-> react/features/base/conference/actions#createConference
-> react/features/base/conference/actions#_addConferenceListeners
-> conference.on(...endpointMessageReceived(...))

Are you certain _addConferenceListeners gets called on web? On web, the typeof APP will be "object," no "undefined." Do you want it called on web? Do you want to avoid having it called on mobile for now?

This comment has been minimized.

@pvgupta24

pvgupta24 Jun 1, 2018

Contributor

Yes 👍

@pvgupta24

pvgupta24 Jun 1, 2018

Contributor

Yes 👍

This comment has been minimized.

@virtuacoplenny

virtuacoplenny Jun 5, 2018

Contributor

I don't think it's worthwhile to leave in commented code like this. Someone will wonder why it was even left in and that might lead to some confusion. The change is simple enough that it can be redone and it's already consistent with the inconsistent way in which web/native add conference listeners.

@virtuacoplenny

virtuacoplenny Jun 5, 2018

Contributor

I don't think it's worthwhile to leave in commented code like this. Someone will wonder why it was even left in and that might lead to some confusion. The change is simple enough that it can be redone and it's already consistent with the inconsistent way in which web/native add conference listeners.

This comment has been minimized.

@pvgupta24

pvgupta24 Jun 6, 2018

Contributor

Yeah, I'll remove it then.

@pvgupta24

pvgupta24 Jun 6, 2018

Contributor

Yeah, I'll remove it then.

// Dispatches into features/base/media follow:
conference.on(
@@ -0,0 +1,48 @@
/**
* The type of (redux) action which indicates that an end point message

This comment has been minimized.

@virtuacoplenny

virtuacoplenny Jun 1, 2018

Contributor

end point -> endpoint

@virtuacoplenny

virtuacoplenny Jun 1, 2018

Contributor

end point -> endpoint

* sent by another participant to the data channel is received.
*
* {
* type: ENDPOINT_MESSAGE_RECEIVED,
* conference: JitsiConference,

This comment has been minimized.

@virtuacoplenny

virtuacoplenny Jun 12, 2018

Contributor

The danger of comments. You should remove this reference to conference.

@virtuacoplenny

virtuacoplenny Jun 12, 2018

Contributor

The danger of comments. You should remove this reference to conference.

* participant: Object,
* p: Object
* }
*/
export const ENDPOINT_MESSAGE_RECEIVED = Symbol('ENDPOINT_MESSAGE_RECEIVED');
/**
* The type of (redux) action which indicates that a transcript with
* a new message_id is received.
*
* {
* type: ADD_TRANSCRIPT_MESSAGE,
* transcriptMessageID: string,
* participantName: string
* }
*/
export const ADD_TRANSCRIPT_MESSAGE = Symbol('ADD_TRANSCRIPT_MESSAGE');
/**
* The type of (redux) action which indicates that a transcript with an
* existing message_id to be updated is received.
* is received.

This comment has been minimized.

@virtuacoplenny

virtuacoplenny Jun 12, 2018

Contributor

A little typo here with the comment.

@virtuacoplenny

virtuacoplenny Jun 12, 2018

Contributor

A little typo here with the comment.

*
* {
* type: UPDATE_TRANSCRIPT_MESSAGE,
* transcriptMessageID: string,
* newTranscriptMessage: Object
* }
*/
export const UPDATE_TRANSCRIPT_MESSAGE = Symbol('UPDATE_TRANSCRIPT_MESSAGE');
/**
* The type of (redux) action which indicates that an existing transcript
* has to be removed from the state.
*
* {
* type: REMOVE_TRANSCRIPT_MESSAGE,
* transciptMessageID: string,
* }
*/
export const REMOVE_TRANSCRIPT_MESSAGE = Symbol('REMOVE_TRANSCRIPT_MESSAGE');
@@ -0,0 +1,89 @@
// @flow
import {
ENDPOINT_MESSAGE_RECEIVED,
ADD_TRANSCRIPT_MESSAGE,
UPDATE_TRANSCRIPT_MESSAGE,
REMOVE_TRANSCRIPT_MESSAGE
} from './actionTypes';
/**
* Signals that a participant sent an endpoint message on the data channel.
*
* @param {JitsiConference} conference - The JitsiConference which had its lock
* state changed.
* @param {Object} participant - The participant details sending the message.
* @param {Object} p - The payload carried in the message.
* @returns {{
* type: ENDPOINT_MESSAGE_RECEIVED,
* conference: JitsiConference,
* participant: Object,
* p: Object
* }}
*/
export function endpointMessageReceived(
conference: Object, participant: Object, p: Object) {
return {
type: ENDPOINT_MESSAGE_RECEIVED,
conference,

This comment has been minimized.

@virtuacoplenny

virtuacoplenny Jun 5, 2018

Contributor

How is conference used? I'm missing the place where it's called.

@virtuacoplenny

virtuacoplenny Jun 5, 2018

Contributor

How is conference used? I'm missing the place where it's called.

This comment has been minimized.

@pvgupta24

pvgupta24 Jun 6, 2018

Contributor

No, its not being used anymore. 👍

@pvgupta24

pvgupta24 Jun 6, 2018

Contributor

No, its not being used anymore. 👍

participant,
p

This comment has been minimized.

@virtuacoplenny

virtuacoplenny Jun 1, 2018

Contributor

Could "p" be passed as a more descriptive name?

@virtuacoplenny

virtuacoplenny Jun 1, 2018

Contributor

Could "p" be passed as a more descriptive name?

};
}
/**
* Signals that a transcript with a new message_id is received.
*
* @param {string} transcriptMessageID - The new message_id.
* @param {string} participantName - The payload carried in the message.
* @returns {{
* type: ADD_TRANSCRIPT_MESSAGE,
* transcriptMessageID: string,
* participantName: string
* }}
*/
export function addTranscriptMessage(transcriptMessageID: string,
participantName: string) {
return {
type: ADD_TRANSCRIPT_MESSAGE,
transcriptMessageID,
participantName
};
}
/**
* Signals that a transcript with an existing message_id to be updated
* is received.
*
* @param {string} transcriptMessageID -The transcript message_id to be updated.
* @param {Object} newTranscriptMessage - The updated transcript message.
* @returns {{
* type: UPDATE_TRANSCRIPT_MESSAGE,
* transcriptMessageID: string,
* newTranscriptMessage: Object
* }}
*/
export function updateTranscriptMessage(transcriptMessageID: string,
newTranscriptMessage: Object) {
return {
type: UPDATE_TRANSCRIPT_MESSAGE,
transcriptMessageID,
newTranscriptMessage
};
}
/**
* Signals that a transcript has to be removed from the state.
*
* @param {string} transcriptMessageID - The message_id to be removed.
* @returns {{
* type: REMOVE_TRANSCRIPT_MESSAGE,
* transcriptMessageID: string,
* }}
*/
export function removeTranscriptMessage(transcriptMessageID: string) {
return {
type: REMOVE_TRANSCRIPT_MESSAGE,
transcriptMessageID
};
}
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.