Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[path_provider_windows] add getExternalStoragePaths() #7018

Closed
wants to merge 5 commits into from

Conversation

squarebrakets
Copy link

@squarebrakets squarebrakets commented Jun 30, 2024

This pr allows to get list of drives in a windows system using getExternalStorageDirectories();

List which issues are fixed by this PR. You must list at least one issue.
flutter/flutter#151059

Pre-launch Checklist

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

@squarebrakets squarebrakets changed the title [path_provider_windows] add getExternalStorageDirectories() [path_provider_windows] add getExternalStoragePaths() Jun 30, 2024
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Per comments below, it's not clear to me that the implementation here matches the expected behavior of this method.

Future<List<String>?> getExternalStoragePaths(
{StorageDirectory? type}) async {
if (type == null) {
return _getLogicalDrives();
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to correspond to the Android implementation, which is the de-facto standard for the behavior of the method (since it has previously been Android-only). From the Android docs:

Returns absolute paths to application-specific directories on all shared/external storage devices where the application can place persistent files it owns. These files are internal to the application, and not typically visible to the user as media.

This does not describe a top-level drive.

Shared storage devices returned here are considered a stable part of the device, including physical media slots under a protective cover. The returned paths do not include transient devices, such as USB flash drives connected to handheld devices.

This does not appear to be true of the implementation here.

Copy link
Author

@squarebrakets squarebrakets Jul 2, 2024

Choose a reason for hiding this comment

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

Returns absolute paths to application-specific directories on all shared/external storage devices where the application can place persistent files it owns. These files are internal to the application, and not typically visible to the user as media.

I assumed getExternalStorageDirectories() is to list storage drive roots to directly list users Storage. Last time I checked by default this function returns Phone and SD card roots. With proper permission this allows to list SD Card and Phone storage. According to.

/// Paths to directories where application specific data can be stored
/// externally.
///
/// These paths typically reside on external storage like separate partitions
/// or SD cards. Phones may have multiple storage directories available.
///

So to reflect that here getLogicalDrive is being used since default root directory is a C drive. Android only gives access to a folder that maybe similar to %USERPROFILE% directory. Difference is that windows does not have any restriction to all parts of the drive like android does.

USB flash drives connected to handheld devices.

User may opt to add custom partiion to their existing drive or may have multiple drives internally inside the machine or a SD card slot similarly to Phones' SIM/SD card slot.

Copy link
Author

Choose a reason for hiding this comment

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

If the androids behavior I have seen is unexpected then getLogicalDrive has to be removed

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.

It would be useful to understand the underlying problem you're trying to solve with this PR. The idea of "external storage paths" as it exists on Android doesn't really map well to Windows.

If you wouldn't mind describing what you're trying to use this for, perhaps there's an alternate solution.

@squarebrakets
Copy link
Author

squarebrakets commented Jul 13, 2024

It would be useful to understand the underlying problem you're trying to solve with this PR. The idea of "external storage paths" as it exists on Android doesn't really map well to Windows.

If you wouldn't mind describing what you're trying to use this for, perhaps there's an alternate solution.

I realize LogicalDrives deosn't really map to Androids storage media. So removing getLogicalDrives

@squarebrakets squarebrakets reopened this Jul 13, 2024
@stuartmorgan
Copy link
Contributor

It would be useful to understand the underlying problem you're trying to solve with this PR. The idea of "external storage paths" as it exists on Android doesn't really map well to Windows.
If you wouldn't mind describing what you're trying to use this for, perhaps there's an alternate solution.

I realize LogicalDrives deosn't really map to Androids storage media. So removing getLogicalDrives

The use case you provided in the associated issue is "Manage users drives from a flutter app"; that's the part of the PR you removed, so it's still not clear what the use case is here. Please provide (in the issue) the use case that is addressed by this PR, and an explanation of why getPath doesn't handle that case.

@squarebrakets
Copy link
Author

It would be useful to understand the underlying problem you're trying to solve with this PR. The idea of "external storage paths" as it exists on Android doesn't really map well to Windows.
If you wouldn't mind describing what you're trying to use this for, perhaps there's an alternate solution.

I realize LogicalDrives deosn't really map to Androids storage media. So removing getLogicalDrives

The use case you provided in the associated issue is "Manage users drives from a flutter app"; that's the part of the PR you removed, so it's still not clear what the use case is here. Please provide (in the issue) the use case that is addressed by this PR, and an explanation of why getPath doesn't handle that case.

getPath would handle these cases. So closing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants