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

Implement moveToApplicationsFolder #10142

Merged
merged 6 commits into from Aug 31, 2017

Conversation

Projects
None yet
8 participants
@MarshallOfSound
Member

MarshallOfSound commented Jul 28, 2017

Closes #1692

This adds two methods:

  • app.isInApplicationsFolder() - Is the app currently in the system applications folder
  • app.moveToApplicationsFolder() - Attempt to move the app to the Applications folder and relaunch on success

Any UX for asking the user if they want to move the app should be implemented by the app either through HTML/CSS or using the dialog module.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jul 28, 2017

Please note this is heavily influenced (and in fact mostly transposed) by LetsMove

### `app.moveToApplicationsFolder()` _macOS_
Returns `Boolean` - Whether the move was successful. Please note that if
the move was successful your application will be quit and relaunched

This comment has been minimized.

@enlight

enlight Jul 28, 2017

Contributor

-> "if the move is successful your application will quit and relaunch"

This method does not have any dialog or confirmation by default, if you want
to ask your user if you should make this move then you should use the `dialog`
API's.

This comment has been minimized.

@enlight

enlight Jul 28, 2017

Contributor

-> No confirmation dialog will be presented by default, if you wish to allow the user to confirm the operation you may do so using the dialog API.

@poiru poiru self-requested a review Jul 30, 2017

@jkleinsc

This comment has been minimized.

Contributor

jkleinsc commented Aug 7, 2017

@MarshallOfSound while this is useful functionality, it is a lot of code to add to Electron for a very specific use case. It seems like it would be better to do this via a library/package such as electron-lets-move.

@baconbrad

This comment has been minimized.

Contributor

baconbrad commented Aug 7, 2017

I second @jkleinsc's remark. If it is a particular and completely optional use case that can be done as an NPM module then I don't see why that shouldn't be the recommended way. While this code doesn't bloat Electron by itself it would add to the overall bloat in Electron. Additionally those who use auto updaters in their applications that bandwidth from overall bloat starts to add up as well.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Aug 8, 2017

@jkleinsc I'm not sure if we have a policy on core / module but the way I look at it, it isn't just a question of whether this can be implemented in a module (I'm pretty sure it can be). It's a question of developer friendliness and also user friendliness (people using Electron apps).

If we implement this in core more developers will use it and therefore users would get a better experience when using Electron apps. I'm perfectly happy to move this into a module if that's what we want to do.

In terms of risk from the amount of code that is added, this code has been battle tested for years in the LetsMove repository so the code itself comes with minimal risk (probably less than most additions to core).

@zeke

This comment has been minimized.

Member

zeke commented Aug 10, 2017

What about moving it to a module that is then depended on by core? Is there a precedent for that?

@enlight

This comment has been minimized.

Contributor

enlight commented Aug 11, 2017

I'm in the keep the core minimal camp, if something can be implemented in user-land without jumping through too many hoops then it should be. Separate modules also make it possible for users to upgrade/downgrade a module while staying on the same Electron version (in some cases), and to customize modules without maintaining a fork of Electron.

@MarshallOfSound certainly has a point about people getting tripped up by native Node modules ALL THE TIME, but that needs to be addressed by some kind of on-demand native module build bot.

@poiru

This comment has been minimized.

Member

poiru commented Aug 11, 2017

I agree that we shouldn't have niche features in core, but this would be a useful feature for almost all macOS apps. Is it worth the complexity of having this as a separate module? The binary size impact of this is probably a couple of kilobytes at most. Having this as a separate module would almost certainly result in a net size increase. A good chunk of Electron app bloat is due to the huge number of npm packages and their dependency chains -- we don't ship any npm packages in our app because of that.

Compare this to e.g. PDF or WebRTC support, which are in the megabytes and which (I'm guessing) are useless to most Electron apps. If we want to reduce bloat, we should start with the big offenders.

[`dialog`](dialog.md) API.
**NOTE:** This method throws errors if anything other than the user causes the
move to fail. For instance if the user cancels the authorization dialog this

This comment has been minimized.

@zeke

zeke Aug 11, 2017

Member

Under what condition does this method return false? That is, when would it not be "successful" but also not throw?

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Aug 11, 2017

Member

If the move requires elevated permissions and the user cancels the elevated permissions prompt this method returns false.

@zeke

This comment has been minimized.

Member

zeke commented Aug 11, 2017

Is it worth the complexity of having this as a separate module?

I don't know, but I'm curious. I just want to be sure we're not missing opportunities to modularize Electron's codebase.

@zeke

This comment has been minimized.

Member

zeke commented Aug 14, 2017

cc @tommoor who wrote https://github.com/tommoor/electron-lets-move and might have some insight.

@zeke

zeke approved these changes Aug 21, 2017

Seems like we kind of stalled out on the "where should this code live" conversation. It's fine with me to ship this as-is and get the feature into the hands of more Electron devs.

@davej

This comment has been minimized.

Contributor

davej commented Aug 30, 2017

+1 to add this in core. electron-lets-move is great but it relies on sudo-prompt which is a bit of a hack. Basically what sudo-prompt does is:

  1. Use "which" to lookup the location of the AppleScript binary (osascript) and copy this to a tmp path, but using the name of your application as the name of the copied binary. This causes the OS X permissions prompt to show the name of your application instead of the AppleScript binary.
  2. Use the new AppleScript binary to set the sudo timestamp for your user:
var command = '"' + tmposascript + '" -e \'do shell script "mkdir -p /var/db/sudo/$USER; touch /var/db/sudo/$USER" with administrator privileges\'';
  1. This command will cause OS X to show a UI to the user asking for their password.

  2. Finally, re-run the command that required privileges.

(from #1692 (comment))

I think it would be much cleaner to do this natively in Electron.

This leads to another issue, is it worth having an API for running a command with permission? Perhaps something like shell.authorizedChildProcess which would pop up a permissions GUI dialog and give access to a child_process with permissions of the authenticated user? Or even something a bit more basicshell.authorizedExec which would just allow exec?

Let me know if it's worthwhile to create an issue for the above API?

@jkleinsc

This comment has been minimized.

Contributor

jkleinsc commented Aug 30, 2017

@MarshallOfSound given the feedback above and @zeke's decision to approve this, I think we should move this forward. It looks like some of the tests are failing (linux in particular). Can you take a look at the failing tests?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Aug 31, 2017

@jkleinsc Looks like the spec was flawed, should have only been running on darwin. Fixed now, the other failures were just flakes IMO

Once these builds run we should be gtg

@jkleinsc

This comment has been minimized.

Contributor

jkleinsc commented Aug 31, 2017

@MarshallOfSound thanks for updating. It looks like there is an extra semicolon in the test:
/Users/electron/workspace/electron-mas-x64/spec/api-app-spec.js:119:48: Extra semicolon.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Aug 31, 2017

:rip: Always mess up switching between airbnb and standard JS 😆

@jkleinsc

This comment has been minimized.

Contributor

jkleinsc commented Aug 31, 2017

Looks good to me now. I'll merge it in.

@jkleinsc jkleinsc merged commit 6b01061 into master Aug 31, 2017

4 of 6 checks passed

electron-win-x64 Build #4025 failed in 14 min
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
electron-mas-x64 Build #5007 succeeded in 11 min
Details
electron-osx-x64 Build #4994 succeeded in 11 min
Details
electron-win-ia32 Build #4043 succeeded in 17 min
Details

@MarshallOfSound MarshallOfSound deleted the lets-move branch Aug 31, 2017

@djalmaaraujo

This comment has been minimized.

djalmaaraujo commented Oct 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment