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

feat: app.moveToApplicationsFolder conflict handling #18916

Merged
merged 10 commits into from Jul 15, 2019

Conversation

@codebytere
Copy link
Member

codebytere commented Jun 21, 2019

Description of Change

Resolves #18805.

We want to keep default move conflict handling behavior in that it's still what most users would expect, but there exist edge cases in which users may not want to be forced into that behavior.

This thus introduces an optional conflict handler that allows developers access to more granular move actions. They could now allow the user to choose whether to delete an existing app in favor of the current one being moved, or whether to quit the current app and focus on the existing one should it both exist and be running. I added a fair amount of new documentation outlining this behavior, but if there are things users may benefit from seeing examples of or nuances that should be added please leave feedback!

cc @MarshallOfSound @deepak1556

Checklist

Release Notes

Notes: Added an optional conflict handling callback to app.moveToApplicationsFolder.

@codebytere codebytere force-pushed the check-sketchy-move branch 2 times, most recently from 63e1b2a to cec9b16 Jun 21, 2019
docs/api/app.md Outdated Show resolved Hide resolved
docs/api/app.md Outdated Show resolved Hide resolved
shell/browser/ui/cocoa/atom_bundle_mover.mm Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the check-sketchy-move branch 2 times, most recently from a01bdea to a58e2e2 Jun 21, 2019
@codebytere codebytere added the wip label Jun 21, 2019
@codebytere codebytere changed the title feat: app.moveToApplicationsFolder conflict handling [WIP] feat: app.moveToApplicationsFolder conflict handling Jun 21, 2019
@codebytere codebytere force-pushed the check-sketchy-move branch from a58e2e2 to 80568f3 Jun 21, 2019
@codebytere codebytere changed the title [WIP] feat: app.moveToApplicationsFolder conflict handling feat: app.moveToApplicationsFolder conflict handling Jun 22, 2019
@codebytere codebytere removed the wip label Jun 22, 2019
@codebytere codebytere force-pushed the check-sketchy-move branch 3 times, most recently from 549adb6 to d2d4ebc Jun 22, 2019
@electron-cation electron-cation bot removed the new-pr 🌱 label Jun 22, 2019
@codebytere codebytere force-pushed the check-sketchy-move branch from d2d4ebc to 089f4eb Jun 23, 2019
Copy link
Member

MarshallOfSound left a comment

I don't think we can add tests for them but I'd like to know for sure what happens if.

  • The handler returns undefined
  • The handler throws an error
docs/api/app.md Outdated Show resolved Hide resolved
docs/api/app.md Outdated Show resolved Hide resolved
docs/api/app.md Outdated Show resolved Hide resolved
shell/browser/ui/cocoa/atom_bundle_mover.h Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the check-sketchy-move branch 2 times, most recently from 7515760 to 05f4718 Jun 27, 2019
@codebytere

This comment has been minimized.

Copy link
Member Author

codebytere commented Jul 2, 2019

@MarshallOfSound it throws this (desired) error:
for both 1) non-boolean return types and 2) manually thrown Errors

Screen Shot 2019-07-01 at 9 29 00 PM

@codebytere codebytere force-pushed the check-sketchy-move branch from 05f4718 to 7f08418 Jul 2, 2019
@codebytere codebytere requested a review from MarshallOfSound Jul 2, 2019
@codebytere codebytere force-pushed the check-sketchy-move branch from 18b8b66 to 826df74 Jul 2, 2019
@codebytere codebytere force-pushed the check-sketchy-move branch from 4a9ef16 to 4861b5a Jul 2, 2019
@codebytere

This comment has been minimized.

Copy link
Member Author

codebytere commented Jul 2, 2019

@MarshallOfSound this should be all set for final review!

@codebytere codebytere merged commit f6a2970 into master Jul 15, 2019
12 of 13 checks passed
12 of 13 checks passed
Artifact Comparison Changes Detected
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20190702.27 succeeded
Details
electron-arm64-testing Build #20190702.26 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Jul 15, 2019

Release Notes Persisted

Added an optional conflict handling callback to app.moveToApplicationsFolder.

@codebytere codebytere deleted the check-sketchy-move branch Jul 15, 2019
@fabiospampinato

This comment has been minimized.

Copy link

fabiospampinato commented Jul 15, 2019

@codebytere I feel this PR completely solves a specific issue: being able to show a dialog in case a conflict arises.

But if an app is already present and running I guess what most people would want to do is quit the old app and then move the new one in, this PR at best would allow me to show the user an error, but what if I just want to quit the old app and proceed? Which would be the most common scenario under this situation IMHO. Is/shoudn't there be an easy way to do that?

And shouldn't an asynchronous conflictHandler function be supported too? Say I want to kill the other app via the shell and I want to wait for it to be actually killed before proceeding.

Also if I don't provide a conflictHandler is this method still returning true in conflicting situations? That would go against the docs that say that an error should be thrown instead: "If we fail to perform the copy, then this method will throw an error."

@codebytere

This comment has been minimized.

Copy link
Member Author

codebytere commented Jul 15, 2019

@fabiospampinato this PR was created to address some use cases; it's unlikely we ever be able to cover all of them.

I don't provide a conflictHandler is this method still returning true in conflicting situations?

No, it does not change previous behavior. All new behavior is optional as documented.

I am happy to continue to iterate on this functionality as time permits.

@fabiospampinato

This comment has been minimized.

Copy link

fabiospampinato commented Jul 15, 2019

No, it does not change previous behavior.

The previous behavior was to return true, so shouldn't the answer here be "yes" and either the behavior or the docs be updated as the two are conflicting? Basically the app is not moved and no error is thrown, this is the opposite of what the docs say and it was the point of my issue that has been closed because of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.