Skip to content

Conversation

@r0b0t3d
Copy link
Contributor

@r0b0t3d r0b0t3d commented Jan 7, 2020

Fix #206 #250

PathResolver is copied from rn-fetch-blob.
I modified a bit to allow download file if cannot get from content resolver.

With this PR, FIELD_URI will be the real file path.

@Elyx0
Copy link
Collaborator

Elyx0 commented Jan 15, 2020

It should be documented and download should be optional with a flag that's documented as well

@r0b0t3d
Copy link
Contributor Author

r0b0t3d commented Feb 13, 2020

@Elyx0 I added a flag getPath for android. Please take a look!

@anastely
Copy link

Any updates? I can't send an audio file to the server!

@rparet
Copy link
Contributor

rparet commented Mar 24, 2020

@vonovak can this be merged?

@JerakRus
Copy link

JerakRus commented Mar 31, 2020

@vonovak I tested this on my application. This is a great fix, and a wonderful job, thanks! Working with it in fileprovider is much more logical. And I would really like to be added to the master as quickly as possible. Can we do it ?
And I found 1 bug, but it will be much better with it than now.
In the screenshot below, there are 2 google drives (mobile - xiaomi redmi note 8 pro). The first one works great. And when I go to the second one, he first downloads the file in 1 section, this happens successfully. But then path: null returns to my application. Then I can go back to the picker, the file has already appeared in the 1 google drive section, I select it, and then it loads fine.

P.s. Sorry for my English, I use google translate)
photo_2020-03-31 20 09 59

@vonovak
Copy link
Member

vonovak commented Apr 8, 2020

hello folks. Thanks for this PR. I have to say I'm not in favor of merging such functionality into the module. I believe it'll be better if this module does one thing, and one thing only - and that is picking files and returning their uris and other info.

Once we start mixing in other functionality, like this one, it'll become much(!) more complicated, bugs will surface (one was already found in the post above!), and guess what - people will want them to be fixed.

Working with uris on android is a PITA especially given the plethora of places the files can come from, ranging from local drives to remote files, also considering the android fragmentation. iOS has its corner cases too, at least from my experience with background uploads.

The maintenance of this module is already a demanding task: 70+ issues that no one has time to fully address. IMO we should focus on the task of picking files and doing it right. What consumers do with those files is another thing and while I agree it'd be nice to include extra functionality into this module, I'm worried it'll lower its quality and become a maintenance issue. Speaking for this particular use case - I use this module and I have custom handling of returned uris in a background service (and I think many users will want to have it customized).

I hope this makes sense, feel free to discuss further.

@rparet
Copy link
Contributor

rparet commented Apr 8, 2020

@vonovak I agree with trying to keep things simple and functional. Unless, I'm mistaken without this PR there is no way to use document-picker and get an actual path to the file on Android. I'm specifically talking about the "ifusePath set to true, this value will be the file path from storage" part of this PR. This is used to support people uploading zip files, which then have to be unzipped and their contents exported, in the COVID Safe Paths app.

Example:
https://github.com/tripleblindmarket/covid-safe-paths/pull/282/files#diff-8f1480ce1ca7c746922d860b27734323R53

I don't have any comments on the "download remote file" functionality, but getting that file path is what's important to me.

@JerakRus
Copy link

JerakRus commented Apr 9, 2020

@vonovak
I fully support that the library should be as simple as possible in support, and I understand the pain of supporting many open PR. But I also support that the library should be cross-platform. In ios, we can upload files from icloud, the picer copies them to the cache, and then we can use them. On android, the analogy is google drive, and without this PR we do not have this opportunity. And moreover, as far as I understand, we generally have no way to get the file path and work with it further, for example, using RN-fetch-blob?

@vonovak
Copy link
Member

vonovak commented May 7, 2020

I believe the correct way forward would be to fix Blob support in react native core react-native-community/discussions-and-proposals#109

@r0b0t3d
Copy link
Contributor Author

r0b0t3d commented Jun 9, 2020

@JerakRus Just fixed the problem with Google Drive app you got above

@r0b0t3d
Copy link
Contributor Author

r0b0t3d commented Jun 14, 2020

@vonovak Any chance to get this PR merged? Basically, this just provide users a way to get local file path. Users can ignore this prop if they don't want to.
Beside, I saw new prop copyTo on iOS. So if possible, we could use that prop for Android and return local path as fileCopyUri. Thought?

@vonovak
Copy link
Member

vonovak commented Jun 14, 2020

hi! yes, I now agree we should merge something like this into the repo as you suggested. I guess if you fix the merge conflicts then we can give it a review and merge

@r0b0t3d
Copy link
Contributor Author

r0b0t3d commented Jun 16, 2020

@vonovak just merge to master to resolve conflict and get base branch to continue. I am working to update with copyTo

@r0b0t3d
Copy link
Contributor Author

r0b0t3d commented Jun 17, 2020

@vonovak I've done implementation. Please help to review!

Copy link
Member

@vonovak vonovak left a comment

Choose a reason for hiding this comment

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

hi! thanks a lot for working on this! I only left some minor comments / suggestion, can you please take a look? Thanks!

@Elyx0
Copy link
Collaborator

Elyx0 commented Jul 19, 2020

The way I see it for v4 is that we'd split into @rndp/essentials or @rndp/core and then add some addons after like @rndp/downloaders @rndp/theme. We'd have much more chance of making the ecosystem thrive on it while keeping specialized sub-scopes for community plugins.

@r0b0t3d
Copy link
Contributor Author

r0b0t3d commented Jan 20, 2021

Hi @vonovak , I updated the code. Please help review the changes. Thanks

@vonovak
Copy link
Member

vonovak commented Feb 6, 2021

@r0b0t3d thanks so much for this. Tested with android 6 and 10 devices, merging

@vonovak vonovak merged commit 02afc3f into react-native-documents:master Feb 6, 2021
DomiR added a commit to DomiR/react-native-document-picker that referenced this pull request Mar 30, 2021
* upstream/master: (22 commits)
  5.0.2
  fix: copyTo on android to put the file in a uniquely named dir (react-native-documents#397)
  5.0.1
  Fix copyFile on Android. (react-native-documents#392)
  chore: document RN blob support
  5.0.0
  support only android >=5 (API 21) (react-native-documents#386)
  [Android][Google Drive] Download file from google drive then cache in local storage (react-native-documents#264)
  4.3.0
  [Windows] Implementing Folder Picker support (react-native-documents#380)
  Fix crash on iOS (react-native-documents#383)
  chore: deleted duplicate test case (react-native-documents#375)
  chore: change readme wording
  4.2.0
  iOS : add mode option (react-native-documents#345)
  chore: update comment for closing issues (react-native-documents#373)
  chore: update discussions in readme (react-native-documents#372)
  chore: introduce GH discussions (react-native-documents#371)
  chore: add issue actions (react-native-documents#369)
  chore: update provided file type shortcuts
  ...
@fekajin
Copy link
Contributor

fekajin commented Apr 19, 2021

still happening same issue

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Permission denied selecting google drive file

7 participants