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

Add functions for obtaining the default storage paths #1598

Merged
merged 2 commits into from Jul 28, 2019
Merged

Add functions for obtaining the default storage paths #1598

merged 2 commits into from Jul 28, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jan 17, 2019

We just discussed this on discord and the situation with the storage available on Android is a little bit confusing, so I read through the docs a little and wrote an additional storage module to add after #1528 is in. Still untested / to be polished

@ghost
Copy link
Author

ghost commented Jan 17, 2019

By the way I'm aware this might be covered by plyer, but this is so basic (especially app_storage_path()) that it really should be available easily without additional larger dependencies, and without this apps like even kivy actually (or mine, admittedly) may hardcode /sdcard which may not actually exist on some oddball devices!

Therefore, I think this should be added & documented in the p4a core android module (I will add documentation once the SDL2 pull request is in, since that one touches the doc section I'd also need to modify here so that will most likely leave me in conflict territory if I do that now)

Copy link
Member

@inclement inclement left a comment

Choose a reason for hiding this comment

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

Sounds good to me, I agree that this kind of thing is what the android module should be for. We've also just been talking about replacing the old cython stuff in the android module.

I added a couple of comments but not sure if you were still working on it anyway.

pythonforandroid/recipes/android/src/android/storage.py Outdated Show resolved Hide resolved
pythonforandroid/recipes/android/src/android/storage.py Outdated Show resolved Hide resolved
"primary external storage path"
)

def secondary_external_storage_path():
Copy link
Member

Choose a reason for hiding this comment

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

Am I right to read that this function is actually identical to the previous one?

Copy link
Author

Choose a reason for hiding this comment

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

the isExternalStorageRemovable condition is different, which marks the difference between internal and external storage

@inclement inclement changed the base branch from master to develop June 6, 2019 20:40
if p is not None:
if os.path.exists(os.path.join(p, "Documents")):
# Looks like it's properly mounted!
return True
Copy link
Member

Choose a reason for hiding this comment

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

Minor: simply go with:

return os.path.exists(os.path.join(p, "Documents"))

@AndreMiras
Copy link
Member

This is looking promising @Jonast what about fixing the little conflict and make it to the tree?

@ghost
Copy link
Author

ghost commented Jul 9, 2019

Sounds good, although it should still be tested (I haven't so far 😂 ) - I'll give it a try and report back, and anyone else is also open to do so

@ghost ghost added the need-testing label Jul 9, 2019
@ghost ghost changed the title [WIP] Add functions for obtaining the default storage paths Add functions for obtaining the default storage paths Jul 9, 2019
@AndreMiras
Copy link
Member

Thanks for updating, it looks good overall, but fails runtime with:

AttributeError: type object 'android.os.Build' has no attribute 'VERSION'

The fix is fairly easy and I'd be happy to test once you have updated the PR

@AndreMiras
Copy link
Member

Hi @Jonast I'm about to give that one another try. Could you also rebase upstream/develop?

@AndreMiras
Copy link
Member

I gave it a try and it's crashing. I'm working on a fix and I'll add commits to the PR.
One of the crash could have been easily caught by unit testing. I'll work on that too later

@ghost
Copy link
Author

ghost commented Jul 28, 2019

Let me know if you need branch access on the repo fork itself, I really wouldn't mind giving you! I want to know how these "adding a commit" things work 😄 and thanks for taking over, that's nice 🙂 (I can also work on some tests though if you want me to)

@AndreMiras
Copy link
Member

I think access to your fork is only if I want to push force, for adding commits it should be just fine now.
I'll let you know.
I can work on the unit test I don't mind and I know what broke hence what to test.

1) fixes isExternalStorageRemovable is expecting a file object, error was:
```
07-28 16:36:46.845  2152  2784 I python  :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/python-installs/etheroll/android/storage.py", line 61, in primary_external_storage_path
07-28 16:36:46.847  2152  2784 I python  :    File "jnius/jnius_export_class.pxi", line 1034, in jnius.jnius.JavaMultipleMethod.__call__
07-28 16:36:46.848  2152  2784 I python  :  jnius.jnius.JavaException: No methods matching your arguments, available: ['()Z', '(Ljava/io/File;)Z']
```

2) fixes `os.path.exists()` doesn't accept `None`, error was:
07-28 22:31:25.877 31203 31313 I python  :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/python-installs/etheroll/android/storage.py", line 101, in secondary_external_storage_path
07-28 22:31:25.878 31203 31313 I python  :    File "/home/andre/workspace/EtherollApp/.buildozer/android/platform/build/build/other_builds/python3-libffi-openssl-sqlite3/armeabi-v7a__ndk_target_21/python3/Lib/genericpath.py", line 19, in exists
07-28 22:31:25.879 31203 31313 I python  :  TypeError: stat: path should be string, bytes, os.PathLike or integer, not NoneType

3) fixes on services `PythonActivity.mActivity` is None, refs kivy/kivy#6388
@AndreMiras
Copy link
Member

I edited through the GitHub UI, I hope I didn't do a mess 😁
I'm going to re-test it from scratch now see how it goes. If things run OK and build is green I'll merge

Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

Runtime is just working perfectly. Here're some trace for outputs from both Python app and service.

07-28 23:19:09.647 10894 11303 I python  : app_storage_path: /data/user/0/com.github.andremiras.etheroll/files
07-28 23:19:09.661 10894 11303 I python  : primary_external_storage_path: /storage/emulated/0
07-28 23:19:09.665 10894 11303 I python  : secondary_external_storage_path: None
07-28 23:19:09.665 10894 11303 I python  : persistent_keystore_path: /sdcard/etheroll
07-28 23:19:09.665 10894 11303 I python  : non_persistent_keystore_path: /data/user/0/com.github.andremiras.etheroll/files

07-28 23:19:10.407 11414 11430 I service : app_storage_path: /data/user/0/com.github.andremiras.etheroll/files
07-28 23:19:10.412 11414 11430 I service : primary_external_storage_path: /storage/emulated/0
07-28 23:19:10.417 11414 11430 I service : secondary_external_storage_path: None
07-28 23:19:10.417 11414 11430 I service : persistent_keystore_path: /sdcard/etheroll
07-28 23:19:10.419 11414 11430 I service : non_persistent_keystore_path: /data/user/0/com.github.andremiras.etheroll/files

Code producing that output was:

from android import storage
app_storage_path = storage.app_storage_path()
print(f'app_storage_path: {app_storage_path}')
primary_external_storage_path = storage.primary_external_storage_path()
print(f'primary_external_storage_path: {primary_external_storage_path}')
secondary_external_storage_path = storage.secondary_external_storage_path()
print(f'secondary_external_storage_path: {secondary_external_storage_path}')
persistent_keystore_path = cls.get_persistent_keystore_path()
print(f'persistent_keystore_path: {persistent_keystore_path}')
non_persistent_keystore_path = cls.get_non_persistent_keystore_path()
print(f'non_persistent_keystore_path: {non_persistent_keystore_path}')

Will merge once Travis turns green

@ghost
Copy link
Author

ghost commented Jul 28, 2019

Neat! Actually looking forward to use this myself in my apps ❤️ it's an overdue addition for sure

@AndreMiras AndreMiras merged commit e21cd8b into kivy:develop Jul 28, 2019
@ghost ghost deleted the storage_android branch July 28, 2019 23:48
@Sahil-pixel
Copy link
Contributor

path of secondary storage is none ..please fix this
: secondary_external_storage_path: None

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