-
-
Notifications
You must be signed in to change notification settings - Fork 801
Adding support for protocol handler #3870
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
base: master
Are you sure you want to change the base?
Adding support for protocol handler #3870
Conversation
28e327a to
5b41936
Compare
|
Looking good so far @marcellintacite! Please also remember to update the DOAP file to mention support for the new XEP |
Co-authored-by: JC Brand <[email protected]>
Co-authored-by: JC Brand <[email protected]>
Co-authored-by: JC Brand <[email protected]>
|
@jcbrand Hello, i would like your review for the test i wrote |
|
@jcbrand i need your help on tests so i can get done with this PR. I am facing some errors |
67fa8c7 to
cd88326
Compare
|
Hello @jcbrand i have fixed test issue |
|
Hi @jcbrand , could you please provide an update on this Pull Request? I'd like to know what's left to address so we can proceed with merging it. |
jcbrand
left a comment
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 took some time to review this PR. Thanks a lot for the work you've done on this so far.
Please have a look at this comment first:
https://github.com/conversejs/converse.js/pull/3870/changes#r2667473276
Since it's the most important one of my review.
It's important that we stick to the plugin architecture and separation of concerns, which is why I think you need to move the functionality to separate plugins.
The roster-related stuff should go into the rosterview plugin and the other actions can go into chatboxviews (which encompasses both MUC and 1:1 whereas chatview is only 1:1).
Let me know if you have any questions or if anything is unclear.
| - #3769: Various OMEMO fixes | ||
| - #3791: Fetching pubsub node configuration fails | ||
| - #3792: Node reconfiguration attempt uses incorrect field names | ||
| - Adds support for opening XMPP URIs in Converse and for XEP-0147 query actions |
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.
This line is under an already released version, it should go under ## Next release.
I think you should also rebase this PR onto the latest commit in the master branch.
| // hash-based (#converse/action?uri=...) | ||
| if (location.hash.startsWith('#converse/action?uri=')) { | ||
| event?.preventDefault(); | ||
| uri = location.hash.split('uri=').pop(); |
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.
Have you tried splititng with the full string? If that works it will be safer.
| uri = location.hash.split('uri=').pop(); | |
| uri = location.hash.split('#converse/action?uri=').pop(); |
|
|
||
| // Decode URI and remove xmpp: prefix | ||
| uri = decodeURIComponent(uri); | ||
| if (uri.startsWith('xmpp:')) uri = uri.slice(5); |
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 would it not start with xmpp: and would it then still be legitimate?
| const cleanUrl = `${window.location.origin}${window.location.pathname}`; | ||
| window.history.replaceState({}, document.title, cleanUrl); |
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 use snake_case for variable names.
| const cleanUrl = `${window.location.origin}${window.location.pathname}`; | |
| window.history.replaceState({}, document.title, cleanUrl); | |
| const clean_url = `${window.location.origin}${window.location.pathname}`; | |
| window.history.replaceState({}, document.title, clean_url); |
|
|
||
| case 'add-roster': | ||
| await handleRosterAction(jid, query_params); | ||
| break; |
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.
Adding users to the roster is not the responsibility of the chatview plugin, which is mainly responsible with showing a UI for 1:1 chats.
We have a plugin architecture in Converse.js and we ideally want each plugin to be solely responsible for its domain.
So the rosterview plugin should be responsible for checking whether there is a roster-related query action.
And if we later have MUC related query actions, they should go into the muc_views plugin, and so on.
The main way in which we enable separation of concerns between the different plugins, is by triggering events and then letting those plugins listen for these events.
So, I think this function needs to be refactored and the roster specific code should be moved into a function in rosterview plugin. You can give it the same name, so there's a routeToQueryAction function here which is only responsible for opening chats and sending messages, and another routeToQueryAction function in rosterview/utils.js which is responsible for handling roster actions.
Then inside the rosterview plugin you also call routeToQueryAction.
Additionally, the other two actions, sending a message and opening a chat apply to both MUCs and 1:1 chats, right? So they should go out of this plugin and into the chatboxviews plugin, since that plugin encompasses both MUC and 1:1 and this particular plugin is only concerned with 1:1 chats.
That way, we avoid spaghetti code and plugins that do things outside of their scope.
| } catch (error) { | ||
| console.warn('Failed to register protocol handler:', error); | ||
| } | ||
| } |
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.
This section of code is more low-level and not directly related to the chatview plugin, so it should go somewhere else.
I think a better place would be to put it inside src/entry.js.
| * Test roster add functionality when action=add-roster | ||
| * This tests URI parsing and adding a contact to the roster | ||
| */ | ||
| it("adds a contact to roster when action=add-roster", |
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.
This test should go into the rosterview plugin once you've done this:
https://github.com/conversejs/converse.js/pull/3870/changes#r2667473276
The other tests should go into the chatboxviews plugin, as explained in the above-linked comment.
Here's a draft for this issue : #1247
This merge request adds support for handling xmpp: protocol links in Converse.js, allowing users to click links like xmpp:[email protected] to automatically open a chat with the specified JID if logged in. This enhances user experience by enabling seamless integration with external XMPP links (e.g., from emails, websites, or other apps).
Before submitting your request, please make sure the following conditions are met:
CHANGES.mddocument it in
docs/source/configuration.rstwith
make checkor you can run them in the browser by runningmake serveand then opening
http://localhost:8000/tests.html.