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

Choose export location #352

Merged

Conversation

TheLastProject
Copy link
Contributor

Should hopefully fix #349 by making the following changes:

  • Don't export to a fixed location, open the filebrowser with the filename LoyaltyCardLocker.csv pre-filled
  • Remove the import from default location button (as the default location selected by Android will be the same for both the import and export button)

This change assumes that every single Android device has a file manager available. I have yet to see an Android device where this is not the case.

@cazzoo
Copy link

cazzoo commented Jan 29, 2020

approved

@brarcher
Copy link
Owner

I have yet to see an Android device where this is not the case.

I'm wondering about devices which are running a variant of AOSP (such as LineageOS) and do not have Google's apps. When the application was first written I was running an AOSP build that did not have a built-in file viewer that could be triggered from an app. The 'default' location was a workaround.

Maybe at some point AOSP started shipping an embedded file browser. I recall another project of mine needed a file browser, and the only way to provide one for all the supported Android versions which also worked on AOSP was to use another open source project and embed the file browsing.

https://github.com/spacecowboy/NoNonsense-FilePicker

Android 6 did not have a formal one, as mentioned here:
https://www.howtogeek.com/231401/how-to-use-android-6.0%E2%80%99s-built-in-file-manager/

I think that Google now has its own file browser called Files. And, that probably will work if one has Google's apps installed.

Anyway, this change probably works for many users. But, i do not think it will work for everyone. Or, maybe I'm missing something?

@TheLastProject
Copy link
Contributor Author

I can only state that every LineageOS version I've ever used has a built-in file manager, but I understand your point.

I think the way to go here may be to try to call the file manager and if there is nothing to answer the intent, to then save to a fixed location. That way it'll work on all devices and everyone with a device modern enough to have a file manager will be able to choose the location themselves.

I would suggest that merging this even with the chance of breaking backup on some pre Android 6 devices with AOSP may be worth it, especially given Google's constant increasing limiting of direct file access and the feeling it may happen on all Android Q devices which will only increase in popularity (sadly, I don't have one myself and I don't have the processing power on my main device to properly run emulators). Still, I will look into trying to add a fallback at some point probably.

@brarcher
Copy link
Owner

brarcher commented Mar 8, 2020

You've convinced me. Let's pull this change in.

I notice that in the "Make linter happy" commit a string resource is removed that is used in the master branch. I had to remove that commit locally to build successfully. Can you update the PR to master? I'll pull the change in then.

@TheLastProject
Copy link
Contributor Author

Should be all good now :)

@brarcher
Copy link
Owner

brarcher commented Mar 9, 2020

Note to self: Travis-CI deprecated how they posted status back to pull requests. The build did trigger and pass, but status was not reported. Link to passing build: https://travis-ci.org/brarcher/loyalty-card-locker/builds/659888648

@brarcher brarcher merged commit 4321bf5 into brarcher:master Mar 9, 2020
@TheLastProject TheLastProject deleted the feature/choose_export_location branch October 24, 2020 12:02
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.

Error while exporting to CSV
3 participants