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

Improve errors and warnings when shortcuts already exist #205

Merged
merged 16 commits into from May 20, 2024

Conversation

marcoesters
Copy link
Contributor

@marcoesters marcoesters commented May 7, 2024

Description

On Windows and Linux, shortcuts are overwritten when they already exist. On MacOS, menuinst exits with an error because it cannot create the Resources directory inside the app.

Since an app is not a simple file, removing the entire directory before creating the new app is the easiest way to ensure that a proper overwrite was done. The test I fails with the current menuinst.

I also decided to add warnings to when shortcuts are overwritten. This behavior has always been a little hidden and could cause some surprises if users are not aware of this behavior.

EDIT: after some discussion, a more prudent way is to improve the error message. Individual packages can still remove the app using pre-link features if they want to overwrite.

Closes #203.

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label May 7, 2024
@marcoesters marcoesters changed the title Osx app overwrite Overwrite MacOS apps if they already exist May 7, 2024
@marcoesters marcoesters marked this pull request as ready for review May 7, 2024 17:58
@marcoesters marcoesters requested a review from a team as a code owner May 7, 2024 17:58
@jaimergp
Copy link
Contributor

jaimergp commented May 8, 2024

I wonder now if we should rename existing shortcuts so there's a backup in case we overstepped accidentally.

tests/test_api.py Outdated Show resolved Hide resolved
@jaimergp
Copy link
Contributor

jaimergp commented May 9, 2024

I wonder now if we should rename existing shortcuts so there's a backup in case we overstepped accidentally.

Any thoughts about this question?

@marcoesters
Copy link
Contributor Author

I wonder now if we should rename existing shortcuts so there's a backup in case we overstepped accidentally.

Any thoughts about this question?

I think this would be too drastic of a change in behavior and can quickly lead to cluttering up a user's system if the designer of the menu item is not careful.

@jaimergp
Copy link
Contributor

I was thinking about how the macos case is a bit different. On Windows and Unix, the "shortcut file" is just a file whose sole purpose is metadata to launch a program. However in macOS it is a directory, and we might be very unlucky if someone publishes a shortcut named Finder (or any other preexisting app). We are writing to ~/Applications for this reason too, but if we change that behavior in the future this might be an oversight.

I think that at the very least we should send it to Trash (via osascript maybe?), instead of removing it. At least they'll have the opportunity for recovery. The annoyance of having multiple copies is not as worse as deleting an important directory.

@marcoesters
Copy link
Contributor Author

I was thinking about how the macos case is a bit different. On Windows and Unix, the "shortcut file" is just a file whose sole purpose is metadata to launch a program. However in macOS it is a directory, and we might be very unlucky if someone publishes a shortcut named Finder (or any other preexisting app). We are writing to ~/Applications for this reason too, but if we change that behavior in the future this might be an oversight.

This is not a risk for system-critical applications because they are in /System/Applications, but your point is well taken. Safari does get installed into /Applications though. We do install into /Applications as root, too.

I think that at the very least we should send it to Trash (via osascript maybe?), instead of removing it. At least they'll have the opportunity for recovery. The annoyance of having multiple copies is not as worse as deleting an important directory.

The problem isn't just that they don't exist, but that menuinst will never remove them as far as I understand it. Let's assume we rename them somehow and we have App.app and App (1).app. Removing the app would leave App (1).app behind.

I see the following ways out:

  1. Move into Trash, as you said, but with a warning
  2. Use subdirectories to isolate the application the way Windows does it - that may be an anti-pattern for MacOS though
  3. Crash as before, but with a very descriptive error message (i.e., "remove the existing application or run as with the --no-shortcuts" option).

@jaimergp
Copy link
Contributor

None is ideal... I'm leaning towards (3), but we should check something first. Does the name of the .app directory affect the display name in the UI? Because it does not (and it gets it from the Plist config inside), maybe we can just add some timestamp to the filename to avoid collisions.

@jaimergp
Copy link
Contributor

Wrt to (2), I'm not sure it's an antipattern. I do see a "Chrome apps" subdirectory in my ~/Applications. See https://apple.stackexchange.com/questions/182626/chrome-keeps-creating-application-folder-in-home-folder for some context. Reading there I also learnt that Chrome puts the same .app directories under ~/Library/Application Support/Google/Chrome/Default/Web Applications/LONG_RANDOM_IDENTIFIER/ too, so we could also do that if needed (with a symlink?). Something like ~/Library/Application Support/Menuinst/X/Y/Z.

@marcoesters
Copy link
Contributor Author

None is ideal... I'm leaning towards (3), but we should check something first. Does the name of the .app directory affect the display name in the UI? Because it does not (and it gets it from the Plist config inside), maybe we can just add some timestamp to the filename to avoid collisions.

I generally wouldn't want timestamps because they don't tell the user anything. However, the point is moot since there is a display change: the name next to the Apple symbol in the taskbar shows the name of the file, not the name inside Info.plist.

Wrt to (2), I'm not sure it's an antipattern. I do see a "Chrome apps" subdirectory in my ~/Applications. See https://apple.stackexchange.com/questions/182626/chrome-keeps-creating-application-folder-in-home-folder for some context. Reading there I also learnt that Chrome puts the same .app directories under ~/Library/Application Support/Google/Chrome/Default/Web Applications/LONG_RANDOM_IDENTIFIER/ too, so we could also do that if needed (with a symlink?). Something like ~/Library/Application Support/Menuinst/X/Y/Z.

While it may not be an anti-pattern, it will at least be a breaking change for other developers who have already pushed MacOS apps using menuinst. The sym-link option is intriguing though.

For now, a better error message may indeed be the solution.

@marcoesters marcoesters changed the title Overwrite MacOS apps if they already exist Improve errors and warnings when shortcuts already exist May 14, 2024
@marcoesters marcoesters requested a review from jaimergp May 14, 2024 22:20
menuinst/platforms/osx.py Outdated Show resolved Hide resolved
menuinst/platforms/osx.py Outdated Show resolved Hide resolved
menuinst/platforms/win.py Outdated Show resolved Hide resolved
@@ -18,6 +18,11 @@ four keys
- `osx`- see {class}`MacOS schema <menuinst._schema.MacOS>`
- `win`- see {class}`Windows schema <menuinst._schema.Windows>`

```{warning}
`menuinst` will overwrite existing shortcuts, so `menu_name` and `name` must be chosen accordingly.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to update this to cover the macOS logic.

marcoesters and others added 2 commits May 15, 2024 06:55
Co-authored-by: jaimergp <jaimergp@users.noreply.github.com>
@marcoesters marcoesters requested a review from jaimergp May 15, 2024 14:16
@marcoesters marcoesters merged commit 4db5647 into conda:main May 20, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Status: 🏁 Done
Development

Successfully merging this pull request may close these issues.

Installation of MacOS apps fail when app already exists
3 participants