xeps icon indicating copy to clipboard operation
xeps copied to clipboard

Relegate message retraction to xep-0424 instead of using custom one.

Open woj-tek opened this issue 3 years ago • 9 comments

As per, rather small, thread [XEP-0407] - Message retraction question on Standards I changed MIX specification (XEP-0407) and replaced custom retraction mechanism with reference to XEP-0424

woj-tek avatar Mar 31 '21 17:03 woj-tek

Thread: https://mail.jabber.org/pipermail/standards/2021-March/038256.html Contains author approval.

horazont avatar Apr 02 '21 11:04 horazont

Can I have a chance to review as a whole when I’m back at work next week before it’s merged/published please? This is slightly more involved than it appears at first glance and I’d rather think it through first rather than potentially reverting it later.

/K

On 2 Apr 2021, at 12:24, Jonas Schäfer @.***> wrote:

Thread: https://mail.jabber.org/pipermail/standards/2021-March/038256.html https://mail.jabber.org/pipermail/standards/2021-March/038256.html Contains author approval.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/xsf/xeps/pull/1047#issuecomment-812489531, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAC5CXKI5FSJG3IIVKVBETTGWSPLANCNFSM42FDR2HA.

Kev avatar Apr 02 '21 11:04 Kev

@Kev Yes, no problem. I will tag it as Needs Author. Please report back when you’re done.

horazont avatar Apr 02 '21 13:04 horazont

Sorry for the delay, thanks for your patience. I've not been able to think of a reason moving to 424 in principle wouldn't work, but I think there might be bits missing from this PR:

  • Under the previous text it was hopefully obvious how to construct the ID (from the MAM service of the MIX - which it needs to be), while 424 usually works on the origin-id, which wouldn't be appropriate here.
  • The old protocol stored a note of who had retracted the item in the retraction, which I think is missing in 424.
  • I think with the removal of the examples (which arguably shouldn't be the only place anyway, I realise), addressing of the retraction was obvious, which might be less true with the examples gone.
  • It's probably worth keeping an example in place even while refering to 424.
  • If delegating to 424, it probably matters which version of 424 needs to be supported.

Kev avatar May 10 '21 14:05 Kev

@Kev Thanks for finding the time to provide feedback!

@woj-tek Can you please take a look at that feedback and let me know how you want to proceed?

horazont avatar May 11 '21 16:05 horazont

@horazont I'll try to address @Kev 's comments and refine this PR.

woj-tek avatar May 12 '21 08:05 woj-tek

I just pushed commit that should address @Kev 's concerns:

Under the previous text it was hopefully obvious how to construct the ID (from the MAM service of the MIX - which it needs to be), while 424 usually works on the origin-id, which wouldn't be appropriate here.

Somewhat of a OT and rant, but origin-id is just awful. Having server generated stanza-id would fix a lot of issues…

I added explicit comment about using stanza-id here, though I'm not sure it doesn't violates/contradicts 424: "by referring to its Unique and Stable Stanza IDs (XEP-0359) [5] origin ID."

The old protocol stored a note of who had retracted the item in the retraction, which I think is missing in 424.

Shouldn't all messages have from? So even if message is retracted it's meta (to/from) remains intact? (though, it's missing from 424's examples - candidate for another PR?)

I think with the removal of the examples (which arguably shouldn't be the only place anyway, I realise), addressing of the retraction was obvious, which might be less true with the examples gone. It's probably worth keeping an example in place even while refering to 424.

Restored and adjusted examples.

If delegating to 424, it probably matters which version of 424 needs to be supported.

Included particular features - is that ok?

woj-tek avatar May 14 '21 17:05 woj-tek

@woj-tek Thanks for adapting your work! I pinged @Kev, hopefully he’ll find the time for another round soon.

horazont avatar May 25 '21 15:05 horazont

Hi, apologies for delay on my part - this got buried in the notifications.

I improved the example so it should be clearer now which ID should be used. On a sidenode - I think that xep-0369 should add dependency on xep-0359 as well.

Whether the message has a from is different from storing who it was who issued the retraction, no?

Correct. I haven't add any information regarding who made the retraction as... it seems that xep-0424 doesn't have that bit? And it should be included in 0424 first?

I don't think that's enough, is it? If someone implements MIX they need to know which version of 424 to read to implement it.

My previous change already included:

MUST advertise it's support by advertising 'urn:xmpp:message-retract:0' feature

so it did include version? Or am I missing something?

woj-tek avatar Dec 03 '21 12:12 woj-tek