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

Uses json message extension for publishing json transcripts. #111

Merged
merged 6 commits into from Jul 10, 2018

Conversation

Projects
None yet
3 participants
@pvgupta24
Contributor

pvgupta24 commented Jun 16, 2018

Relies on PR jitsi/jitsi#495

@pvgupta24 pvgupta24 changed the title from WIP : Uses json message extension transcripts. to WIP : Uses json message extension for publishing json transcripts. Jun 16, 2018

@nikvaessen

This comment has been minimized.

Show comment
Hide comment
@nikvaessen

nikvaessen Jun 16, 2018

Contributor

Please create a new branch based on master and cherry-pick your commit on top of it.

Contributor

nikvaessen commented Jun 16, 2018

Please create a new branch based on master and cherry-pick your commit on top of it.

@pvgupta24 pvgupta24 changed the title from WIP : Uses json message extension for publishing json transcripts. to Uses json message extension for publishing json transcripts. Jun 21, 2018

Show outdated Hide outdated ...main/java/org/jitsi/jigasi/transcription/LocalJsonTranscriptHandler.java
*/
protected void sendJsonMessage(ChatRoom chatRoom, T jsonMessage)
{
if (chatRoom == null)

This comment has been minimized.

@nikvaessen

nikvaessen Jun 21, 2018

Contributor

maybe you can check already here if chatroom is of the right type and return when it's not

@nikvaessen

nikvaessen Jun 21, 2018

Contributor

maybe you can check already here if chatroom is of the right type and return when it's not

}
catch (OperationFailedException e)
{
logger.warn("Failed to send json message " + messageString, e);

This comment has been minimized.

@nikvaessen

nikvaessen Jun 21, 2018

Contributor

with the current PR in jitsi this will result in 1 error and 1 warning log message for the same failure of not sending?

@nikvaessen

nikvaessen Jun 21, 2018

Contributor

with the current PR in jitsi this will result in 1 error and 1 warning log message for the same failure of not sending?

This comment has been minimized.

@pvgupta24

pvgupta24 Jun 22, 2018

Contributor

Removed it from Jitsi PR.

@pvgupta24

pvgupta24 Jun 22, 2018

Contributor

Removed it from Jitsi PR.

@pvgupta24

This comment has been minimized.

Show comment
Hide comment
@pvgupta24

pvgupta24 Jun 26, 2018

Contributor

@nikvaessen, please review.

Contributor

pvgupta24 commented Jun 26, 2018

@nikvaessen, please review.

@nikvaessen

LGTM, maybe one small change, up to you

@nikvaessen

This comment has been minimized.

Show comment
Hide comment
@nikvaessen

nikvaessen Jul 4, 2018

Contributor

pom.xml should be updated with new desktop version before this is merged.

Contributor

nikvaessen commented Jul 4, 2018

pom.xml should be updated with new desktop version before this is merged.

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Jul 9, 2018

Member

You can now update jitsi-desktop.version to 2.13.2c8714b (which is the latest one, generated after jitsi#495 was merged, see jitsi/jitsi-maven-repository@f884a04).

LGTM otherwise.

Member

bgrozev commented Jul 9, 2018

You can now update jitsi-desktop.version to 2.13.2c8714b (which is the latest one, generated after jitsi#495 was merged, see jitsi/jitsi-maven-repository@f884a04).

LGTM otherwise.

@bgrozev bgrozev merged commit 89bfa54 into jitsi:master Jul 10, 2018

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