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

Conversation

Projects
None yet
5 participants
@nikvaessen
Contributor

nikvaessen commented Aug 21, 2017

@virtuacoplenny virtuacoplenny self-requested a review May 31, 2018

@virtuacoplenny

This comment has been minimized.

Show comment
Hide comment
@virtuacoplenny

virtuacoplenny May 31, 2018

Contributor

Jenkins this one is ok to test.

Contributor

virtuacoplenny commented May 31, 2018

Jenkins this one is ok to test.

Show outdated Hide outdated react/features/transcription/reducer.js
Show outdated Hide outdated react/features/transcription/reducer.js
Show outdated Hide outdated react/features/transcription/reducer.js
Show outdated Hide outdated react/features/transcription/functions.js
Show outdated Hide outdated react/features/transcription/functions.js
Show outdated Hide outdated react/features/transcription/actionTypes.js
Show outdated Hide outdated react/features/transcription/actions.js
Show outdated Hide outdated react/features/transcription/middleware.js
Show outdated Hide outdated react/features/transcription/reducer.js
Show outdated Hide outdated react/features/transcription/middleware.js

pvgupta24 and others added some commits Jun 3, 2018

Merge pull request #3 from pvgupta24/transcriptions
Changes Object to Map, css fixes and alphabetic ordering.

pvgupta24 and others added some commits Jun 6, 2018

Merge pull request #4 from pvgupta24/transcriptions
Send Map of transcriptMessages as prop to Component
@pvgupta24

This comment has been minimized.

Show comment
Hide comment
@pvgupta24

pvgupta24 Jun 21, 2018

Contributor

Uses updated json-message packet. Relies on

Contributor

pvgupta24 commented Jun 21, 2018

Uses updated json-message packet. Relies on

pvgupta24 and others added some commits Jun 21, 2018

Merge pull request #5 from pvgupta24/transcriptions
Uses config from redux state and documentation fixes
@nikvaessen

This comment has been minimized.

Show comment
Hide comment
@nikvaessen

nikvaessen Jul 4, 2018

Contributor

LGTM, @virtuacoplenny do you agree with renaming the feature to 'subtitles'?

Contributor

nikvaessen commented Jul 4, 2018

LGTM, @virtuacoplenny do you agree with renaming the feature to 'subtitles'?

@@ -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/subtitles';

This comment has been minimized.

@virtuacoplenny

virtuacoplenny Jul 5, 2018

Contributor

I wonder if endpointMessageReceived should be located somewhere other than subtitles if the endpoint messages could be used for more than subtitles. I suppose it doesn't hurt to keep it in subtitles for now though.

@virtuacoplenny

virtuacoplenny Jul 5, 2018

Contributor

I wonder if endpointMessageReceived should be located somewhere other than subtitles if the endpoint messages could be used for more than subtitles. I suppose it doesn't hurt to keep it in subtitles for now though.

This comment has been minimized.

@nikvaessen

nikvaessen Jul 5, 2018

Contributor

iirc voting/polls might use it in the future.

@nikvaessen

nikvaessen Jul 5, 2018

Contributor

iirc voting/polls might use it in the future.

Show outdated Hide outdated config.js
Show outdated Hide outdated react/features/large-video/components/LargeVideo.web.js
Show outdated Hide outdated react/features/subtitles/components/TranscriptionSubtitles.web.js
Show outdated Hide outdated react/features/subtitles/components/TranscriptionSubtitles.web.js
Show outdated Hide outdated react/features/subtitles/components/TranscriptionSubtitles.web.js
// unstable results
newTranscriptMessage.stable = text;
newTranscriptMessage.unstable = undefined;

This comment has been minimized.

@virtuacoplenny

virtuacoplenny Jul 5, 2018

Contributor

Is there any danger of modifying the object in place instead of creating a copy of newTranscriptMessage?

@virtuacoplenny

virtuacoplenny Jul 5, 2018

Contributor

Is there any danger of modifying the object in place instead of creating a copy of newTranscriptMessage?

This comment has been minimized.

@pvgupta24

pvgupta24 Jul 7, 2018

Contributor

Doesn't seem to cause any trouble but I will make a copy as redux recommends not to update state variable directly and reducer states should be immutable.

@pvgupta24

pvgupta24 Jul 7, 2018

Contributor

Doesn't seem to cause any trouble but I will make a copy as redux recommends not to update state variable directly and reducer states should be immutable.

Show outdated Hide outdated react/features/subtitles/middleware.js
@virtuacoplenny

This comment has been minimized.

Show comment
Hide comment
@virtuacoplenny

virtuacoplenny Jul 6, 2018

Contributor

Jenkins test this please.

Contributor

virtuacoplenny commented Jul 6, 2018

Jenkins test this please.

pvgupta24 and others added some commits Jul 7, 2018

Merge pull request #6 from pvgupta24/transcriptions
Moves subtitles config to interfaceConfig and minor fixes
@virtuacoplenny

This comment has been minimized.

Show comment
Hide comment
@virtuacoplenny

virtuacoplenny Jul 7, 2018

Contributor
19:31:57 /home/jenkins/jitsi-meet/react/features/subtitles/middleware.js
19:31:57   74:19  error  A space is required after '{'   object-curly-spacing
19:31:57   74:66  error  A space is required before '}'  object-curly-spacing
Contributor

virtuacoplenny commented Jul 7, 2018

19:31:57 /home/jenkins/jitsi-meet/react/features/subtitles/middleware.js
19:31:57   74:19  error  A space is required after '{'   object-curly-spacing
19:31:57   74:66  error  A space is required before '}'  object-curly-spacing
@pvgupta24

This comment has been minimized.

Show comment
Hide comment
@pvgupta24

pvgupta24 Jul 8, 2018

Contributor

Weird. I got that particular lint error before the commit and did fix it. 🤔

Contributor

pvgupta24 commented Jul 8, 2018

Weird. I got that particular lint error before the commit and did fix it. 🤔

There have been many changes since this review

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Jul 9, 2018

Member

I don't know why gh marks the tests as failed, while none of them actually failed

Member

bgrozev commented Jul 9, 2018

I don't know why gh marks the tests as failed, while none of them actually failed

@virtuacoplenny

This comment has been minimized.

Show comment
Hide comment
@virtuacoplenny

virtuacoplenny Jul 9, 2018

Contributor

Github shows the tests as failed because ios didn't get run. Considering there is a deploy happening and we should wait until that happens, I can re-run these tests when ios gets fixed (or we can probably go forward if the fix never happens).

Contributor

virtuacoplenny commented Jul 9, 2018

Github shows the tests as failed because ios didn't get run. Considering there is a deploy happening and we should wait until that happens, I can re-run these tests when ios gets fixed (or we can probably go forward if the fix never happens).

@bgrozev bgrozev merged commit d3dd54a into jitsi:master Jul 17, 2018

1 check failed

default 777 tests run, 635 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment