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

show file filter name for accessory view of file dialog #11959

Merged
merged 2 commits into from Apr 9, 2018

Conversation

Projects
None yet
5 participants
@yuya-oc
Copy link
Contributor

yuya-oc commented Feb 19, 2018

This PR would improve format picker of macOS file fialog.

  • Respect filters option of dialog.showOpenDialog() and dialog.showSaveDialog(). (#10335)
  • Show "All Files" for <input> apart from "accept" attribute. (#11456 and the original problem of #9745)

format_filter

dialog.showSaveDialog({
  filters: [
    {name: 'Images', extensions: ['jpg', 'png', 'gif']},
    {name: 'Movies', extensions: ['mkv', 'avi', 'mp4']},
    {name: 'Custom File Type', extensions: ['as']},
    {name: 'All Files', extensions: ['*']}
  ]
});

Unfortunately I could not find a way to automate tests.

@yuya-oc yuya-oc requested a review from as a code owner Feb 19, 2018

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Feb 24, 2018

@yuya-oc hey sorry for the delay on this; i'll review it within the next day or two so we can move forward with it :)

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented Feb 26, 2018

Preliminarily, I understand the changes you're making to this code, but i think it's not generally clear why so much code is necessary if this PR's only purpose is to show the file filter name as opposed to the extension.

Could you include a more thorough explanation of why some of the changes are necessary, and perhaps trim down some of the changes that don't add functionality like moving some of the lines around?

When that happens i think this will be ready to merge :)

@yuya-oc

This comment has been minimized.

Copy link
Contributor Author

yuya-oc commented Feb 26, 2018

@codebytere Thanks! Surely I also think my code is so much. Unfortunately I'm not good at Objective-C, so it's helpful if there is a good way.

https://github.com/electron/electron/pull/11959/files#diff-68a7264c657f5cfda99d3cdd12649277R55-68
In the master branch, filters' extensions are finally stored into a flatten NSArray. However I needed to create a NSArray of NSArray in order to keep filters' structure (array of array) when passing to PopUpButtonHandler. In that case, the conditions to show the accessory view or to allow all files(*) are also changed. So I got much changes for this PR finally. But this operation is simple operation of array. So should we create helper functions?

https://github.com/electron/electron/pull/11959/files#diff-68a7264c657f5cfda99d3cdd12649277R84
By changing if statement, I would be able to reduce amount of diff. But is it good?

if (count <= 1) return;

https://github.com/electron/electron/pull/11959/files#diff-68a7264c657f5cfda99d3cdd12649277L78
And I think the handler was initialized only at the first time. Probably this makes problem when showing dialogs many times or in multi-window apps. So I needed to change also this.

@yuya-oc yuya-oc force-pushed the yuya-oc:filter-for-mac-dialog branch from 3619af3 to e278f5e Mar 15, 2018

@yuya-oc

This comment has been minimized.

Copy link
Contributor Author

yuya-oc commented Mar 15, 2018

@codebytere Rebased and updated. As I wrote in my previous comment, unfortunately I have no more ideas to reduce amount of changes.

@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented Mar 22, 2018

It looks like we should merge this first, and then merge #11573 to fix the bug on accept attribute completely? Or is #11573 no longer needed if we have this?

@yuya-oc

This comment has been minimized.

Copy link
Contributor Author

yuya-oc commented Mar 22, 2018

I believe #11573 would be no longer needed. By merging this, All Files filter will appear for <input> which has accept attribute on macOS. It’s expected behavior of #9745.

@yuya-oc

This comment has been minimized.

Copy link
Contributor Author

yuya-oc commented Mar 25, 2018

I forgot to mention. #11954 is as well as #11573.

yuya-oc and others added some commits Feb 19, 2018

Show file filter name for accessory view of file dialog
- Respect filters option of dialog.showOpenDialog() and
  dialog.showSaveDialog(). (#10335)
- Show "All Files" for <input> apart from "accept" attribute. (#11456)

@zcbenz zcbenz force-pushed the yuya-oc:filter-for-mac-dialog branch from e278f5e to 41134f5 Apr 9, 2018

@zcbenz

zcbenz approved these changes Apr 9, 2018

@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented Apr 9, 2018

Manually tested this and it works great, thanks for fixing the problem beautifully.

@zcbenz zcbenz merged commit 0e5aaab into electron:master Apr 9, 2018

8 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@yuya-oc yuya-oc deleted the yuya-oc:filter-for-mac-dialog branch Apr 9, 2018

@ckerr ckerr added the semver/minor label Apr 24, 2018

@codebytere

This comment has been minimized.

Copy link
Member

codebytere commented May 1, 2018

/trop run backport-to 2-0-x

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented May 1, 2018

The backport process for this PR has been manually initiated, sending your 1's and 0's to "2-0-x" here we go! :D

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented May 1, 2018

We have automatically backported this PR to "2-0-x", please check out #12779

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