Skip to content
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

Fix: Make Electron apps available on macOS again #80

Merged

Conversation

datenreisender
Copy link
Contributor

`window.nativeFile` is only defined when running in the desktop shell
and the version of the desktop shell is recent enough to provide that
property.
The change is meant for macOS
@datenreisender
Copy link
Contributor Author

@pimterry I addressed the issues regarding the wrong type of nativeFile and then wrong "win" in two commits. Squashing them into the first commit would also be fine for me, I just didn't know whether you prefer to keep them separate in the PR to make it easier to see them during review.

Now I see two more aspects:

  1. Use the native file open dialog in uploadFile on all platforms
  2. Adjust the help text on macOS

Use the native file open dialog in uploadFile on all platforms

While I think it is a good idea to do it, I would rather do that in a PR on its own. This has too many other things to be considered (e.g. handling when mime types are passed to uploadFile, maybe setting a better title for the different file open dialogs) that I would rather not do it in this PR here.

Adjust the help text on macOS

The text in

For .app bundles, enter either the bundle name (with or without .app)
or the full path to the executable itself, typically stored in
Contents/MacOS inside the bundle.
isn't really correct anymore now, because with the native file open dialog, people cannot really select app bundle itself any longer. And from what I understood, the function to determine the executable within the app bundle didn't work very well before anyhow, which is why you disabled the Electron injector on macOS in the first place?

I have three ideas for this:

  1. Shorten the text to For .app bundles select the executable itself, typically stored in Contents/MacOS inside the bundle..
  2. Leave it as it is for now.
  3. Leave the text mostly and also enable the selection of directories, so that the app bundle itself can also again be selected, by adding 'openDirectory' to https://github.com/httptoolkit/httptoolkit-desktop/pull/60/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R593

I prefer 1.

What do you think?

@pimterry
Copy link
Member

Thanks for those fixes @datenreisender.

from what I understood, the function to determine the executable within the app bundle didn't work very well before anyhow, which is why you disabled the Electron injector on macOS in the first place?

This is worth clarifying, because that's not quite right. The problem is that if you don't enable treatPackageAsDirectory then when you select a .app file, MacOS doesn't select the directory, instead it tries to find the single file you really meant.

It does that by searching within the entire app package automatically, which is very slow and freezes everything for a while, and usually picks the wrong file at the end anyway. That all happened before we even received the selected file, so there was nothing we could do about this.

If the .app is actually selected as a directory though (i.e. so the server receives "/Applications/abc.app" as the target to intercept) then it's all fine and everything will work great. The logic powering that is here (then called here) - it just reads the .app bundle metadata to get the binary path, and then runs that instead.

This is a good point though, because it highlights that we're not currently supporting that here, but as you point out we could, very easily! I think we should - I'll add a note on the desktop PR.


For your two questions:

I'm happy to enable this Mac-only to start with, and expand support for other features separately, but in that case we do still need to make sure that the API (as defined by the desktop PR) is carefully designed & set up for expansion.

Once any of this is shipped it stays shipped forever (especially in the desktop) so we'll have to remain compatible with it in future. We can't ship a nativeFile.open API that won't work for future expansions cases (or at least, if we wanted to change it incompatibly later we'd have to ship a totally separate API too and support both, which would be a bit annoying). People keep running weird desktop versions for ages, and even downloading & installing them from scratch years later.

I think that's actually fine though - we can handle this by making this open method specific to selecting applications, fully handle that (I think the logic for that specifically is pretty simple) and we can use it here just for that case and delay all other open-file cases to a separate method we'll create later.

I'll add some comments on the desktop PR to explain what I mean in a second.

For the text you mentioned, we need to be a little clever, because the current text is still correct & valid for anybody using HTTP Toolkit from a browser without Electron (where they enter the app path manually with a prompt). That's who the test is currently intended for really, since it's not possible in Electron right now, and that case will still exist in future too. We could detect the different cases, but I think it's easiest to just be very generic with something like:

For .app bundles, you can intercept either the bundle (the .app directory) or the executable itself, typically stored in Contents/MacOS inside the bundle.

(Assuming the changes I'm about to propose on the desktop side, so that works)

Does that make sense?

Because, if added to httptoolkit-desktop, it will be supported on all
platforms.
To take into account that on macOS on the desktop Electron executables
can be selected again in the file picker, which means their path is not
really “entered”.
@datenreisender
Copy link
Contributor Author

@pimterry: Matching httptoolkit/httptoolkit-desktop#60 I also did the needed changes here.

Copy link
Member

@pimterry pimterry left a comment

Choose a reason for hiding this comment

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

The changes here all look good to me, thanks for those changes. It's not strictly required, but I'd like to finish up some changes & testing in for the next desktop release and ship that before I merge this, so I'll leave this open for a little bit, but otherwise I think it's all good to go, thanks!

@pimterry pimterry merged commit 96dfd11 into httptoolkit:main Jul 13, 2023
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.

2 participants