OMEMO-encrypted MUC logging functionality #94
Conversation
| QStringList List = from.split("/"); | ||
| QString room = List.takeFirst(); | ||
| if (!List.isEmpty()) { | ||
| from = List.join("/"); | ||
| } | ||
| if (from != m_mucNicks[room]) { |
There was a problem hiding this comment.
| QStringList List = from.split("/"); | |
| QString room = List.takeFirst(); | |
| if (!List.isEmpty()) { | |
| from = List.join("/"); | |
| } | |
| if (from != m_mucNicks[room]) { | |
| QString room = from.section('/', 0, 0); | |
| QString nickname = from.section('/', 1); | |
| if (nickname != m_mucNicks.value(room)) { |
prevent empty insertions if for some reason room is not in the map.
| QString Text = body.text(); | ||
| QString MyJid = m_accountInfo->getJid(account); | ||
| MyJid = MyJid.replace("@", "_at_"); | ||
| logMuc(room, from, MyJid, Text, Stamp); |
There was a problem hiding this comment.
| logMuc(room, from, MyJid, Text, Stamp); | |
| logMuc(room, nickname, MyJid, Text, Stamp); |
taking into account my previous suggestion
| // get all MUC nicks of the current account for groupchat logging | ||
| // functionality | ||
| else { | ||
| QStringList room_nick = xml.attribute("to").split("/"); |
There was a problem hiding this comment.
I believe this thing may interfere with some non-muc presences. Like for example when we send a direct presence to a single contact.
Please at least add FIXME here.
| // functionality | ||
| else { | ||
| QStringList room_nick = xml.attribute("to").split("/"); | ||
| m_mucNicks.insert(room_nick[0], room_nick[1]); |
There was a problem hiding this comment.
Seems like we never clean it.
from the first glance it may look like a memory leak. But if it's rather a small cache for Psi life time, likely it's fine.
And I'd rather also use sections here to avoid side effects splitting by "/"
| @@ -330,6 +358,22 @@ bool OMEMOPlugin::encryptMessageElement(int account, QDomElement &message) | |||
| return false; | |||
| } | |||
|
|
|||
| // logging functionality for OMEMO-encrypted groupchats | |||
| if (message.attribute("type") == "groupchat") { | |||
There was a problem hiding this comment.
Please prefer wrapping string literals with QLatin1String. This way we improve performance by avoiding string conversion from "maybe unicode" to "unicode".
QLatin1String("groupchat")
| // logging functionality for OMEMO-encrypted groupchats | ||
| if (message.attribute("type") == "groupchat") { | ||
| QString room = message.attribute("to"); | ||
| QString from = m_mucNicks[room]; |
There was a problem hiding this comment.
m_mucNicks.value(room)
will avoid invalid empty nickname insertion if room is not there.
Also it may be necessary to do some initialization if it's not there.
There was a problem hiding this comment.
Okay, but wouldn't .value() also return an empty string if the room is not there (unless the second parameter of defaultValue is specified, but I'm not sure what kind of default value would be good for this)? Qt docs say so, although I might be misunderstanding it.
There was a problem hiding this comment.
It will return empty string. that's correct.
Since nickname can't be empty, you can check for emptiness and maybe do some initialization if empty.
Imagine a case where you enable OMEMO plugin already after joining. So you missed the initial presence and have to live with this somehow. (drop everything encrypted / force rejoin / ask user what to do)
There was a problem hiding this comment.
I'd go with the easiest way improving the stuff with consequent MRs :)
There was a problem hiding this comment.
Okay, so I changed the behavior to logging the full jid instead of the nick in case the nick is empty. Would that be fine for now? Can't think of any elegant solution to the problem yet
There was a problem hiding this comment.
yep. should be good for now.
but technically all the MUC info has to be available somewhere in Psi including nicks.
There was a problem hiding this comment.
Well, there is a mucNicks() function in ContactInfoAccessingHost, but that just gives a list of nicks from a MUC (without the real jids), so I suppose the realJid() function could be ran on each of the nicks until the one associated with the needed account is found... But that would probably be slow, especially if the MUC is big (although I strongly doubt that there exist big OMEMO-encrypted MUCs, as that's very uncomfortable since everyone seems to have to have everyone else in their roster to encrypt all messages). Not sure what other options are there to fix this.
There was a problem hiding this comment.
It's fine to modify plugin interfaces if anything is needed.
We didn't release any stable plugins sdk yet. So it's Ok to change it.
I think after the coming release we can think about some stabilization of the plugins API.
There was a problem hiding this comment.
Oh, okay. I'll see what can be done, would be great to remove the presence tracking hack as a whole
|
Also, should I add a line into changelog.txt (of OMEMO plugin or/and Conference Plugin) about these changes or are they too minor to mention? |
please add. you can also bump the plugin version. |
|
@kssytsrk: Thanks a lot for your contributions! |
Fixes #93. The changes have been made to the OMEMO plugin to log messages in groupchats, Conference Logger-style, and to the Conference Logger plugin for it to not log "placeholders" of encrypted messages ("You received a message encrypted with OMEMO but your client doesn't support OMEMO or its support is currently disabled.").