Skip to content

Conversation

ChaosKid42
Copy link
Contributor

fixes #3815

@ChaosKid42 ChaosKid42 force-pushed the fix_bookmark_removal branch from d2e96ee to 141e163 Compare August 31, 2025 15:43
@jcbrand
Copy link
Member

jcbrand commented Sep 2, 2025

Thanks @ChaosKid42 for making this PR.

There's a slightly older PR to fix #3815 here:
#3821

I think that PR is more correct because with XEP-0048 PEP it's a single item that contains all bookmarks which needs to be updated and so a retract element shouldn't be sent if old XEP-0048 bookmarks are being used.

@ChaosKid42
Copy link
Contributor Author

I don't mind if the other PR is used. Whatever works is fine for me.

I thought BOOKMARKS2 was associated with XEP-0402. The examples in the document make use of retract. That's the reason I implemented it the way it is.

@jcbrand
Copy link
Member

jcbrand commented Sep 2, 2025

I thought BOOKMARKS2 was associated with XEP-0402.

Yes, but the fallback BOOKMARKS uses XEP-0048 (with PEP).

@ChaosKid42
Copy link
Contributor Author

Ah I see. The other PR is supposed to work for XEP-048, too.

So I basically agree with your assessment with one exception: Shouldn't the other PR use retract in case of XEP-0402?

@jcbrand
Copy link
Member

jcbrand commented Sep 2, 2025

Oh, I missed that difference. I've asked for clarification: https://github.com/conversejs/converse.js/pull/3821/files#r2315090585

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.

Removal of boomark leads to new bookmark named 'Symbol(lit-nothing)'

2 participants