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

Fire JitsiConferenceEvents.ENDPOINT_MESSAGE_RECEIVED events via JSON messages received via the Chatroom #575

Merged
merged 5 commits into from Aug 23, 2017

Conversation

Projects
None yet
3 participants
@nikvaessen
Copy link
Contributor

nikvaessen commented Aug 17, 2017

No description provided.

Fire JitsiConferenceEvents.ENDPOINT_MESSAGE_RECEIVED events
via JSON messages received via the Chatroom
chatRoom.addListener(XMPPEvents.JSON_MESSAGE_RECEIVED,
(from, payload) => {
const participant = conference.getParticipantById(
from.split('/')[1]);

This comment has been minimized.

@bgrozev

bgrozev Aug 17, 2017

Member

I think we have a utility function for this (Strophe.getResourceFromJid)

} else {
logger.warn(
'Ignored ENDPOINT_MESSAGE_RECEIVED for not existing '
+ `participant: ${from}`,

This comment has been minimized.

@bgrozev

bgrozev Aug 17, 2017

Member

Technically we ignored an XMPPEvents.JSON_MESSAGE_RECEIVED. I think it might be useful to change the message so that logs from the two places can be distinguished.

@bgrozev

This comment has been minimized.

Copy link
Member

bgrozev commented Aug 17, 2017

@jitsi/developers can some of you library people also take a look, please?

* Check if the given argument is a valid JSON string by parsing it.
* If it successfully parses, the JSON object is returned.
*
* @param jsonString check if this string is a valid json string

This comment has been minimized.

@paweldomas

paweldomas Aug 18, 2017

Member

jsonString type should be documented:
@param {string} jsonString

This comment has been minimized.

@nikvaessen

nikvaessen Aug 18, 2017

Author Contributor

fixed

const json = tryParseJSON(txt);

if (json) {
logger.log('chat json message', from, json);

This comment has been minimized.

@paweldomas

paweldomas Aug 18, 2017

Member

is it going to be many messages during regular session ? maybe debug level would be more appropriate here

This comment has been minimized.

@nikvaessen

nikvaessen Aug 18, 2017

Author Contributor

Yes, it might actually be more than a few per seconds... maybe we should get rid of it altogether?

This comment has been minimized.

@paweldomas

paweldomas Aug 18, 2017

Member

In that case it would be better to get id of it especially that our logging_config.js is not working very well with deployment upgrades

This comment has been minimized.

@nikvaessen

nikvaessen Aug 18, 2017

Author Contributor

Fixed

@paweldomas

This comment has been minimized.

Copy link
Member

paweldomas commented Aug 18, 2017

LGTM just few really minor things

@nikvaessen

This comment has been minimized.

Copy link
Contributor Author

nikvaessen commented Aug 18, 2017

As I'm already considering how to send the json from Jigasi to the react module, I was thinking about how to fix the todo in this PR (to not make every json object passed in the chat suddenly an endpoint message).

Would the following work? The JSON in the chat message needs to contain 2 keys.

  • jitsi-meet-muc-chatroom-endpoint-message-name: Which needs to have a string as a value. I don't think it's necessary to keep a list of strings and check if the given string is in that list, but it might be a possibility.
  • payload: Should we require the key to be an object? or only check if it's given?

Example json:

{
  "jitsi-meet-muc-chatroom-endpoint-message-name":"transcription-result" // or whatever you want
  "payload": {
      the intering json object
  }
}

Any feedback?

@paweldomas

This comment has been minimized.

Copy link
Member

paweldomas commented Aug 18, 2017

Good idea, but maybe something shorter than 'jitsi-meet-muc-chatroom-endpoint-message-name' like 'jitsi-meet-muc-msg-name' or 'jitsi-meet-muc-msg-topic' or 'jitsi-meet-muc-msg-subject'

return json;
}

console.debug('parsing valid json but does not have correct '

This comment has been minimized.

@paweldomas

paweldomas Aug 21, 2017

Member

probably logger should be used instead of console

@paweldomas
Copy link
Member

paweldomas left a comment

LGTM

@bgrozev

This comment has been minimized.

Copy link
Member

bgrozev commented Aug 23, 2017

In the longer term I think we should signal that the message contains JSON in XMPP (e.g. put the JSON in a separate element, not in "message>body"). But this is reasonable for now and doesn't make it any harder to switch later.

@bgrozev bgrozev merged commit 3e9f98e into jitsi:master Aug 23, 2017

1 check passed

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