[path_provider] Add Windows support #2818

timsneath · 2020-06-05T19:29:38Z

Description

Implements path provider for Windows

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

packages/path_provider/path_provider/lib/path_provider.dart

stuartmorgan

Sorry for the review delay. I'm still trying to get Windows CI set up.

packages/path_provider/path_provider/pubspec.yaml

packages/path_provider/path_provider_windows/CHANGELOG.md

packages/path_provider/path_provider_windows/README.md

packages/path_provider/path_provider_windows/example/pubspec.yaml

stuartmorgan · 2020-06-17T21:35:48Z

packages/path_provider/path_provider_windows/lib/path_provider_windows.dart

+  /// On Windows, this the path specified by the TMP environment variable, or
+  /// the TEMP environment variable, or the USERPROFILE environment variable,
+  /// or the Windows directory, in order of preference. Windows does not
+  /// guarantee that the path exists or is writeable to.


I think we should handle existence here; path_provider users should be shielded from the platform-specific details, and they are unlikey to come read these docs. Returning a temp path that doesn't exist seems like an unnecessary foot-gun for developers.

packages/path_provider/path_provider_windows/lib/path_provider_windows.dart

technolion · 2020-06-26T08:31:18Z

Could you please add support for getDownloadsPath as suggested in this PR?
google/flutter-desktop-embedding#765

stuartmorgan

A couple more notes based on having recently been doing Windows plugin work.

It looks like we could have CI online soon (maybe next week?), at which point this will be unblocked. Will you have time to iterate on this PR soon? If not, should we do a handoff? I'd like to get it in ASAP after the CI is online so that we have something exercising the CI to prevent it from breaking without us noticing.

packages/path_provider/path_provider_windows/example/test_driver/path_provider_e2e.dart

stuartmorgan · 2020-09-02T18:23:22Z

packages/path_provider/path_provider_windows/lib/path_provider_windows.dart

+  /// files.
+  @override
+  Future<String> getApplicationSupportPath() =>
+      getPath(WindowsKnownFolder.ProgramData);


There's a mismatch here between what the Windows API gives and what a path_provider user is likely to expect; on mobile these folders are in the app sandbox, so it's reasonable to just create files in the directory returned by getApplicationSupportPath; in a Win32 app (unless running in a compatibility hypervisor, IIRC) it's the top level folder that all applications share.

For the FDE plugin what I did is append a directory using the executable name (from GetModuleFileName). I think we need do that (or something like it if there's a better value we have access to) there.

(We may want to add an API to path_provider_windows that allows people to manually set it to a value of their choosing, overriding the default of exe name.)

Wouldn't it make sense to place all app data in the userprofile/AppData/local/AppName as default?

Oh, I missed that difference from FDE. I was using RoamingAppData; that (or LocalAppData) seems like the right location for application support data. ProgramData sounds like it's not user-specific.

@timsneath I've updated this to match FDE (using RoamingAppData, and appending the exe name), ~~with a placeholder for the exe name until win32 is updated to GetModuleFileName or GetCurrentProcess~~. Let me know if you have any concerns about using that path.

packages/path_provider/path_provider_windows/test/path_provider_windows_test.dart

timsneath · 2020-09-03T19:18:35Z

Will you have time to iterate on this PR soon? If not, should we do a handoff? I'd like to get it in ASAP after the CI is online so that we have something exercising the CI to prevent it from breaking without us noticing.

Thanks for the prod (and for the pull request review comments). It's time I picked this bit up again (got distracted with Windows Runtime API interop improvements). I'll take a look over this Labor Day weekend and see if I can patch this into good shape.

…dows

stuartmorgan

(Marking as "reviewed" so it doesn't continue to indicate that the PR is waiting for changes I requested.)

stuartmorgan · 2020-09-16T02:28:18Z

@csells @cyanglaz Since I made non-trivial changes to the PR, I'll wait for one of you to review it before landing.

csells

all of it looks good except I'm not sure about the need for a Windows-specific sample

csells · 2020-09-16T03:32:18Z

packages/path_provider/path_provider_windows/example/README.md

@@ -0,0 +1,8 @@
+# path_provider_windows_example


do we need a separate sample for Windows? won't the normal path_provider sample do?

There's an open question in the longer term about how to best structure the tests and examples for federated plugins. The plugin repo tooling expects them though, and it does have value in the general case:

It's a self-contained way to build and test the plugin, which is convenient for plugin development.

It can test platform-specific APIs (which we don't currently have, but could in the future).

So long term, TBD, short term, this is the standard practice we've been following.

stuartmorgan · 2020-09-16T16:41:28Z

Per #2988 (comment) the application support path approach here isn't correct. I'll need to update the template to add VERSIONINFO, and read that here.

stuartmorgan · 2020-09-16T16:46:31Z

@timsneath It looks like we'll need GetFileVersionInfo, GetFileVersionInfoSize, and VerQueryValue in win32 now as well, to read this information.

timsneath · 2020-09-16T18:11:07Z

Have a PR ready that will add these APIs, but I need to add some tests before I merge and publish. I'll do that by EOD. dart-windows/win32#103

timsneath · 2020-09-17T18:03:51Z

OK, version APIs are all in win32 v1.7.1 (https://timsneath.github.io/win32/topics/version-topic.html)

Follows the full convention of company-name\app-name\ as the subdirectory, using the VERSIONINFO resource if there.

stuartmorgan · 2020-09-17T20:32:08Z

@csells Please re-review for the new AppData subdirectory logic.

stuartmorgan · 2020-09-18T00:29:48Z

I just wasn't aware of it. Updated.

I'm going to have to revert this; it appear that it makes the flutter/plugins repo tooling think the tests have failed:

RUNNING path_provider/path_provider_windows tests...
Running "flutter pub get" in path_provider_windows...               1.4s
No tests ran.
SKIPPING path_provider/path_provider_windows/example - no test subdirectory



Tests for the following packages are failing (see above):
 * path_provider/path_provider_windows

This wasn't happening with the skip option 🤷

This reverts commit 80a0b6a.

This reverts commit 1685a28.

Implements path_provider for Windows, adding path_provider_windows and endorsing it in path_provider.

Tim Sneath added 8 commits June 3, 2020 22:28

Initial Windows repo

7e018b9

Implementation for getTemporaryPath() and getDownloadsPath()

712c043

Tidy up Windows plugin implementation

55429f6

Add folder documentation comments.

52d5b0d

Add Windows runner

aea6a4c

Fix doc comment nit

f20a00e

Set pubspec version

1be04d7

Add manual registration of Windows plug-in

306868e

timsneath added the WIP label Jun 5, 2020

timsneath requested a review from stuartmorgan June 5, 2020 19:29

googlebot added the cla: yes label Jun 5, 2020

Tim Sneath added 5 commits June 5, 2020 19:29

Fix bug in pubspec

cd0ea88

Tweaks

0eba356

Remove windows folder

06bd98f

Add missing copyright comment

383ef8f

More minor copyright tweaks

ad0c2b9

renefloor reviewed Jun 12, 2020

View reviewed changes

packages/path_provider/path_provider/lib/path_provider.dart Show resolved Hide resolved

Tim Sneath added 2 commits June 16, 2020 21:57

Add warning

f264046

Bump changelog

03a126e

stuartmorgan suggested changes Jun 17, 2020

View reviewed changes

stuartmorgan mentioned this pull request Jul 11, 2020

[Windows] Support offline font caching on windows material-foundation/flutter-packages#70

Closed

stuartmorgan suggested changes Sep 2, 2020

View reviewed changes

stuartmorgan reviewed Sep 2, 2020

View reviewed changes

packages/path_provider/path_provider_windows/test/path_provider_windows_test.dart Outdated Show resolved Hide resolved

Tim Sneath added 4 commits September 4, 2020 14:21

Wrap pointer allocation in try/free

5057d47

Merge remote-tracking branch 'upstream/master' into path_provider_win…

9da128b

…dows

path endsWith

37eba73

Trim back comment on TEMP environment variable

a3a2d41

stuartmorgan mentioned this pull request Sep 8, 2020

[shared_preferences] Add shared_preferences_windows #2988

Merged

13 tasks

stuartmorgan reviewed Sep 16, 2020

View reviewed changes

stuartmorgan approved these changes Sep 16, 2020

View reviewed changes

csells reviewed Sep 16, 2020

View reviewed changes

Use 0.0.2, since there were code changes

cd2f366

Use VERSIONINFO to figure out the appdata subdir

7d46f63

Follows the full convention of company-name\app-name\ as the subdirectory, using the VERSIONINFO resource if there.

timsneath requested a review from matthew-carroll as a code owner September 17, 2020 20:30

csells approved these changes Sep 17, 2020

View reviewed changes

stuartmorgan added 2 commits September 17, 2020 16:05

Use TestOn

80a0b6a

Cap directory component length

3a3fdb7

stuartmorgan added 5 commits September 17, 2020 17:30

Revert "Use TestOn"

954a8df

This reverts commit 80a0b6a.

Update the test added after the revert change

be299b9

Use a named constant for the language code

e826b17

Make the name more accurate

370f3ce

Fix long path handling

fe6d472

renefloor mentioned this pull request Sep 18, 2020

Add Windows support or allow to pass in an initialized sqlite instance Baseflow/flutter_cache_manager#224

Closed

stuartmorgan merged commit 1685a28 into flutter:master Sep 18, 2020

timsneath deleted the path_provider_windows branch September 18, 2020 14:12

nabil6391 added a commit to nabil6391/plugins that referenced this pull request Sep 18, 2020

Revert "[path_provider] Add Windows support (flutter#2818)"

cd89d40

This reverts commit 1685a28.

stuartmorgan removed the WIP label Sep 18, 2020

stuartmorgan mentioned this pull request Sep 18, 2020

[path_provider] Path provider windows updates #3049

Merged

13 tasks

danielroek pushed a commit to Baseflow/flutter-plugins that referenced this pull request Oct 1, 2020

[path_provider] Add Windows support (flutter#2818)

e4d4c4c

Implements path_provider for Windows, adding path_provider_windows and endorsing it in path_provider.

jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020

[path_provider] Add Windows support (flutter#2818)

aba492d

Implements path_provider for Windows, adding path_provider_windows and endorsing it in path_provider.

FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020

[path_provider] Add Windows support (flutter#2818)

5077037

Implements path_provider for Windows, adding path_provider_windows and endorsing it in path_provider.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[path_provider] Add Windows support #2818

[path_provider] Add Windows support #2818

timsneath commented Jun 5, 2020 •

edited by stuartmorgan

stuartmorgan left a comment

stuartmorgan Jun 17, 2020

technolion commented Jun 26, 2020

stuartmorgan left a comment

stuartmorgan Sep 2, 2020

escamoteur Sep 3, 2020

stuartmorgan Sep 3, 2020

stuartmorgan Sep 15, 2020 •

edited

timsneath commented Sep 3, 2020

stuartmorgan left a comment

stuartmorgan commented Sep 16, 2020

csells left a comment

csells Sep 16, 2020

stuartmorgan Sep 16, 2020

stuartmorgan commented Sep 16, 2020

stuartmorgan commented Sep 16, 2020

timsneath commented Sep 16, 2020

timsneath commented Sep 17, 2020 •

edited

stuartmorgan commented Sep 17, 2020

stuartmorgan commented Sep 18, 2020

[path_provider] Add Windows support #2818

[path_provider] Add Windows support #2818

Conversation

timsneath commented Jun 5, 2020 • edited by stuartmorgan

Description

Related Issues

Checklist

Breaking Change

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan Jun 17, 2020

Choose a reason for hiding this comment

technolion commented Jun 26, 2020

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan Sep 2, 2020

Choose a reason for hiding this comment

escamoteur Sep 3, 2020

Choose a reason for hiding this comment

stuartmorgan Sep 3, 2020

Choose a reason for hiding this comment

stuartmorgan Sep 15, 2020 • edited

Choose a reason for hiding this comment

timsneath commented Sep 3, 2020

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan commented Sep 16, 2020

csells left a comment

Choose a reason for hiding this comment

csells Sep 16, 2020

Choose a reason for hiding this comment

stuartmorgan Sep 16, 2020

Choose a reason for hiding this comment

stuartmorgan commented Sep 16, 2020

stuartmorgan commented Sep 16, 2020

timsneath commented Sep 16, 2020

timsneath commented Sep 17, 2020 • edited

stuartmorgan commented Sep 17, 2020

stuartmorgan commented Sep 18, 2020

timsneath commented Jun 5, 2020 •

edited by stuartmorgan

stuartmorgan Sep 15, 2020 •

edited

timsneath commented Sep 17, 2020 •

edited