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: modify file extension generation on Windows #34723

Merged
merged 12 commits into from Aug 2, 2022
Merged

fix: modify file extension generation on Windows #34723

merged 12 commits into from Aug 2, 2022

Conversation

mlaurencin
Copy link
Contributor

@mlaurencin mlaurencin commented Jun 23, 2022 •
edited

Description of Change

Closes #34450

Previously when saving a file on Windows, the file type would not be defined based on the extension of the file. (The linked issue also mentions that the file name should include the extension, but I could not replicate this on non-Electron applications). The file dialog will now properly display the file type and the saved file will include the extension.

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • tests are changed or added
  • relevant documentation is changed or added

Release Notes

Notes: None

@mlaurencin mlaurencin added semver/patch backwards-compatible bug fixes target/18-x-y labels Jun 23, 2022
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 23, 2022
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 24, 2022
@ckerr ckerr self-requested a review June 27, 2022 18:11
@mlaurencin mlaurencin changed the title [WIP] fix: modify file extension generation on Windows fix: modify file extension generation on Windows Jun 27, 2022
@mlaurencin mlaurencin marked this pull request as ready for review June 27, 2022 21:54
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 27, 2022
codebytere
codebytere reviewed Jun 28, 2022
View reviewed changes
Copy link
Member

@codebytere codebytere left a comment •
edited

Choose a reason for hiding this comment

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

It looks like there's still a bit of strange behavior here when showing extension and switching between potential save file types 🤔

If I open the html file from this gist in Edge on Windows, right click, and choose "Save Link As" - we get the following popup:

Screen Shot 2022-06-28 at 11 09 00 AM

The title of the item doesn't change if I then switch the file type to All Files:

Screen Shot 2022-06-28 at 11 11 24 AM

I would expect that if I ran the fiddle and clicked the "download" link, I would see a similar popup. At first, the behavior matches:

Screen Shot 2022-06-28 at 11 12 11 AM

However, if I switch the file save type to All Files, the title logic gets a little confused:

Screen Shot 2022-06-28 at 11 12 56 AM

and then if I switch back to PDF File, disappears altogether:

Screen Shot 2022-06-28 at 11 13 45 AM

I'd ideally like to see these match for a more intuitive user experience, and the i think we can do a final review for this :)

codebytere
codebytere requested changes Jun 28, 2022
View reviewed changes
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.

(oops see above, meant to req changes!)

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 28, 2022
@mlaurencin @ckerr
Update shell/browser/electron_download_manager_delegate.h
9b587d0 
Co-authored-by: Charles Kerr <charles@charleskerr.com>
zcbenz
zcbenz reviewed Jul 18, 2022
View reviewed changes
shell/browser/electron_download_manager_delegate.cc Outdated Show resolved Hide resolved
shell/browser/electron_download_manager_delegate.cc Outdated Show resolved Hide resolved
shell/browser/electron_download_manager_delegate.h Outdated Show resolved Hide resolved
shell/browser/electron_download_manager_delegate.cc Outdated Show resolved Hide resolved
shell/browser/electron_download_manager_delegate.cc Outdated Show resolved Hide resolved
@zcbenz zcbenz deleted the file-extension-fix branch August 2, 2022 00:40
@release-clerk
Copy link

release-clerk bot commented Aug 2, 2022 •
edited by MarshallOfSound

No Release Notes

@trop trop bot mentioned this pull request Aug 2, 2022
Merged
trop bot pushed a commit that referenced this pull request Aug 2, 2022
@mlaurencin @ckerr @electron-bot
fix: modify file extension generation on Windows (#34723)
ba353f7 
* fix: modify file extension generation on Windows

* modify includes

* include vector in header

* add win build flags

* remove hardcoded strings

* Update shell/browser/electron_download_manager_delegate.h

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* fix string manipulation and function definitions

* Update electron_download_manager_delegate.h

* convert to std::string and modify for electron

* Update shell/browser/electron_download_manager_delegate.cc

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* remove vector include and update conversion

* add vectr include for lint

Co-authored-by: Charles Kerr <charles@charleskerr.com>
trop bot pushed a commit that referenced this pull request Aug 2, 2022
@mlaurencin @ckerr @electron-bot
fix: modify file extension generation on Windows (#34723)
e7a55cb 
* fix: modify file extension generation on Windows

* modify includes

* include vector in header

* add win build flags

* remove hardcoded strings

* Update shell/browser/electron_download_manager_delegate.h

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* fix string manipulation and function definitions

* Update electron_download_manager_delegate.h

* convert to std::string and modify for electron

* Update shell/browser/electron_download_manager_delegate.cc

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* remove vector include and update conversion

* add vectr include for lint

Co-authored-by: Charles Kerr <charles@charleskerr.com>
This was referenced Aug 2, 2022
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Aug 2, 2022

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

@trop
Copy link
Contributor

trop bot commented Aug 2, 2022

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

@trop
Copy link
Contributor

trop bot commented Aug 2, 2022

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

jkleinsc pushed a commit that referenced this pull request Aug 2, 2022
@trop @mlaurencin @ckerr
fix: modify file extension generation on Windows (#35172)
eee147b 
fix: modify file extension generation on Windows (#34723)

* fix: modify file extension generation on Windows

* modify includes

* include vector in header

* add win build flags

* remove hardcoded strings

* Update shell/browser/electron_download_manager_delegate.h

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* fix string manipulation and function definitions

* Update electron_download_manager_delegate.h

* convert to std::string and modify for electron

* Update shell/browser/electron_download_manager_delegate.cc

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* remove vector include and update conversion

* add vectr include for lint

Co-authored-by: Charles Kerr <charles@charleskerr.com>

Co-authored-by: Michaela Laurencin <35157522+mlaurencin@users.noreply.github.com>
Co-authored-by: Charles Kerr <charles@charleskerr.com>
jkleinsc pushed a commit that referenced this pull request Aug 2, 2022
@trop @mlaurencin @ckerr
fix: modify file extension generation on Windows (#35171)
56a0b45 
fix: modify file extension generation on Windows (#34723)

* fix: modify file extension generation on Windows

* modify includes

* include vector in header

* add win build flags

* remove hardcoded strings

* Update shell/browser/electron_download_manager_delegate.h

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* fix string manipulation and function definitions

* Update electron_download_manager_delegate.h

* convert to std::string and modify for electron

* Update shell/browser/electron_download_manager_delegate.cc

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* remove vector include and update conversion

* add vectr include for lint

Co-authored-by: Charles Kerr <charles@charleskerr.com>

Co-authored-by: Michaela Laurencin <35157522+mlaurencin@users.noreply.github.com>
Co-authored-by: Charles Kerr <charles@charleskerr.com>
@zcbenz zcbenz mentioned this pull request Aug 31, 2022
Closed
3 tasks
@trippier1 trippier1 mentioned this pull request Sep 7, 2022
Closed
3 tasks
@mlaurencin mlaurencin mentioned this pull request Sep 8, 2022
Merged
3 tasks
@commit-lint commit-lint bot mentioned this pull request Oct 11, 2022
Open
schetle pushed a commit to schetle/electron that referenced this pull request Nov 3, 2022
@mlaurencin @ckerr @schetle
fix: modify file extension generation on Windows (electron#34723)
5f5dbd5 
* fix: modify file extension generation on Windows

* modify includes

* include vector in header

* add win build flags

* remove hardcoded strings

* Update shell/browser/electron_download_manager_delegate.h

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* fix string manipulation and function definitions

* Update electron_download_manager_delegate.h

* convert to std::string and modify for electron

* Update shell/browser/electron_download_manager_delegate.cc

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* remove vector include and update conversion

* add vectr include for lint

Co-authored-by: Charles Kerr <charles@charleskerr.com>
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@mlaurencin @ckerr @khalwa
fix: modify file extension generation on Windows (electron#34723)
b5f1c7c 
* fix: modify file extension generation on Windows

* modify includes

* include vector in header

* add win build flags

* remove hardcoded strings

* Update shell/browser/electron_download_manager_delegate.h

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* fix string manipulation and function definitions

* Update electron_download_manager_delegate.h

* convert to std::string and modify for electron

* Update shell/browser/electron_download_manager_delegate.cc

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* remove vector include and update conversion

* add vectr include for lint

Co-authored-by: Charles Kerr <charles@charleskerr.com>
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

@zcbenz zcbenz zcbenz approved these changes

@codebytere codebytere codebytere approved these changes

Assignees
No one assigned
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: File extension is missing on Windows
4 participants
@mlaurencin @ckerr @zcbenz @codebytere