[file_selector] Add file group to save return value #4222

stuartmorgan-g · 2023-06-15T18:23:56Z

This deprecates getSavePath, which returned a target path string, in favor of a new getSaveLocation, which returns an object containing both a path and, optionally, a selected file group. This allows clients to use the selected group when deciding what path to use when saving (see discussion in linked issue).

This includes an implementation for Windows. It will also apply to Linux, and I've verified that the structure works, but it's not included here because it requires some non-trivial refactoring in the Linux implementation (we can't get the current index, only the current filter object pointer, which means we need to pass more data around between the various functions to map back to an index... and it's GObject so making internal data utility classes is fiddly.) For now Linux just always returns a null group, and we can add it later.

Most of flutter/flutter#107093

Pre-launch Checklist

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

cbracken · 2023-06-15T23:15:30Z

packages/file_selector/file_selector/test/file_selector_test.dart

+        confirmButtonText: confirmButtonText,
+      ),
+    ))
+        ?.path;


Was this intentional (or produced with dartfmt)?

dartfmt :( But @ditman's suggestion fixed it.

cbracken · 2023-06-15T23:22:29Z

...le_selector/file_selector_platform_interface/test/file_selector_platform_interface_test.dart

+
+class OldFileSelectorPlatformImplementation extends FileSelectorPlatform {
+  static const String savePath = '/a/path';
+// Only implement the deprecated getSavePath.


pedantic nit: indent

Fixed. I've never figured out when dart format doesn't fix comment indentation.

cbracken · 2023-06-15T23:28:59Z

packages/file_selector/file_selector_windows/example/lib/save_text_page.dart

+    if (!path.split(Platform.pathSeparator).last.contains('.')) {
+      final XTypeGroup? activeGroup = result.activeFilter;
+      if (activeGroup != null) {
+        path = '$path.${activeGroup.extensions!.first}';


Is the extensions list always guaranteed to contain at least one so long as there's an activeFilter? (That makes logical sense but it's a bummer we need to know it here)

This is dependent on the code 15-20 lines up where we define the groups. I'll add a comment explaining that so it's clear to a reader why it's safe.

cbracken · 2023-06-15T23:32:18Z

packages/file_selector/file_selector_windows/windows/file_selector_plugin.cpp

+    FileDialogResult result(files, nullptr);
+    UINT file_type_index;
+    if (SUCCEEDED(dialog_controller_->GetFileTypeIndex(&file_type_index)) &&
+        file_type_index > 0) {


😢 If this were pascal...

cbracken · 2023-06-15T23:34:03Z

packages/file_selector/file_selector_windows/windows/test/file_selector_plugin_test.cpp

  ASSERT_FALSE(result.has_error());
-  const EncodableList& paths = result.value();
+  const EncodableList& paths = result.value().paths();
+  EXPECT_EQ(paths.size(), 1);


Given the next line, arguably ASSERT_EQ.

Fixed everywhere.

cbracken

ditman

Wow, what a big PR for a relatively small fix/feature!

The PR LGTM (I skipped .g.dart and .mock.dart files). Web bits look good (since they're pretty much a Noop). cpp stuff looks... almost intelligible :P

Thanks for fixing this!

packages/file_selector/file_selector/test/file_selector_test.dart

.../file_selector/file_selector_platform_interface/lib/src/types/file_save_location_result.dart

stuartmorgan-g · 2023-06-16T20:39:42Z

I'll split out the first sub-PR shortly.

…ace (#4254) Platform interface portion of #4222 Part of flutter/flutter#107093

…#4273) Implementation package portion of #4222 Part of flutter/flutter#107093

…packages#4222)

flutter/packages@d041934...6b70804 2023-06-23 [email protected] [go_router] Reduces excessive rebuilds due to inherited look up. (flutter/packages#4227) 2023-06-23 [email protected] [image_picker] Update to 1.0 (flutter/packages#4285) 2023-06-23 [email protected] [tool] Consider comment-only changes to be dev-only (flutter/packages#4279) 2023-06-23 [email protected] [ci] Switch to LUCI for Android build-all (flutter/packages#4274) 2023-06-23 [email protected] [file_selector] Add file group to save return value (flutter/packages#4222) 2023-06-23 [email protected] [go_router] Adds onException to GoRouter constructor. (flutter/packages#4216) 2023-06-23 [email protected] [file_selector] Add file group to save return value - implementations (flutter/packages#4273) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

stuartmorgan-g added 11 commits May 31, 2023 08:28

WIP

fe1b7be

Split and comment

7566d9f

Pathify deps

9c8e359

Deprecate old version

3535dc2

Add initial passthrough implementations of the new method

31edcc4

Test the interface fallback

4f7da27

Add unit tests for higher levels

af2e10e

Merge branch 'main' into file-selector-save-extension-group

9ad7d01

Implement for Windows

cee261f

Bump versions

8d9a32f

Linux TODO

f315e22

stuartmorgan-g requested review from cbracken and ditman as code owners June 15, 2023 18:23

github-actions bot added p: file_selector platform-ios platform-linux platform-macos platform-web platform-windows labels Jun 15, 2023

cbracken reviewed Jun 15, 2023

View reviewed changes

cbracken approved these changes Jun 15, 2023

View reviewed changes

ditman approved these changes Jun 16, 2023

View reviewed changes

packages/file_selector/file_selector/test/file_selector_test.dart Outdated Show resolved Hide resolved

.../file_selector/file_selector_platform_interface/lib/src/types/file_save_location_result.dart Outdated Show resolved Hide resolved

Review feedback

7854c63

Add missed file

411be21

stuartmorgan-g mentioned this pull request Jun 20, 2023

[file_selector] Add file group to save return value - platform interface #4254

Merged

11 tasks

auto-submit bot pushed a commit that referenced this pull request Jun 21, 2023

[file_selector] Add file group to save return value - platform interf…

9af50d4

…ace (#4254) Platform interface portion of #4222 Part of flutter/flutter#107093

Merge branch 'main' into file-selector-save-extension-group

2ea2d71

stuartmorgan-g force-pushed the file-selector-save-extension-group branch from 693c7b3 to 2ea2d71 Compare June 22, 2023 14:24

stuartmorgan-g mentioned this pull request Jun 22, 2023

[file_selector] Add file group to save return value - implementations #4273

Merged

11 tasks

stuartmorgan-g added 4 commits June 22, 2023 10:37

Update dependencies

2af5085

Restore changes accidentally lost in the merge

c1c42be

Update readme excerpt

0e4a2e0

Update dependency

04fc894

auto-submit bot pushed a commit that referenced this pull request Jun 23, 2023

[file_selector] Add file group to save return value - implementations (…

6d8782d

…#4273) Implementation package portion of #4222 Part of flutter/flutter#107093

stuartmorgan-g added 3 commits June 23, 2023 12:04

Merge branch 'main' into file-selector-save-extension-group

df77e54

Revert iOS

8615f42

Update implementation versions

22353ab

github-actions bot removed platform-macos platform-ios platform-linux platform-web platform-windows labels Jun 23, 2023

stuartmorgan-g added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 23, 2023

auto-submit bot merged commit 25e1d87 into flutter:main Jun 23, 2023

engine-flutter-autoroll mentioned this pull request Jun 26, 2023

Roll Packages from d041934b296d to 6b70804799a9 (7 revisions) flutter/flutter#129559

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jun 26, 2023

25e1d87 [file_selector] Add file group to save return value (flutter/…

f8d9bba

…packages#4222)

This was referenced Jul 7, 2023

[Tool] New tool to download android dependencies #4408

Merged

[camera] tree fix format 2023 05 12 #4450

Closed

[tooling] mark infra steps as infra steps in yaml files #4473

Merged

reidbaker mentioned this pull request Jul 26, 2023

[Tooling] Add google owned cache for dependencies as an option in ci #4567

Merged

11 tasks

reidbaker mentioned this pull request Aug 4, 2023

[All] Expand artifact hub to all plugins #4645

Merged

11 tasks

[file_selector] Add file group to save return value #4222

[file_selector] Add file group to save return value #4222

Uh oh!

Conversation

stuartmorgan-g commented Jun 15, 2023

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbracken left a comment

Choose a reason for hiding this comment

Uh oh!

ditman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g commented Jun 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants