Desktop: Handle URL links #5302

roman-r-m · 2021-08-13T22:21:44Z

Partially resolves #5168

roman-r-m · 2021-08-13T22:24:15Z

So far I've only tested in on Linux. I can also test on Windows but I have no way to test Mac OS so looking for volunteers.

Annoyingly on Linux I have to build the AppImage to get it to work but I think on Win & Mac OS it should work with dev builds too.

laurent22 · 2021-08-14T08:57:04Z

Thanks for implementing this. Would you mind running npm run linter ./ from the root to fix the linter errors (as they make it hard to read the diff)? In fact, is the pre-commit not working for you?

I could test the macOS version.

roman-r-m · 2021-08-14T11:09:31Z

Thanks for implementing this. Would you mind running npm run linter ./ from the root to fix the linter errors (as they make it hard to read the diff)? In fact, is the pre-commit not working for you?

I had to commit with --no-verify to disable the linter as there were tons of errors unrelated to my changes. I suspect these files were committed like that before the linter was set up. I'll do it later once all the functional changes are in place.

I could test the macOS version.

That'd be great. There are 2 scenarios:

Joplin is running, opening joplin://<note id> should activate the window and switch to the note.
Joplin is not running. Then opening the above URL should start Joplin and switch to the note.

There are still a few more things to add here but none of them are OS-specific.

laurent22 · 2021-08-14T12:17:57Z

I had to commit with --no-verify to disable the linter as there were tons of errors unrelated to my changes. I suspect these files were committed like that before the linter was set up. I'll do it later once all the functional changes are in place.

It might print many warnings for class member accessibility but you can ignore these (hopefully they'll be fixed over time but there are too many in the old codebase to turn this into an error). Other than that the whole codebase should pass the linter without any error. If you're having one unrelated to your code, please post it here.

… functions to a separate file

roman-r-m · 2021-08-14T12:35:03Z

Linter errors should be fixed now.

tessus · 2021-08-14T12:42:52Z

What happens, if you use the id from a tag or a notebook? Arent't the specs a bit different to your implementation or is this just the first attempt?

packages/app-desktop/ElectronAppWrapper.ts

packages/app-desktop/gui/MainScreen/MainScreen.tsx

packages/lib/ProtocolUtils.ts

packages/app-desktop/gui/Sidebar/Sidebar.tsx

laurent22 · 2021-08-14T13:46:47Z

packages/app-desktop/gui/utils/NoteListUtils.ts

+			if (noteIds.length == 1) {
+				menu.append(
+					new MenuItem({
+						label: _('Copy note URL'),


This is a bit confusing because we already have "Copy markdown link". I think we need to discuss all this in the spec and perhaps also see how it's done in other applications?

Markdown link is a different format though.

I'm not sure how people would use this function but it seems logical to allow for easy creation of links to items that can be linked.

How about calling this "Copy external link"? (and then same for tags and folders)

laurent22 · 2021-08-14T13:49:15Z

packages/lib/ProtocolUtils.ts

+
+export function parseUrl(s: string): UlrInfo {
+	if (!s.startsWith('joplin://')) return null;
+	const url = new URL(s);


Not sure if "URL" is defined in React Native. Perhaps you could use the "url-parse" which is already part of the lib package?

I was going to implement the same functionality for mobile next. Until then I don't think anyone is going to call this code on mobile so while it's not safe it shouldn't be an issue either.

laurent22 · 2021-08-14T13:50:34Z

packages/lib/ProtocolUtils.ts

+}
+
+export function parseUrl(s: string): UlrInfo {
+	if (!s.startsWith('joplin://')) return null;


That should throw an error if the url is invalid. Also shouldn't it start with joplin://x-callback-url/ ?

Actually I don't think throwing error here is a good idea because the error will not be visible in dev tools

packages/lib/ProtocolUtils.ts

packages/app-desktop/main.js

roman-r-m · 2021-08-14T19:15:14Z

What happens, if you use the id from a tag or a notebook? Arent't the specs a bit different to your implementation or is this just the first attempt?

Currently nothing happens except for a message in the log. Is there a way to show a notification to the user?

The spec has

Opening a note: joplin://x-callback-url/openNote?id=NOTE_ID

and

Opening a notebook: joplin://x-callback-url/openFolder?id=FOLDER_ID

I have both of these plus a similar thing for tags: joplin://x-callback-url/openTag?id=TAG_ID

roman-r-m · 2021-08-14T21:29:25Z

This is strange. When I run linter locally there are no errors. I tried npm run linter and npm run linter-ci.

roman-r-m · 2021-08-14T21:34:16Z

And also, I think previously whenever I added a new .ts file and ran npm i the corresponding .js files were added to .gitignore & .eslintignore. Looks like this isn't working anymore for me.

laurent22 · 2021-09-03T15:44:06Z

Ok that should be fixed now. It was incorrectly scanning directories that were ignored due to slightly wrong glob pattern.

mzguy · 2021-11-14T16:09:11Z

Perhaps I'm doing something wrong? I'm testing this feature and when using a callback URL Joplin gains focus, but the note isn't selected. The same note stays on the display. Could something be incorrectly registered in the protocol handler?

I've tested by opening the links from Firefox, and also with xdg-open on the command line. Edited to add: forgot to mention that xdg-open gives an "xdg-settings: invalid application name" error.

roman-r-m · 2021-11-14T16:54:06Z

What's your OS? Since you mention xdg-open I assume it's Linux? How did you install Joplin?

mzguy · 2021-11-14T16:55:45Z

Yes, Ubuntu 20.04 LTE. Joplin installed via the script on the main Github page, so I have the AppImage.

Daeraxa · 2021-11-14T17:33:17Z

I tried it out on Linux Mint 20.04 with 2.5.12 (script appimage) and saw the same issue.

Copy external link + change note
Paste into Firefox
Joplin comes to focus with current or last selected note - not the copied link

Makes no difference if Joplin was already running or was closed. Trying the link again doesn't go to the correct note.

roman-r-m · 2021-11-14T17:46:13Z

Could you also check if the same happens with links to folders or tags?

mzguy · 2021-11-14T18:54:04Z

Same behavior with notebooks (I think that's what you meant) and tags.

Daeraxa · 2021-11-14T19:27:08Z

Same behaviour here, selecting tags or notebooks just goes to the same page you already had open (even from closed).

tessus · 2021-11-14T20:11:25Z

Just tested it on macOS and it works. Well, we always had the issue on macOS that Joplin had to run, because for some reason the info was not passed as an argument. So, if Joplin runs, all is good on macOS.

This seems to be an issue with Linux then. However, I am sure that Roman tested this on Linux when he wrote this PR and afaik it was working back then.

tessus · 2021-11-14T20:12:57Z

Btw, this is the wrong PR. It was merged by accident and then reverted. The correct PR is #5416

Daeraxa · 2021-11-14T20:14:26Z

As the (correct) PR is already merged should this just be opened as an issue instead?

tessus · 2021-11-14T20:24:20Z

Yes, an issue might be better to track this.

roman-r-m · 2021-11-14T20:27:21Z

FWIW it's working for me on Linux. But I did not install it the recommended way, instead I built an AppImage via npm run dist and installed it directly. Just running the recommended install script now to check.

roman-r-m · 2021-11-14T20:43:31Z

Here's what I did.

Uninstalled Joplin completely.
Removed a stray .desktop file and verified with xdg-settings get default-url-scheme-handler joplin
Installed using the official script
Confirmed url handler was set up:

xdg-settings get default-url-scheme-handler joplin
appimagekit-joplin.desktop

Ran Joplin - got a prompt asking to integrate the AppImage which I did
External links are working

Now, I'm not too familiar with different flavours of Linux but I suspect it's the "integrate the app image part" that may be different.

Daeraxa · 2021-11-14T22:41:51Z

Out of interest what are you using? As Linux Mint is just a flavour of Ubuntu we don't have the greatest variation from the existing reports.
Url handler looks ok for me too - or at least the same as yours.

I have to admit I don't ever recall being asked to integrate the app image, it has always just kind of worked without me messing around with it and I don't recall using any other installations (for example the software centre contains the flatpak version).

mzguy · 2021-11-15T02:21:24Z

Now, I'm not too familiar with different flavours of Linux but I suspect it's the "integrate the app image part" that may be different.

I never got a prompt. That said, I first tried it under Firefox, not in a terminal. Perhaps that's necessary?

roman-r-m · 2021-11-15T09:36:54Z

Out of interest what are you using? As Linux Mint is just a flavour of Ubuntu we don't have the greatest variation from the existing reports. Url handler looks ok for me too - or at least the same as yours.

I have to admit I don't ever recall being asked to integrate the app image, it has always just kind of worked without me messing around with it and I don't recall using any other installations (for example the software centre contains the flatpak version).

I'm on Manjaro.
I'll see if I can spin up a VM with Ubuntu and reproduce this.

roman-r-m · 2021-11-15T21:49:31Z

I managed to reproduce it on Ubuntu though I still don't understand where the issue is. Since Joplin receives focus the url-scheme-handler must be working. My only idea is that the OS does something to the url that makes it invalid.
I'll see if I can debug it.

mzguy · 2021-11-15T21:53:52Z

Perhaps you can simply view the URL received from within Joplin to debug. I think your suspicion is correct.

Do you also get "xdg-settings: invalid application name" error when running xdg-open on a Joplin link? That could also be a hint as to what the problem is.

mzguy · 2021-11-15T21:55:34Z

Could there be something different in the implementation of the AppImage on Ubuntu, or perhaps something common to all AppImages, that interferes with the URL being passed?

roman-r-m · 2021-11-15T22:04:27Z

Do you also get "xdg-settings: invalid application name" error when running xdg-open on a Joplin link? That could also be a hint as to what the problem is.

On my main Manjaro system - no, it opens Joplin and switches to the note.

Could there be something different in the implementation of the AppImage on Ubuntu, or perhaps something common to all AppImages, that interferes with the URL being passed?

Possibly, but I am not an expert on Linux.

mzguy · 2021-11-15T22:09:43Z

I figured out a solution.

Go to ~/.local/share/applications and edit appimagekit-joplin.desktop

Change this line:
Exec=~/.joplin/Joplin.AppImage

To:
Exec=~/.joplin/Joplin.AppImage %U

I don't know why it works in some distros though. Also, I figure there should be a way to automate this without requiring a user manually edit the .desktop file.

roman-r-m · 2021-11-15T22:11:58Z

Ha! Was just about to post exactly that.

Including the part where I have no idea why it's working for me.

roman-r-m · 2021-11-15T22:12:47Z

Also, I figure there should be a way to automate this without requiring a user manually edit the .desktop file.

That I can take care of.

roman-r-m · 2021-11-15T22:32:08Z

Just to close this off - I think I now know why it was working for me.

There are 2 .desktop files, one is created by the script, another one is created by electron-builder during the AppImage build and is packed inside the AppImage. I suspect when I "integrated" the AppImage, Manjaro used the 2nd .desktop file. Which does have the %U

mzguy · 2021-11-15T23:25:43Z

To resolve going forward, are you going to fix the script? When installing (or at least updating) via script, there's no prompt to integrate the AppImage, and apparently that isn't done automatically.

Daeraxa · 2021-11-16T00:02:51Z

The PR above has the %u added to the install and update script

roman-r-m added 5 commits August 12, 2021 21:51

Register to handle joplin:// links

b17d8bc

Trying to get it to work on Linux

f909fe6

Actually open the note

0a6390e

Cleanup

0050489

Open the note from URL even if Joplin isn't running

6116103

Handle openFolder and openTag too; change the URL format; extract ULR…

f118f52

… functions to a separate file

roman-r-m marked this pull request as ready for review August 14, 2021 12:35

laurent22 reviewed Aug 14, 2021

View reviewed changes

packages/app-desktop/ElectronAppWrapper.ts Show resolved Hide resolved

packages/app-desktop/gui/MainScreen/MainScreen.tsx Outdated Show resolved Hide resolved

laurent22 reviewed Aug 14, 2021

View reviewed changes

packages/lib/ProtocolUtils.ts Outdated Show resolved Hide resolved

laurent22 reviewed Aug 14, 2021

View reviewed changes

packages/app-desktop/gui/Sidebar/Sidebar.tsx Outdated Show resolved Hide resolved

laurent22 reviewed Aug 14, 2021

View reviewed changes

Read initial url from a field

047883b

roman-r-m added 6 commits August 14, 2021 20:20

Add a function to check for valid callback url

f454c4e

Rename copy folder URL -> copy notebook URL

305d0ff

Code review comments

ecf7180

Review comments - escape vars in url

f0361bf

Rename initialUrl -> initialCallbackUrl

2b6b4dd

Review comments - throw an error if callback url is not valid

ee46978

roman-r-m added 3 commits August 14, 2021 22:34

Update ignore files

b02baa6

Fix linter errors

90621a8

Rename parseUrl -> parseCallbackUrl

2386abe

roman-r-m mentioned this pull request Sep 3, 2021

Desktop: Handle URL links #5416

Merged

roman-r-m mentioned this pull request Nov 15, 2021

Linux: Fix .desktop file to make external links work on Ubuntu #5736

Merged

fstanis mentioned this pull request Nov 20, 2021

Android: Handle URL links #5764

Closed

Desktop: Handle URL links #5302

Desktop: Handle URL links #5302

Conversation

roman-r-m commented Aug 13, 2021

roman-r-m commented Aug 13, 2021

laurent22 commented Aug 14, 2021

roman-r-m commented Aug 14, 2021

laurent22 commented Aug 14, 2021

roman-r-m commented Aug 14, 2021

tessus commented Aug 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roman-r-m commented Aug 14, 2021

roman-r-m commented Aug 14, 2021

roman-r-m commented Aug 14, 2021

laurent22 commented Sep 3, 2021

mzguy commented Nov 14, 2021

roman-r-m commented Nov 14, 2021

mzguy commented Nov 14, 2021

Daeraxa commented Nov 14, 2021

roman-r-m commented Nov 14, 2021

mzguy commented Nov 14, 2021

Daeraxa commented Nov 14, 2021

tessus commented Nov 14, 2021

tessus commented Nov 14, 2021

Daeraxa commented Nov 14, 2021

tessus commented Nov 14, 2021

roman-r-m commented Nov 14, 2021

roman-r-m commented Nov 14, 2021

Daeraxa commented Nov 14, 2021 • edited Loading

mzguy commented Nov 15, 2021

roman-r-m commented Nov 15, 2021

roman-r-m commented Nov 15, 2021

mzguy commented Nov 15, 2021

mzguy commented Nov 15, 2021

roman-r-m commented Nov 15, 2021

mzguy commented Nov 15, 2021 • edited Loading

roman-r-m commented Nov 15, 2021

roman-r-m commented Nov 15, 2021

roman-r-m commented Nov 15, 2021

mzguy commented Nov 15, 2021

Daeraxa commented Nov 16, 2021 • edited Loading

Daeraxa commented Nov 14, 2021 •

edited

Loading

mzguy commented Nov 15, 2021 •

edited

Loading

Daeraxa commented Nov 16, 2021 •

edited

Loading