-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[path_provider] Use the application ID in the application support path (#2845) #3077
[path_provider] Use the application ID in the application support path (#2845) #3077
Conversation
99c6a8c
to
d7abd60
Compare
packages/path_provider/path_provider_linux/lib/path_provider_linux.dart
Outdated
Show resolved
Hide resolved
@robert-ancell Is this something you are still planning to do? It's a breaking change (e.g., for shared_preferences), so the earlier the better, although I think we'll need to make it opt-in at this point since there's been more time for people to start depending on the current behavior. |
It's something we really should do, though I haven't been looking at it. I'll put it back on my TODO list. |
697e0bf
to
6b91be1
Compare
I've updated this to use the legacy directory name if it exists, and use the new one if not. This should mean existing apps keep working as expected. |
packages/path_provider/path_provider_linux/lib/src/path_provider_linux_stub.dart
Outdated
Show resolved
Hide resolved
The test failure seems unrelated to this PR - a trivial PR (#3774) also shows the same failure. |
packages/path_provider/path_provider_linux/lib/src/get_application_id_real.dart
Outdated
Show resolved
Hide resolved
packages/path_provider/path_provider_linux/lib/src/path_provider_linux.dart
Outdated
Show resolved
Hide resolved
packages/path_provider/path_provider_linux/lib/src/path_provider_linux.dart
Show resolved
Hide resolved
packages/path_provider/path_provider_linux/lib/src/path_provider_linux.dart
Outdated
Show resolved
Hide resolved
packages/path_provider/path_provider_linux/lib/src/path_provider_linux.dart
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you still planning on updating this with tests for the new functionality?
@robert-ancell Ping on this? I do still think this is something we should do, it just needs the final testing. |
Yes, will update this next week. |
aacae67
to
2ac7e58
Compare
Updated now with tests. I've rebased because there have been enough upstream changes to warrant giving it a whole look over again. |
flutter#2845) Use the existing executable named directory if it exists, to allow backwards compatibility to work.
2ac7e58
to
dc5f8b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits.
packages/path_provider/path_provider_linux/lib/src/path_provider_linux.dart
Outdated
Show resolved
Hide resolved
packages/path_provider/path_provider_linux/test/path_provider_linux_test.dart
Outdated
Show resolved
Hide resolved
packages/path_provider/path_provider_linux/lib/src/path_provider_linux.dart
Outdated
Show resolved
Hide resolved
@stuartmorgan is the "submit-queue" test failure related to this? Seems to be a video player issue. |
|
flutter#2845) (flutter#3077) Use the existing executable named directory if it exists, to allow backwards compatibility to work.
flutter#2845) (flutter#3077) Use the existing executable named directory if it exists, to allow backwards compatibility to work.
Description
Change getApplicationSupportPath to use the Linux application ID instead of the executable name.
The was landed, then reverted in #2845.
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.///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?