zulip-desktop
zulip-desktop copied to clipboard
zulip
[WIP] Add zulip:// URI scheme for navigating within app
What's this PR do?
Registers links starting with zulip:// to open with the Zulip app. Redirects users with installed app to the correct narrow and server (only if the domain is already saved) in the desktop app.
Any background context you want to provide?
Discussion at #470.
Screenshots?
You have tested this PR on:
- [x] Windows
- [ ] Linux/Ubuntu
- [x] macOS
Is this compatible with the Zulip Android app, which also uses zulip:// URLs? If we’re going to do this, we should make sure to keep them compatible.
If we’re going to do this, we should make sure to keep them compatible.
@andersk can you explain what you mean by compatible here?
The mobile app's existing use of zulip:// URLs is only for an authentication flow.
But yes, we should coordinate with the mobile team on what URL syntax we want to use for this because the two apps will end up sharing that syntax.
For the benefit of everyone in this thread, I texted Boris on czo yesterday and asked him to have a look at this PR. Waiting for his suggestions. :)
I don't think we need to call the execPath. Also, you need to register the app's protocol in the package.json.
https://github.com/oikonomopo/electron-deep-linking-mac-win
https://stackoverflow.com/questions/43912119/open-app-and-pass-parameters-with-deep-linking-using-electron-macos
https://discuss.atom.io/t/enable-deep-linking/27518/7
I don't think we need to call the execPath.
Right, I wasn't sure about this earlier. We should note that this will raise an error in development mode when a developer tries to use this feature because then if Zulip isn't installed the conventional way, it won't find an executable. (happened to me while testing)
I ran npm run dist and worked fine. I'll remove execPath.
Also, you need to register the app's protocol in the package.json.
I don't see the need - the system registers the app's protocol regardless, if we just use the current method. Can you explain why this is required if the feature works both on npm start and npm run dist?
I don't see the need - the system registers the app's protocol regardless, if we just use the current method.
I think this is required for macOS.
https://stackoverflow.com/questions/45809064/registering-custom-protocol-at-installation-process-in-electron-app
I think this is required for macOS.
I checked it on a Mac in debug mode, without creating an executable. It worked well then. I'll check once more to make sure, and also create an executable for testing.
I'll post back later tonight (I'll have to borrow a Mac, can't do that immediately)
I'll post back later tonight
I checked this on a Mac and the protocol is registered, but since the open-url event passes the URL as a string rather than as an array (which is the case on Windows), redirection doesn't happen on macOS. Simple enough problem, I'll push a fix.
Alright, I've done a couple of things.
- Made sure that the scheme works for Windows, both in dev and production. (the API needs different arguments in different cases for Windows)
- Fixed macOS redirection problems.
- Added a reload view to make redirection more obvious.
@akashnimare let me know if it works for you now.
Haven't tested this yet, just out of curiosity from the gif, does this work if the app is not currently running.
Haven't tested this yet, just out of curiosity from the gif, does this work if the app is not currently running.
It does, tested that. :)
@kanishk98 i think we only need the URI scheme for the production environment and not for the dev environment. Also, i want to work on this. Ping me if some work is required on this.
i think we only need the URI scheme for the production environment and not for the dev environment.
I agree, but it's not really that big of a hassle to include it in the dev environment. Just an extra condition that makes the experience better for Windows developers too.
Also, i want to work on this. Ping me if some work is required on this.
Thanks a ton! I didn't see this earlier, so I ended up testing out electron-builder configs that focus and start the app when the browser receives a zulip:// URL. That seems to work properly right now, and I'm trying to figure how to catch the URL on Linux systems (again, macOS and Windows work well with just the Electron APIs). I'll text you if I get stuck with some Linux stuff.
This is WIP, I encountered a few bugs in the redirect logic that I haven't fixed yet.
@kanishk98 i went through the PR. What seems to be the issue here? Maybe, i can help.
What seems to be the issue here? Maybe, i can help
That'd be great. Basically the app doesn't redirect to the intended URL if it's not running (the URL just opens up the app and then it stays there). I'm working on this right now, so I'll PM you for some help.
To update everyone keeping an eye on this PR, there are some problems with <webview> that cause bugs with the redirection flow in the app. I've not been able to identify them, mostly because they're pretty erratic across operating systems and I think that it's best if I keep this on hold until work with the move to BrowserView is complete. That should address the problems with this PR.
@kanishk98 are we planning to work on this after BrowserView PR gets merge?
are we planning to work on this after BrowserView PR gets merge?
Yeah, I've rechecked the logic for the redirection and the reloading of the <webview> (controlled by the loadURL() method, not us) is pretty erratic. I'd suggest we wait until that happens because we'll have to change things a bit anyway.
Heads up @kanishk98, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.
An alternative to this, or addition, would be a way to paste a URL into the Zulip App. I get emails with Zulip App URLs. If I open them in the browser, it loads the web app. The emails would have to use the zulip:// scheme instead of https:// for this PR to work with emails. But that would be confusing if the user isn't using the app.
A way to handle any URL may be to paste the URL into the Zulip App search bar and hit enter and have the Zulip App recognize and go to it.
But that would be confusing if the user isn't using the app
Hmm, I'm not sure if I agree with this statement. I'd say that OSes are fairly vocal when redirecting users to desktop apps from URLs - think Windows when it asks if you want to open a Zoom link in the app. Shouldn't that make things obvious to the user?
@akashnimare in case we're planning to pick this up this week or so, I can fix the conflicts and work with @manavmehta on this.