Skip to content

Add proper native bookmarks deletion #3821

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add proper native bookmarks deletion #3821

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 38 additions & 2 deletions src/headless/plugins/bookmarks/collection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,14 @@ class Bookmarks extends Collection {
this.on('change:autojoin', this.onAutoJoinChanged, this);
this.on(
'remove',
/** @param {Bookmark} bookmark */
(_, bookmark) => this.sendBookmarkStanza(bookmark),
/** @param {Bookmark} bookmarks */
async (bookmark, bookmarks) => {
const bare_jid = _converse.session.get('bare_jid');
const sourceBookmark = (await api.disco.supports(`${Strophe.NS.BOOKMARKS2}#compat`, bare_jid))
? bookmark
: bookmarks;
this.removeBookmarkStanza(sourceBookmark)
},
Comment on lines +40 to +47
Copy link
Member

@jcbrand jcbrand Sep 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your pull request!

From what I can tell, the code looks correct, but I have a few suggestions regarding structure.

I think here we can have:

this.on('remove', this.removeBookmark, this);

Note: removeBookmarkStanza makes it sound like a stanza is being removed, which is not the case.

Then in removeBookmark, you can put the logic above to determine sourceBookmark.

BTW, in this codebase we name variables with snake-case, so sourceBookmark should be source_bookmark.
I know this is not how it's done in most JS codebases, but this repo started 10 years ago before JS conventions were well established and for consistency's sake we should keep to this convention.

Also, would you be willing to look into adding tests for this?
You can add the tests here: https://github.com/conversejs/converse.js/blob/master/src/headless/plugins/bookmarks/tests/bookmarks.js

There are multiple examples which should hopefully make it clear to you how to write a test.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, no problem. As for the tests, would it be okay if I make 2 separate tests for both Strophe.NS.BOOKMARKS2 and Strophe.NS.BOOKMARKS?

Copy link
Member

@jcbrand jcbrand Sep 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, no problem. As for the tests, would it be okay if I make 2 separate tests for both Strophe.NS.BOOKMARKS2 and Strophe.NS.BOOKMARKS?

Yes that would be great thanks!

We put the BOOKMARKS tests inside a file called deprecated.js, see here for example in the bookmark-views plugin:
https://github.com/conversejs/converse.js/blob/master/src/plugins/bookmark-views/tests/deprecated.js

So you'll have to create that file in src/headless/plugins/bookmarks/tests/ and put those tests there.

this
);

Expand Down Expand Up @@ -127,6 +133,36 @@ class Bookmarks extends Collection {
}
}

/**
* @param {Bookmark} bookmark
* @returns {Promise<void|Element>}
*/
async removeBookmarkStanza (bookmark) {
const bare_jid = _converse.session.get('bare_jid');

const node = (await api.disco.supports(`${Strophe.NS.BOOKMARKS2}#compat`, bare_jid))
? Strophe.NS.BOOKMARKS2
: Strophe.NS.BOOKMARKS;

if (node === Strophe.NS.BOOKMARKS2) {

const retractItem = stx`
<item id="${bookmark.get('jid')}">
<remove/>
</item>`;
Copy link
Member

Choose a reason for hiding this comment

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

Where did you get this markup?

According to the XEP it should look like this:

    <retract node='urn:xmpp:bookmarks:1' notify='true'>
      <item id='theplay@conference.shakespeare.lit'/>
    </retract>

See: https://xmpp.org/extensions/xep-0402.html#removing-a-bookmark

Copy link
Author

Choose a reason for hiding this comment

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

I initially put it in as a placeholder but forgot to replace it with the code from the spec because it didn't throw any errors and actually worked. I couldn't find any mentions of it being officially supported, except for the fact that a similar syntax is used for account deletion (where I got it from) and removing a contact from the WaitingList, though.

The corrected version looks like this:

if (node === Strophe.NS.BOOKMARKS2) {

	const stanza = stx`
	<iq type="set" from="${api.connection.get().jid}" xmlns="jabber:client">
	  <pubsub xmlns='http://jabber.org/protocol/pubsub'>
		<retract node="${node}">
		  <item id="${bookmark.get('jid')}"/>
		</retract>
	  </pubsub>
	</iq>`;

	return await api.sendIQ(stanza);
}

If that's okay, I can change the stanza to this in my commit

Copy link
Member

Choose a reason for hiding this comment

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

Yes please!


return api.pubsub.publish(
null,
node,
retractItem,
{ persist_items: true }
);

}

return this.sendBookmarkStanza(bookmark);
}

/**
* @param {'urn:xmpp:bookmarks:1'|'storage:bookmarks'} node
* @param {Bookmark} bookmark
Expand Down
7 changes: 6 additions & 1 deletion src/headless/types/plugins/bookmarks/collection.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ declare class Bookmarks extends Collection {
* @param {object} [options]
*/
setBookmark(attrs: import("./types").BookmarkAttrs, create?: boolean, options?: object): void;
/**
* @param {Bookmark} bookmark
* @returns {Promise<void|Element>}
*/
removeBookmarkStanza(bookmark: Bookmark): Promise<void | Element>;
/**
* @param {'urn:xmpp:bookmarks:1'|'storage:bookmarks'} node
* @param {Bookmark} bookmark
Expand Down Expand Up @@ -74,4 +79,4 @@ declare class Bookmarks extends Collection {
import { Collection } from '@converse/skeletor';
import Bookmark from './model.js';
import { Stanza } from 'strophe.js';
//# sourceMappingURL=collection.d.ts.map
//# sourceMappingURL=collection.d.ts.map
Loading