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: remove extra dot in extension #35618

Merged
merged 2 commits into from Sep 13, 2022
Merged

fix: remove extra dot in extension #35618

merged 2 commits into from Sep 13, 2022

Conversation

mlaurencin
Copy link
Contributor

@mlaurencin mlaurencin commented Sep 8, 2022

Description of Change

Closes #35594 #35429

Removes an extra period from the extension portion of the dialog filter on Windows. This is not expected in Electron's implementation of filters, but was in place for Chromium's. Updates #34723

Checklist

Release Notes

Notes: Added support for Windows drop-down dialog extensions

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 8, 2022
@mlaurencin mlaurencin changed the title fix: remove extra period of extension fix: remove extra dot in extension Sep 8, 2022
@mlaurencin mlaurencin added semver/patch backwards-compatible bug fixes target/18-x-y labels Sep 8, 2022
@mlaurencin mlaurencin marked this pull request as ready for review September 8, 2022 23:20
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 9, 2022
@Prinzhorn
Copy link
Contributor

Prinzhorn commented Sep 11, 2022

I wonder if there's a technical limitation (I don't really do c/c++) or if GetRegistryDescriptionFromExtension and FormatFilterForExtensions can be unit tested? They do look very testable. I'm also curious why this and the previous PR didn't have any release notes, but maybe I'm missing something?

@mlaurencin
Copy link
Contributor Author

@Prinzhorn Initially I didn’t add release notes as it was putting in place something standard to Windows functionality, but if people think one is warranted, I can certainly add one.

For unit testing, the issue did not arise from those functions, but rather in a misunderstanding of the difference in Chromium and Electron code paths. As for creating a test that would mimic the related issues, from what I can tell that would require manually switching between file types from the dropdown menu. I am a bit unsure as to how that sort of test can be constructed, but if someone has a suggestion, please let me know.

@Prinzhorn
Copy link
Contributor

Initially I didn’t add release notes as it was putting in place something standard to Windows functionality, but if people think one is warranted, I can certainly add one.

From #34723 I thought this was fixing a behavior that was previously considered a bug. I personally go through all release notes when I upgrade Electron and thought the Electron team would enforce release notes for bug fixes, that's why I was asking. And this PR is also fixing/closing two issues, so I think there certainly need to be release notes.

As for creating a test that would mimic the related issues, from what I can tell that would require manually switching between file types from the dropdown menu. I am a bit unsure as to how that sort of test can be constructed, but if someone has a suggestion, please let me know.

Fair enough, maybe this can only be manually tested with reasonable effort. I'm certainly the wrong person to answer that.

@ckerr
Copy link
Member

ckerr commented Sep 12, 2022

not ok 238 BrowserWindow module navigation events will-navigate event is triggered when a cross-origin iframe navigates _top

build-linux CI failure appears to be unrelated to PR

@ckerr
Copy link
Member

ckerr commented Sep 13, 2022

Quick, let's merge it while everything's green!!

@ckerr ckerr merged commit 12a7d7e into main Sep 13, 2022
@ckerr ckerr deleted the file-ext-period-fix branch September 13, 2022 18:47
@release-clerk
Copy link

release-clerk bot commented Sep 13, 2022

Release Notes Persisted

Added support for Windows drop-down dialog extensions

@trop
Copy link
Contributor

trop bot commented Sep 13, 2022

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

@trop
Copy link
Contributor

trop bot commented Sep 13, 2022

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

@trop
Copy link
Contributor

trop bot commented Sep 13, 2022

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

@mlaurencin
Copy link
Contributor Author

/trop run backport-to 21-x-y

@trop
Copy link
Contributor

trop bot commented Oct 13, 2022

The backport process for this PR has been manually initiated - sending your PR to 21-x-y!

@trop
Copy link
Contributor

trop bot commented Oct 13, 2022

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

@trop trop bot added in-flight/21-x-y merged/21-x-y PR was merged to the "21-x-y" branch. and removed in-flight/21-x-y labels Oct 13, 2022
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/21-x-y PR was merged to the "21-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: File extension appended multiple times to file name when saving
6 participants