search

conda / menuinst

Cross platform menu item installation
https://conda.github.io/menuinst/
BSD 3-Clause "New" or "Revised" License
33 stars 41 forks source link

Improve errors and warnings when shortcuts already exist #205

Closed marcoesters closed 1 month ago

marcoesters commented 1 month ago

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 ...

jaimergp commented 1 month ago

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

jaimergp commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.