Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use PEP (and XEP-0222) for XEP-0292 #1246

Merged
merged 3 commits into from Mar 31, 2023
Merged

Conversation

singpolyma
Copy link
Contributor

While the existing version says this was rejected by community consensus, since then the consensus of most implementations is to use PEP directly even though it was never specified to work, and that the raw-IQ protocol adds complexity. This update removes the raw-IQ mode and specifies the reuse of PEP.

While the existing version says this was rejected by community consensus, since
then the consensus of most implementations is to use PEP directly even though it
was never specified to work, and that the raw-IQ protocol adds complexity.  This
update removes the raw-IQ mode and specifies the reuse of PEP.
@CLAassistant
Copy link

CLAassistant commented Nov 9, 2022

CLA assistant check
All committers have signed the CLA.

<example caption="Receiving a vCard publication/update"><![CDATA[
<message from='romeo@montague.lit'
to='juliet@capulet.lit'
type='headline'>
<event xmlns='http://jabber.org/protocol/pubsub#event'>
<items node='urn:xmpp:vcard4'>
<item id='current'/>
<item id='1e3fdd90-f542-41d3-82ae-33d183467a7a'/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this? Asked in xsf@

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was more clear for the ID here to match the ID in the publication example above, as this is an example notification about that publication, in effect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But shouldn't the publication also use id='current' to avoid to accumulate ever more items?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably pubsub#max_items should be set to 1 for the node.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(tho you'd probably limit the node to 1 item)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this conversation get resolved?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dunno, should it follow the singleton pattern or nah?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given what the XEP already says

Note: There is no payload, because this is a pure notification (the receiver needs to retrieve the vCard using an IQ-get as described earlier).

I think as it was written (using current) is basically not usable because you will always get a notification on connect that says "vcard is current" which... is just obviously true and tells you no information?

@Kev Kev added the Needs Author The XEP is experimental and the PR was not made by the author. The author needs to acknowledge it. label Feb 28, 2023
@Kev
Copy link
Member

Kev commented Feb 28, 2023

@stpeter You're probably the easier of the two Authors to get hold of to approve this :)

@Kev Kev added the Needs Version Block The change requires a version block, and this is to be done by Editors at merge time. label Feb 28, 2023
@Kev
Copy link
Member

Kev commented Feb 28, 2023

This also needs a version block.

@stpeter
Copy link
Member

stpeter commented Feb 28, 2023

+1 LGTM

@guusdk guusdk removed Needs Author The XEP is experimental and the PR was not made by the author. The author needs to acknowledge it. Needs Version Block The change requires a version block, and this is to be done by Editors at merge time. labels Mar 14, 2023
@Kev Kev merged commit 4c644ca into xsf:master Mar 31, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants