zom / Zom-iOS-XMPP

THIS PROJECT IS NOW CLOSED. WE HAVE MOVED TO A NEW ZOM 2.0 MATRIX CORE. FOLLOW THE LINK!
https://github.com/zom/zom-ios-matrix
Mozilla Public License 2.0
37 stars 23 forks source link

Delivery receipts in OMEMO groups #571

Open cstiens opened 6 years ago

N-Pex commented 6 years ago

@chrisballinger It seems the problem lies in XMPPFramework, Extensions/XEP-0184/XMPPMessageDeliveryReceipts.m, line 143. We don't request receipts for group messages. Is there a particular reason for this? If I comment out that check everything seems to work for me.

tiffrobo commented 6 years ago

What should I see when testing?

What I am seeing: Whoever has OMEMO turned on in the group:

If someone does NOT have OMEMO turned on in the group:

Device: iPhone 6, iPhone 7, iPad OS: 10.3.3, 11.2.2, 10.3.2 App version: build 129

chrisballinger commented 6 years ago

@N-Pex Ah good catch. I'm not sure the original reasoning. We do manually add the receipt request:

    @objc public func sendRoomMessage(_ message: OTRXMPPRoomMessage) {
        let elementId = message.xmppId ?? message.uniqueId
        let body = XMLElement(name: "body", stringValue: message.messageText)
        let xmppMessage = XMPPMessage(messageType: nil, to: nil, elementID: elementId, child: body)
        xmppMessage.addReceiptRequest()
        let originId = message.originId ?? message.xmppId ?? message.uniqueId
        xmppMessage.addOriginId(originId)
        send(xmppMessage)
    }

Wonder why that doesn't work as expected.

N-Pex commented 6 years ago

@chrisballinger Because the OMEMO group messages never go down that path, please see MessageQueueHandler.sendGroupMessage. Do you want me to submit a pull request for XMPPFramework (Robbie's upstream repo) or should we just add "addReceiptRequest()" to the other places where it's needed?

chrisballinger commented 6 years ago

I think either approach is fine. If you put a PR for XMPPFramework I'd suggest adding a BOOL option that retains the old behavior by default. The sending was decoupled from OMEMOModule a while ago so it's a lot easier to do manually: https://github.com/robbiehanson/XMPPFramework/blob/master/Extensions/OMEMO/OMEMOModule.h#L132

cstiens commented 6 years ago

@N-Pex will you close this or tag it with 'Test Again' when you feel that your part of the work is done?

N-Pex commented 6 years ago

Ok, should work in build 130

cstiens commented 6 years ago

Thanks @N-Pex I'm going to move this to the 'test again' milestone so we can keep our eye on how consistent the delivery receipts are. So, far, they are showing up now!