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

Conversation

@rodydavis
Copy link
Contributor

Description

Returns the correct path for MacOS, Windows and Linux when running Flutter on the desktop on the call getApplicationDocumentsDirectory().

Inspired by @lesnitsky

@rodydavis rodydavis changed the title [path_provider] - Adding Desktop Support for getApplicationDocumentsDirectory [path_provider] Add Desktop Support for getApplicationDocumentsDirectory May 6, 2019
@collinjackson
Copy link
Contributor

collinjackson commented May 6, 2019

Thanks for the PR!

If we're going to support this plugin on Linux and Mac, we should probably come up with a documented behavior for all the methods. Would you like to take a crack at the other ones?

It feels like the approach of doing some of the work in Dart and some work in native-land might become challenging to maintain over time, but I do see value in offering this package to desktop developers.

See also #1566

@rodydavis
Copy link
Contributor Author

@collinjackson Sure thing! I'll see what I can do.

The reason it is split right now between dart and native land is because there is no official way for plugin maintainers to add plugin desktop support that i am aware of, and each plugin comes but with their own way. If there was an official way to publish a plugin package to support desktop with platform channels then i agree it should go there an be in Dart.

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented May 9, 2019

The desktop platforms have platform channel support, it's just not stable yet. Working around that within the official plugin repository is not the right path forward here (and won't be possible in general, so this doesn't establish an approach that can be replicated). Instead we should be adding desktop support the right way, as the APIs stabilize and the tooling for pulling the native sides matures.

If people experimenting with desktop before things stabilize want to use a plugin, they can implement the native side locally using the current platform channel support, or in cases like this they can do the workarounds themselves in their Dart code above the level of the plugin's Dart code.

@lesnitsky
Copy link
Contributor

@AppleEducate seems like this one should live here for now

@stuartmorgan-g
Copy link
Contributor

@AppleEducate seems like this one should live here for now

An actual implementation of path_provider (#1716 for instance) could be staged in the FDE repo. This PR isn't a correct implementation though (${Platform.environment['HOME']}/.config is not the documents directory on macOS, for instance).

@lesnitsky
Copy link
Contributor

@stuartmorgan maybe worth moving #1716 to FDE repo? I wasn't aware of this PR and was about to start reimplementing the same. IMO having desktop "adapters" for plugins in FDE repo is totally working

@lesnitsky
Copy link
Contributor

This PR isn't a correct implementation

I should have been more specific in first comment, I meant actual implementation of path_provider in swift, so agree

@stuartmorgan-g
Copy link
Contributor

@stuartmorgan maybe worth moving #1716 to FDE repo?

Yes, I'll do that now. Honestly I just forgot I hadn't moved it (at the time it needed a Dart change since there was a Dart-side whitelist-style platform check on at least one method, so it wasn't trivial to move to FDE like the others were, and I never followed up on doing just that Dart change; it turns out to be gone now anyway).

@lesnitsky
Copy link
Contributor

lesnitsky commented Aug 28, 2019 via email

@ened ened mentioned this pull request Aug 29, 2019
13 tasks
@stuartmorgan-g
Copy link
Contributor

This PR isn't a correct implementation though (${Platform.environment['HOME']}/.config is not the documents directory on macOS, for instance).

@collinjackson I would recommend closing the PR given this.

@collinjackson
Copy link
Contributor

Closing per @stuartmorgan's recommendation.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants