Add setting to optionally include own URLs in tab-completion

[shan995-cell]

PR submitted on behalf of "onf", who sent these two patches by email. Below is his message (with slight modifications for readability).

---

Hi,

I am sending updated patches. I have tried addressing jubalh's feedback:

Thanks for pointing all of these issues out. I am still somewhat new to XMPP so please excuse my ignorance and verbosity and please correct me if I got anything wrong.

I did test the patch before submitting it. Specifically, I tested live messages in chatwin, mucwin and privwin, and history in chatwin. I also briefly tested mucwin history, but clearly not well enough, and carbons I've missed completely.

Autocompletion for /url own and its on/off values has been added and carbons fixed.

I am not completely sure what you mean by "legacy MUC history". Please correct me if any of the following is wrong: If I turn off MAM and join a MUC, the server immediately sends a bunch of old messages with <delay/> but without any namespace whatsoever. That's the legacy MUC history. Those messages are handled as regular MUC messages without passing through any special functions. Profanity also maintains local MUC history which it loads from a sqlite DB and that one passes through the win_print*history functions.

By the way, if I scroll up above these retrieved messages, history is loaded from the DB (passed through win_print_old_history) and suddenly the history is there twice, which I assume isn't the intended behavior.

Regarding MUCs, as you point out, history includes only "MUC-local" JIDs based on users' nicks, making it impossible to figure out whether a message came from us or not (unless the nick is registered in the MUC, but there doesn't seem to be a reliable way of keeping track of that IIUC). get_display_name won't help us because it treats all messages as being sent by us if nick matches which cannot be assumed to always hold true. The only reasonable approach seems to be to add all URLs to completion regardless of url.own setting as you have suggested.

win_print_history and win_print_old_history are used for both mucwin and chatwin. The check is there for chatwin, for which it does the right thing. For mucwin it always evalutes to true, adding all messages to completion as before, and privwin doesn't use history IIUC.

This time I have tested it more thoroughly, with the following results:

chatwin live incoming/outgoing msgs = behavior matches url.own setting
chatwin win_print_history msgs      = behavior matches url.own setting
chatwin win_print_old_history msgs  = behavior matches url.own setting
chatwin carbons                     = behavior matches url.own setting
muc live incoming/outgoing msgs     = behavior matches url.own setting
muc legacy history msgs             = ignores url.own, adds all URLs
muc win_print_history msgs          = haven't figured out how to trigger it
muc win_print_old_history msgs      = ignores url.own, adds all URLs
muc carbons                         = N/A IIUC
privwin live incoming/outgoing msgs = behavior matches url.own setting
privwin history                     = N/A IIUC
privwin carbons                     = completely broken

I didn't determine whether privwin carbons are correctly added to autocomplete because privwin carbons are completely broken. The carbon messages are completely identical to chatwin carbon messages except for the JIDs being "MUC-local JIDs", so Profanity treats them as chatwin messages and opens chatwin to the bare JID, which is obviously a MUC and not an XMPP account, so it doesn't work. Try for yourself, especially when the Profanity receiving carbons hasn't joined the MUC.

For that to be fixed, it seems we need a way to determine if the bare JID is a MUC. If we are already in the MUC, it's easy, otherwise we need some fallback, unfortunately I don't know enough about the possible options here.

Also, if two devices (resources) join a MUC, each under a different nick, and message each other, the messages get duplicated due to carbons. Are you interested in a patch for this edge case?

Anyway, in summary, this should hopefully be good to go. The problems outlined above are results of problems that are already present in master.

[jubalh]

Was this tested by either one of you?

[jubalh]

A first quick and superficial review:

The new own also needs to be added to the tab autcompleter in src/command/cmd_ac.c. Look for url_ac there. And then own also needs to complete on/off.

And I think the scanning of URLs in own messages isn't done for carbon copied messages (sent from another device while we are online with this one).

I didn't check in detail but it looks like equals_our_barejid() isn't appropriate in all the cases. In MUCs message->from_jid->barejid probably will the the MUC JID (without nick) right? Maybe worth checking out get_display_name().

Unconditionally adding the URLs when we get the (legacy) MUC history is okay since we can't validate reliably whether it was sent by us or not.

Would be great if you could also state which paths were tested (in case they were).

[shan995-cell]

Updated to v3 (description updated too) -- printing the url.own setting is now more consistent with the rest of Profanity, following discussion with @jubalh.

Hope I did nothing wrong while submitting the patches...

[shan995-cell]

Updated to v4 -- fixes an indentation error in cmd_ac.c which was flagged by the coding style check thingy.

[jubalh]

In my earlier review I already stated I didn't check in detail but it looks like equals_our_barejid() isn't appropriate in all the cases. In MUCs message->from_jid->barejid probably will the the MUC JID (without nick) right? Maybe worth checking out get_display_name().

I didn't get any feedback on that neither was it adressed in the code.

Speaking of it I didn't receive any answer regarding:
Would be great if you could also state which paths were tested (in case they were).