Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[file_selector] Add getDirectoryPaths implementation on macOS #6575

Closed
wants to merge 4 commits into from
Closed

[file_selector] Add getDirectoryPaths implementation on macOS #6575

wants to merge 4 commits into from

Conversation

VanesaOshiro
Copy link
Contributor

@VanesaOshiro VanesaOshiro commented Oct 14, 2022 •
edited
Loading

This PR adds the macOS implementation for retrieving multiple directories paths from a select folder dialog.

Issue: Support for selection of multiple directories, through desktop's native open panel, in 'file_selector' package

image
New option on example application

image
GetDirectoriesPaths

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@VanesaOshiro VanesaOshiro marked this pull request as draft October 14, 2022 15:49
@VanesaOshiro VanesaOshiro force-pushed the 66978-add-get-multiple-directories-in-macos branch from 50b7a3b to 843a89f Compare October 14, 2022 15:54
@adpinola adpinola mentioned this pull request Oct 14, 2022
Closed
@VanesaOshiro VanesaOshiro force-pushed the 66978-add-get-multiple-directories-in-macos branch from 843a89f to cde8ad2 Compare October 14, 2022 17:30
This was referenced Oct 14, 2022
Draft
Closed
@adpinola adpinola force-pushed the 66978-add-get-multiple-directories-in-macos branch from cde8ad2 to 693812d Compare October 14, 2022 20:35
stuartmorgan
stuartmorgan suggested changes Oct 17, 2022
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

This change needs changes to the native unit tests to cover the new functionality.

packages/file_selector/file_selector_macos/lib/file_selector_macos.dart Outdated Show resolved Hide resolved
packages/file_selector/file_selector_macos/pubspec.yaml Outdated Show resolved Hide resolved
packages/file_selector/file_selector_macos/macos/Classes/FileSelectorPlugin.swift Outdated Show resolved Hide resolved
@VanesaOshiro VanesaOshiro force-pushed the 66978-add-get-multiple-directories-in-macos branch 5 times, most recently from c48a6c6 to d8a826d Compare October 18, 2022 12:59
@VanesaOshiro
Copy link
Contributor Author

Changes applied, thanks!

@VanesaOshiro VanesaOshiro force-pushed the 66978-add-get-multiple-directories-in-macos branch from d8a826d to 41ee3b5 Compare October 18, 2022 15:18
@VanesaOshiro VanesaOshiro changed the title [file_selector] Add getDirectoriesPaths implementation on macOS [file_selector] Add getDirectoryPaths implementation on macOS Oct 19, 2022
@VanesaOshiro VanesaOshiro force-pushed the 66978-add-get-multiple-directories-in-macos branch 2 times, most recently from 05c06a0 to 308609a Compare October 19, 2022 16:52
@adpinola adpinola force-pushed the 66978-add-get-multiple-directories-in-macos branch from 308609a to e880dfe Compare November 2, 2022 12:43
stuartmorgan
stuartmorgan reviewed Nov 10, 2022
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

The comment sounded like this was ready for review, but it seems like it's still actually a draft since it doesn't appear this version was ever run for manual testing. Was this waiting for something on the review side before being completed?

packages/file_selector/file_selector_macos/macos/Classes/FileSelectorPlugin.swift Outdated Show resolved Hide resolved
...or/file_selector_platform_interface/lib/src/method_channel/method_channel_file_selector.dart Outdated Show resolved Hide resolved
@adpinola adpinola force-pushed the 66978-add-get-multiple-directories-in-macos branch from e880dfe to 64723e7 Compare November 11, 2022 14:01
@adpinola
Copy link
Contributor

adpinola commented Nov 11, 2022 •
edited
Loading

Hi @stuartmorgan, referring to your last comment:

The comment sounded like this was ready for review, but it seems like it's still actually a draft since it doesn't appear this version was ever run for manual testing. Was this waiting for something on the review side before being completed?

This PR is still in draft because it depends on #6572, the same occurs with #6573 which depends also on #6572.
#6572 has your approval but we are still waiting on the second one, so we can merge that and set this ready for review.

We tested this functionality directly from the branch and is working ok.

Thanks!

@VanesaOshiro VanesaOshiro force-pushed the 66978-add-get-multiple-directories-in-macos branch from 64723e7 to 2cba5a6 Compare November 14, 2022 12:33
@adpinola adpinola force-pushed the 66978-add-get-multiple-directories-in-macos branch 3 times, most recently from baf818e to b359016 Compare November 17, 2022 19:28
@adpinola adpinola force-pushed the 66978-add-get-multiple-directories-in-macos branch 2 times, most recently from 6349ceb to 722cbc2 Compare November 18, 2022 12:58
@VanesaOshiro VanesaOshiro force-pushed the 66978-add-get-multiple-directories-in-macos branch from 722cbc2 to 4fa2182 Compare November 18, 2022 13:19
@VanesaOshiro VanesaOshiro marked this pull request as ready for review November 18, 2022 13:52
eugerossetto and others added 3 commits November 22, 2022 11:18
@eugerossetto @adpinola
Add getDirectoriesPaths method to the file_selector_platform_interface
aff719c 
Add getDirectoriesPaths to method channel.

Increment version to 2.3.0

apply feedback

extract assertion method

apply feedback

Add getDirectoryPaths macOS implementation
@adpinola
Add getDirectoryPaths macOS implementation
2a2ec50
@adpinola
fix native tests
f18c7cf
@adpinola adpinola force-pushed the 66978-add-get-multiple-directories-in-macos branch from d9380d2 to 7cb1053 Compare November 22, 2022 15:16
@VanesaOshiro VanesaOshiro force-pushed the 66978-add-get-multiple-directories-in-macos branch from 7cb1053 to 32ca9d8 Compare November 22, 2022 15:26
@stuartmorgan
Copy link
Contributor

What's the state of this PR? Is it ready for re-review?

@adpinola
Copy link
Contributor

What's the state of this PR? Is it ready for re-review?

Yes, it is.

@stuartmorgan
Copy link
Contributor

Apologies for the churn, but this will need to be reworked for the just-landed #6902. I forgot that this was in flight when doing that migration, or I would have waited for this to land first.

@stuartmorgan
Copy link
Contributor

@VanesaOshiro Are you still planing on updating this PR?

@VanesaOshiro
Copy link
Contributor Author

@VanesaOshiro Are you still planing on updating this PR?

Hi @stuartmorgan! Sorry for the delay in the response, we are going to close this PR and we will open another one in the next few days to integrate the use of Pigeon.

@VanesaOshiro
Copy link
Contributor Author

Hi @stuartmorgan. We closed this PR and opened a new one integrating the use of Pigeon.
Please feel free to review it.

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@adpinola adpinola adpinola left review comments

@stuartmorgan stuartmorgan stuartmorgan requested changes

@cbracken cbracken Awaiting requested review from cbracken cbracken is a code owner

Assignees
No one assigned
Labels
p: file_selector platform-macos
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@VanesaOshiro @adpinola @stuartmorgan @eugerossetto