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.

Sign up for GitHub

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

Jump to bottom

fix: edge case in app.isInApplicationsFolder() #35636

Merged
merged 8 commits into from Sep 19, 2022
Merged

fix: edge case in app.isInApplicationsFolder() #35636

merged 8 commits into from Sep 19, 2022

Conversation

KishanBagaria
Copy link
Contributor

Description of Change

This fixes an edge case in app.isInApplicationsFolder() which returns false when an app is launched from Terminal like this:

/applications/App.app/Contents/MacOS/App

while this works fine:

/Applications/App.app/Contents/MacOS/App

(Altho this seems unlikely to affect things much, we've received at least one user report of app.isInApplicationsFolder() returning false incorrectly when the app is launched from Finder and this has some likelihood of fixing that.)

Checklist

Release Notes

Notes: Fixed an edge case in app.isInApplicationsFolder() which would return false incorrectly in some cases.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 11, 2022
ckerr
ckerr previously approved these changes Sep 12, 2022
View reviewed changes
miniak
miniak requested changes Sep 12, 2022
View reviewed changes
Copy link
Contributor

@miniak miniak left a comment

Choose a reason for hiding this comment

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

the filesystem can be formatted as case-sensitive

@miniak
Copy link
Contributor

miniak commented Sep 12, 2022

https://stackoverflow.com/questions/4706215/mac-os-x-how-to-determine-if-filesystem-is-case-sensitive

@ckerr ckerr dismissed their stale review September 12, 2022 21:21

@miniak's suggestion is correct

@KishanBagaria
Copy link
Contributor Author

@miniak can you say more about how the change should be implemented?

@MarshallOfSound
Copy link
Member

You need to normalize the bundle path (effectively call realpath on it)

@KishanBagaria
Copy link
Contributor Author

Good idea, done.

@ckerr ckerr requested a review from miniak September 15, 2022 16:52
@codebytere codebytere added semver/patch backwards-compatible bug fixes target/19-x-y target/21-x-y PR should also be added to the "21-x-y" branch. and removed platform/macOS labels Sep 15, 2022
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 15, 2022
@ckerr ckerr requested a review from miniak September 15, 2022 22:08
@jkleinsc jkleinsc merged commit 76ce6d5 into electron:main Sep 19, 2022
@release-clerk
Copy link

release-clerk bot commented Sep 19, 2022

Release Notes Persisted

Fixed an edge case in app.isInApplicationsFolder() which would return false incorrectly in some cases.

@trop trop bot mentioned this pull request Sep 19, 2022
Merged
trop bot pushed a commit that referenced this pull request Sep 19, 2022
@KishanBagaria @jkleinsc @electron-bot
fix: edge case in app.isInApplicationsFolder() (#35636)
bb274e8 
* fix: edge case in IsInApplicationsFolder

* use realpath instead

* lint

* revert lowercasing

* optimize

* Update shell/browser/ui/cocoa/electron_bundle_mover.mm

* lint

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
@trop
Copy link
Contributor

trop bot commented Sep 19, 2022

I have automatically backported this PR to "19-x-y", please check out #35729

trop bot pushed a commit that referenced this pull request Sep 19, 2022
@KishanBagaria @jkleinsc @electron-bot
fix: edge case in app.isInApplicationsFolder() (#35636)
78cd682 
* fix: edge case in IsInApplicationsFolder

* use realpath instead

* lint

* revert lowercasing

* optimize

* Update shell/browser/ui/cocoa/electron_bundle_mover.mm

* lint

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
trop bot pushed a commit that referenced this pull request Sep 19, 2022
@KishanBagaria @jkleinsc @electron-bot
fix: edge case in app.isInApplicationsFolder() (#35636)
1af0d11 
* fix: edge case in IsInApplicationsFolder

* use realpath instead

* lint

* revert lowercasing

* optimize

* Update shell/browser/ui/cocoa/electron_bundle_mover.mm

* lint

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
This was referenced Sep 19, 2022
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Sep 19, 2022

I have automatically backported this PR to "21-x-y", please check out #35730

@trop trop bot added in-flight/21-x-y and removed target/21-x-y PR should also be added to the "21-x-y" branch. labels Sep 19, 2022
@trop
Copy link
Contributor

trop bot commented Sep 19, 2022

I have automatically backported this PR to "20-x-y", please check out #35731

@KishanBagaria KishanBagaria deleted the patch-1 branch September 19, 2022 18:21
@trop trop bot added merged/21-x-y PR was merged to the "21-x-y" branch. and removed in-flight/21-x-y labels Sep 19, 2022
jkleinsc added a commit that referenced this pull request Sep 19, 2022
@trop @KishanBagaria @jkleinsc
fix: edge case in app.isInApplicationsFolder() (#35730)
3437ffc 
fix: edge case in app.isInApplicationsFolder() (#35636)

* fix: edge case in IsInApplicationsFolder

* use realpath instead

* lint

* revert lowercasing

* optimize

* Update shell/browser/ui/cocoa/electron_bundle_mover.mm

* lint

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

Co-authored-by: Kishan Bagaria <hi@kishan.info>
Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
@trop trop bot added the merged/20-x-y label Sep 21, 2022
codebytere pushed a commit that referenced this pull request Sep 21, 2022
@trop @KishanBagaria @jkleinsc
fix: edge case in app.isInApplicationsFolder() (#35731)
91bafdd 
fix: edge case in app.isInApplicationsFolder() (#35636)

* fix: edge case in IsInApplicationsFolder

* use realpath instead

* lint

* revert lowercasing

* optimize

* Update shell/browser/ui/cocoa/electron_bundle_mover.mm

* lint

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>

Co-authored-by: Kishan Bagaria <hi@kishan.info>
Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
@commit-lint commit-lint bot mentioned this pull request Jan 12, 2023
Open
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@KishanBagaria @jkleinsc @khalwa
fix: edge case in app.isInApplicationsFolder() (electron#35636)
81e1370 
* fix: edge case in IsInApplicationsFolder

* use realpath instead

* lint

* revert lowercasing

* optimize

* Update shell/browser/ui/cocoa/electron_bundle_mover.mm

* lint

Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckerr ckerr ckerr approved these changes

@jkleinsc jkleinsc jkleinsc approved these changes

@miniak miniak miniak approved these changes

@codebytere codebytere codebytere approved these changes

Assignees
No one assigned
Labels
merged/21-x-y PR was merged to the "21-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@KishanBagaria @miniak @MarshallOfSound @ckerr @jkleinsc @codebytere