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

fix: normalize path before calling showItemInFolder and openPath #41642

Merged
merged 2 commits into from Mar 22, 2024

Conversation

piotrpdev
Copy link
Contributor

Description of Change

Resolves #11617, e.g. before Windows Explorer wouldn't open when calling shell.showItemInFolder(fullPath) with a path that contains forward slashes. This PR makes the change of first normalizing the path that was passed in using NormalizePathSeparators().

Checklist

Release Notes

Notes: Fixed shell.showItemInFolder not opening Windows Explorer if the passed path contains forward slashes

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Mar 21, 2024
@piotrpdev
Copy link
Contributor Author

piotrpdev commented Mar 21, 2024

Not sure why this Mac test is the only one that's failing; it doesn't seem relevant to me.

EDIT: Looks like this is happening to others so probably a false positive: #41577 (comment)

electron/spec/webview-spec.ts

Lines 2110 to 2142 in 7609156

// TODO(miniak): figure out why this is failing on windows
// TODO(vertedinde): figure out why this is failing on mac arm64
ifdescribe(process.platform !== 'win32' && !isMacArm64)('<webview>.capturePage()', () => {
it('returns a Promise with a NativeImage', async function () {
this.retries(5);
const src = 'data:text/html,%3Ch1%3EHello%2C%20World!%3C%2Fh1%3E';
await loadWebViewAndWaitForEvent(w, { src }, 'did-stop-loading');
const image = await w.executeJavaScript('webview.capturePage()');
expect(image.isEmpty()).to.be.false();
// Check the 25th byte in the PNG.
// Values can be 0,2,3,4, or 6. We want 6, which is RGB + Alpha
const imgBuffer = image.toPNG();
expect(imgBuffer[25]).to.equal(6);
});
it('returns a Promise with a NativeImage in the renderer', async function () {
this.retries(5);
const src = 'data:text/html,%3Ch1%3EHello%2C%20World!%3C%2Fh1%3E';
await loadWebViewAndWaitForEvent(w, { src }, 'did-stop-loading');
const byte = await w.executeJavaScript(`new Promise(resolve => {
webview.capturePage().then(image => {
resolve(image.toPNG()[25])
});
})`);
expect(byte).to.equal(6);
});
});

@piotrpdev
Copy link
Contributor Author

Not sure who to ask for review on this (maybe @codebytere?), very small change anyway.

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! It also looks like shell.openPath might be subject to the same issue in the Windows impl? We should address that here as well if that's the case.

shell/common/api/electron_api_shell.cc Outdated Show resolved Hide resolved
@piotrpdev piotrpdev changed the title fix: normalize path before calling showItemInFolder fix: normalize path before calling showItemInFolder and openPath Mar 21, 2024
@piotrpdev piotrpdev force-pushed the showItemInFolder-normalize branch 2 times, most recently from d7556f7 to 8273670 Compare March 21, 2024 13:53
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

thanks!

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/28-x-y PR should also be added to the "28-x-y" branch. target/29-x-y PR should also be added to the "29-x-y" branch. target/30-x-y PR should also be added to the "30-x-y" branch. labels Mar 21, 2024
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Mar 22, 2024
@jkleinsc jkleinsc merged commit 90a7e5a into electron:main Mar 22, 2024
25 checks passed
Copy link

welcome bot commented Mar 22, 2024

Congrats on merging your first pull request! 🎉🎉🎉

Copy link

release-clerk bot commented Mar 22, 2024

Release Notes Persisted

Fixed shell.showItemInFolder not opening Windows Explorer if the passed path contains forward slashes

@trop
Copy link
Contributor

trop bot commented Mar 22, 2024

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

@trop
Copy link
Contributor

trop bot commented Mar 22, 2024

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

@trop
Copy link
Contributor

trop bot commented Mar 22, 2024

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

@trop trop bot added in-flight/30-x-y in-flight/29-x-y merged/29-x-y PR was merged to the "29-x-y" branch. merged/30-x-y PR was merged to the "30-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. and removed target/30-x-y PR should also be added to the "30-x-y" branch. target/29-x-y PR should also be added to the "29-x-y" branch. in-flight/29-x-y in-flight/30-x-y in-flight/28-x-y labels Mar 22, 2024
@piotrpdev piotrpdev deleted the showItemInFolder-normalize branch May 3, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/28-x-y PR was merged to the "28-x-y" branch. merged/29-x-y PR was merged to the "29-x-y" branch. merged/30-x-y PR was merged to the "30-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

shell.showItemInFolder won't open Explorer if forward slashes are present in path
3 participants