XEP-0333 Chat Markers implementation

[rufferson]

I was trying to create test unit for markers and stumbled upon several inconsistencies

• No caps discovery - it relies on receipts_conceivably_supported which checks for receipts but not markers
• No ignore unknown markers rule - unsolicited marker creates a new channel
• Marker is parsed as part of the message while XEP mandates marker to be sent in separate message, similar to receipt, and receipt is parsed (and ignored) separately.

@noonien-d I might be missing something - do you remember what driven you to these decisions, was it just a quick'n'dirty patch in (proof of concept implementation) or there were some other drivers doing it this way? I.e. I think the driver was to use carbon extraction and perhaps just misses a condition case to suppress create_if_missing?

[noonien-d]

I implemented chat markers for two use cases:

• show a read maker on the senders device
• remove notifications on my other devices for that conversation (using carbon copied chat markers)

For the latter, I need to ignore the senders capabilities. Obviously, that does not comply with the standard, but I assumed that clients will simply ignore unknown message types.

I'm not sure if I intentionally left the ignore unknown markers part. Maybe because I didn't want to lose markers after a reconnection..? That should be less of an issue when we have long living connections due to sm.
At least I know that I decided to open channels for markers in 4b138f5 . That could also be a Sailfish thing, iirc the TpChannels there get closed after some time.

Sorry, I don't get your last point. To what part of the XEP do you refer?

[rufferson]

Sorry my mind wandered it seems, I meant the receipt is handled separately with own callback which does not create channel. So i meant to say if the reason to land markers to message parser (instead of receipt extractor) was due to carbons, and channel creation just crept in as an oversight of this decision. But ok I see it was intentional. Need to dig deeper into channels as when they are considered as existing. Technically if there's no channel - there's nothing to mark. In case of history (mam) it's all different but then channels should be mapped to conversations (like in purple) which should be recovered from history.

Anyway I'll probably add missing code to align it with specification and add test unit according to spec.
Carbon-copy will ignore caps anyway, caps are needed to make a decision to set <markable> tag. And currently it is set only if counterpart supports receipts (receipts_conceivably_supported - regardless of whether they support markers or not). So the only real question is about create_if_missing. Which is deeper than mere markers.

[rufferson]

Also it leaks the presence, during test execution, if contact not on roster sends us markable message, and we read it (ack) - it emits 'displayed' marker

def replying_to_requests(q, bus, conn, stream):
    jid = 'lechuck@lucasarts.lit'

    # We shouldn't send markers to people who aren't on our roster.
    q.forbid_events([EventPattern('stream-message', to=jid)])
    stream.send(
        elem('message', from_=jid, type='chat', id='alpha')(
          elem('body')(
            u"You didn't kill me, you moron!"
          ),
          elem(ns.NS_CHAT_MARKERS, 'markable')
        ))

    event = q.expect('dbus-signal', signal='NewChannels')
    path, props = event.args[0][0]
    text_chan = wrap_channel(bus.get_object(conn.bus_name, path), 'Text')
    event = q.expect('dbus-signal', signal='MessageReceived')
    message = event.args[0]
    header, body = message
    message_id = header['pending-message-id']
    text_chan.Text.AcknowledgePendingMessages([message_id])

    sync_stream(q, stream)
    q.unforbid_all()
-----
servicetest.ForbiddenEventOccurred: 
- type stream-message
- message_type: 'chat'
- stanza: ''
- stream: '<gabbletest.XmppXmlStream object at 0x7f1deccd6880>'
- subqueue: 'stream'
- subtype: 'message'
- to: 'lechuck@lucasarts.lit'

So in total to fix (note to myself or to whoever wants to tackle it):

• Correctly implement caps - stamp markable when contact supports markers or forced (or bare, optionally)
• Correctly handle incoming markables - strip flag if contact has no from presence subscription (explicitly or we're invisible)
• Correctly handle incoming markers - (needs additional investigation, maybe nothing to be done here - the point is preventing a new chat on unsolicited marker)