Skip to content

feat(android):add content uri support for File - #9807

Merged
NathanWalker merged 2 commits into
release/8.2.0from
feat/android-file-helper
Mar 5, 2022
Merged

NathanWalker merged 2 commits into
release/8.2.0from
feat/android-file-helper

Conversation

@triniwiz

@triniwiz triniwiz commented Mar 4, 2022

Copy link
Copy Markdown
Member

Allows the File class to handle content:// uris on android

@cla-bot cla-bot Bot added the cla: yes label Mar 4, 2022
@farfromrefug

Copy link
Copy Markdown
Collaborator

@triniwiz i feel like the Android File implementation is a bit dirty being done the way it is . Why not in file-system-access.android.ts ? It would be much cleaner and prevent the need for the global.isAndroid

Also is your code working with a file picked through Intent.ACTION_OPEN_DOCUMENT_TREE or Intent.ACTION_OPEN_DOCUMENT ? Seems weird that it would work without using DocumentContract.

Also it is a bit sad that your rewrite a way to read text in FileHelper and not use the same way in https://github.com/NativeScript/NativeScript/blob/master/packages/ui-mobile-base/android/widgets/src/main/java/org/nativescript/widgets/Async.java
My implementation here #9661 had the benefit of refactoring code and not have duplicated functions.
May i ask you decided not to build on top? It even had a native test app to run automatic test on the native impl

@triniwiz

triniwiz commented Mar 5, 2022

Copy link
Copy Markdown
Member Author

@farfromrefug this is a temp what I'm really working on is much larger that this , also I just noticed that read from the async method an it's a bit cleaner now

@triniwiz

triniwiz commented Mar 5, 2022

Copy link
Copy Markdown
Member Author

I don't see why it won't work with files picked with those , I believe the content resolver should be fine with those.

@NathanWalker
NathanWalker merged commit c68d002 into release/8.2.0 Mar 5, 2022
@NathanWalker
NathanWalker deleted the feat/android-file-helper branch March 5, 2022 02:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants