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

Conversation

@stuartmorgan-g
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g commented Sep 24, 2021

Brings file_selector_macos into flutter/plugins from FDE, with the following changes:

  • Refactored slightly to allow for unit tests of almost all of the native code
  • Added native unit test coverage
  • Translated to Swift, to follow repo conventions for macOS plugins
  • Added an in-package example (almost an exact duplicate of the app-facing version, but written against the platform interface, as is our current practice)
  • Moved to an in-package method channel. As part of that, moved the flattening of type groups from Swift to Dart.

Does not currently include native UI tests to allow for end-to-end testing (since Flutter integration tests can't be used). They should be added later (they are currently blocked on flutter/flutter#90673), but the unit tests give substantial coverage, making it substantially better to move the plugin now to get those tests running.

macOS portion of flutter/flutter#70221

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. (Note that 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.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

@stuartmorgan-g stuartmorgan-g marked this pull request as ready for review September 24, 2021 16:24
@stuartmorgan-g
Copy link
Contributor Author

@cbracken There are two options for review here:

  1. As a full review
  2. As diffs from what's in FDE, using the incremental commits

2 is complicated somewhat by the fact that after I had the unit tests in place I translated the plugin to Swift, but that's its own commit so it can be done as a side-by-side.

Either way:

  • You can pretty much ignore example/lib/ unless you have a strong desire to review it, because it's just a clone of file_selector/file_selector/example/lib/ with very minor changes to use a different interface (something we're trying to figure out a good solution for in federated plugins in general, but don't currently have).
  • All of example/macos/ is just boilerplate flutter create, other than the unit test file (and the addition of a unit test target)

@ditman ditman removed their request for review September 24, 2021 23:01
@ditman
Copy link
Member

ditman commented Sep 24, 2021

(Maybe some day :P)

@Hixie
Copy link
Contributor

Hixie commented Dec 15, 2021

What's the status of this PR?

@stuartmorgan-g
Copy link
Contributor Author

What's the status of this PR?

Waiting for @cbracken to have time to review it.

@godofredoc godofredoc changed the base branch from master to main January 6, 2022 22:44
import 'package:file_selector_platform_interface/file_selector_platform_interface.dart';
import 'package:flutter/material.dart';

/// Screen that shows an example of getDirectoryPath
Copy link
Member

@cbracken cbracken Jan 25, 2022

Choose a reason for hiding this comment

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

See comments on the Windows patch for the example.

@stuartmorgan-g
Copy link
Contributor Author

@cbracken This is ready for review:

  • I updated the example to match the Windows changes
  • I added the in-package method channel
  • I reworked the native code and tests to put a stub-able protocol between the plugin implementation and the FlutterPluginRegistrar because it turns out we can't always support both master and stable with direct stubs. Master added a new method that uses a new type recently, and a stub would have to implement that method... but can't because the type of one of its arguments doesn't exist on stable. (And more generally, we'd be broken any time a new method were added to FlutterPluginRegistrar, and would have to update the tests, which isn't great.)

Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

lgtm just two minuscule nits.

import 'package:flutter/material.dart';

/// Screen that allows the user to select a directory using `getDirectoryPath`,
/// then displays the selected directory in a dialog.
Copy link
Member

Choose a reason for hiding this comment

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

pedantic nit: s/ then/then/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Future<void> _saveFile() async {
final String fileName = _nameController.text;
final String? path = await FileSelectorPlatform.instance.getSavePath(
// Operation was canceled by the user.
Copy link
Member

Choose a reason for hiding this comment

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

Move comment a few lines down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦🏻

@stuartmorgan-g
Copy link
Contributor Author

For some reason I'm not able to add labels to this PR at the moment, so landing manually.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants