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
Change android App.config path #5237
base: master
Are you sure you want to change the base?
Conversation
Thanks for looking into this. One issue with merging this is that anyone who released an android app using a prior version of Kivy which they subsequently update to a new Kivy version will find that already installed android apps will lose all settings when updated. Maybe a method could be provided that lets legacy apps copy across old settings to the new location? |
@hottwaj That's of course clear to me and it's the main reason why I haven't merged it and only proposed the change. Everything that depends on this location, in the sense that some files are pulled from there and not just saved, will be broken with just this fix. I just brought it up, so that it triggered some discussion as for "what next". |
kivy/app.py
Outdated
@@ -654,7 +661,10 @@ def get_application_config(self): | |||
''' | |||
|
|||
if platform == 'android': | |||
defaultpath = '/sdcard/.%(appname)s.ini' | |||
defaultpath = join( | |||
dirname(abspath(sys.executable)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't sys.executable
just ''
with python2 or android_python
on Android? It would probably be better to just check the environment variables (ANDROID_PRIVATE
) etc. to find the app's own directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works just fine with Pyonic on python 2 the env var is set in PythonActivity.java
+ start.c
, so we actually just need to make sure we don't change the core that launches the app (and if we change, it won't launch), so we should be quite safe with this one, because it'd be probably the first thing to fix.
Apparently there's a little change and that's a missing app/
in the path, so on py3 you get Context.getFilesDir() + '/app/.<appname>.ini
and on py2 Context.getFilesDir() + '/.<appname>.ini
Great! I think the target directory should be a little different, but this is an excellent change. Per the discussion elsewhere, I had thought it should be fine to first check if an old config file already exists in /sdcard, to avoid breaking existing apps. That seems like it should be safe, but I'm not sure. Do you think it would be fine? |
Hm, seems like a good idea. Maybe just check if something is in that place and pull it to the new one (+ remove?). Also, it's apparently the only thing that requires sdcard, or at least that's so obvious, that it uses it. The rest, such as using pyjnius for storage would mean the user knows what storage (s)he wants to access, therefore knows the permissions (i.e. not our problem to fix). |
I patched it, so that it detects the file on sdcard if available and moves to the new location. Then it ignores the old location completely. I guess it should be fine? Edit: Might and might not be an API break, but I just tagged it, so that it doesn't get lost until the next release. |
kivy/app.py
Outdated
activity = autoclass('org.kivy.android.PythonActivity').mActivity | ||
defaultpath = join( | ||
activity.getFilesDir().getAbsolutePath(), | ||
'app', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should omit the 'app', that way it isn't in the main application files directory (which is fine), and also will work if anyone happens to use it with old p4a versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated comment: I'm pretty sure we should omit the 'app'
. This should mean that the config does not get overwritten on app update. This is actually quite important.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@inclement I'm on it.
Seems good to me. I'll test it to check, but other than that I'm all for making the change. |
@inclement ping |
More about why is in the linked issue (#4777), but this change also does something "bad" in the sense that an ordinary user won't be able to edit and most likely even view the config file. On the other hand, the file will get removed by uninstalling the application and probably will be safe for updating the Kivy app (I think the
<app>/files/app
folder isn't removed with an update. Not sure though).This change removes the necessity of having a
WRITE_EXTERNAL_STORAGE
permission for the default Kivy app and if required, the dev can change the folder manually by overriding the method if the dev would like to create the file in/sdcard/<somewhere>
with adding the permission by own needs.I think
sys.executable
returns such a path because of this line which is used instart.c
to set the Python paths.I'm not sure, is this considered an API break if we change the path only and not the call directly? I mean, a dev should use the API we provide and not the raw path, so this should be safe, I guess?