-
Notifications
You must be signed in to change notification settings - Fork 49
presence: draft interface for Directed Presence management #259
base: devel
Are you sure you want to change the base?
Conversation
I’d like to get some feedback on the interface before I go about implementing it. Specifically:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far.
We have to think about the outbound presence filters, I think the directed presence should be in filter chain, perhaps we want an extra chain for directed presence to make it easier for services to work selectively.
def subscribe_peer_directed(self, | ||
peer: aioxmpp.JID, | ||
muted: bool = False): | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should take an initial presence as optional argument (so that you can already send a modified directed presence with the request).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s what muted=True
is for. Workflow would be:
server = client.summon(PresenceServer)
handle = server.subscribe_peer_directed(some_peer, muted=True)
handle.presence_filter = …
handle.set_muted(False)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My argument was less about the capability, but more about convenience of use, and conciseness of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but then I’d prefer to be able to set a presence_filter
via the interface.
def rebind_directed_presence(self, | ||
relationship: DirectedPresenceHandle, | ||
new_peer: aioxmpp.JID): | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is the rebinding necessary? Do we direct presence to full JIDs? (Note: This is probably just my ignorance.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we do for MUC. In MUC, you send directed presence to $bare_muc_jid/$nickname. When changing the nickname (either by choice or because the server forces you), you have to redirect the directed presence. Ugly, but that’s how it is.
The directed presences pass through the outbound presence filter chain like any other presence. It is easy to detect, because it has a non-empty I don’t think we need a specific filter chain for that, especially since the outbound/inbound stanza filters are on the StanzaStream level and the StanzaStream should not need to know the difference. |
Is directed presence with other types disallowed? Someone may need the capability to send a "busy" presence to their obnoxious relatives ;). I agree it is easy to detect, but I like pre-filtering if it deduplicates code and reduces the number of filter chain calls (by reducing the number of subscriptions). However, your separation of layers argument makes sense. |
A The other presence types are subscription related and not handled by PresenceServer. |
20d416a
to
4de61e3
Compare
@sebastianriese Mind to give this a review? Somehow, the tight coupling between the DirectedPresenceHandle and the PresenceServer feels ugly. And also I assume that I’m missing some stuff here because I implemented this over the course of a few weeks with loooong breaks. |
return self._muted | ||
|
||
@property | ||
def presence_filter(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am opposed to call it presence_filter and not be an aioxmpp.callbacks.filter. (Why not use an aioxmpp.callbacks.Filter anyway?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of agreed on the nomenclature. Suggestions?
It is not a Filter
object because the handle is under full control of a single object usually (in contrast to the filters on the stanza stream). If the single object needs to delegate control to multiple other things, it can do that by assigning a filter to that attribute.
|
||
Directed Presence is specified in :rfc:`6121` section 4.6. Since the users | ||
server is not responsible for distributing presence updates to peers to | ||
which the client has sent directed presence, special handling is needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we need even more special handling when sending directed presence to peers with from or both subscription (as the RFC specifies that those still receive presence broadcasts). So the current code sends the presence twice to those. (Probably we simply do not want to use this service with those we share subscription with due to the different processing by the server. This should be documented.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We indeed need to document that care should be taken. It’s not as trivial as that, for example when stuff like XEP-0186 (Invisible Command) comes into play.
|
||
If the presence relationship is still active when the method is called, | ||
:attr:`~aioxmpp.PresenceType.UNAVAILABLE` presence is sent to the peer | ||
immediately. Otherwise, no stanza is sent. The stanza is passed through |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this to happen in all cases? (e.g. to peers that are subscribed to our presence).
TODO:
aioxmpp.muc
Fixes #213, fixes #212.