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

Change android App.config path #5237

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Change android App.config path #5237

wants to merge 4 commits into from

Conversation

KeyWeeUsr
Copy link
Contributor

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 in start.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?

@hottwaj
Copy link

hottwaj commented Jul 12, 2017

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?

@KeyWeeUsr
Copy link
Contributor Author

@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)),
Copy link
Member

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.

Copy link
Contributor Author

@KeyWeeUsr KeyWeeUsr Jul 22, 2017

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

@inclement
Copy link
Member

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?

@KeyWeeUsr
Copy link
Contributor Author

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).

@KeyWeeUsr
Copy link
Contributor Author

KeyWeeUsr commented Jul 24, 2017

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.

@KeyWeeUsr KeyWeeUsr added the Notes: API-break API was broken with backwards incompatibality label Jul 24, 2017
kivy/app.py Outdated
activity = autoclass('org.kivy.android.PythonActivity').mActivity
defaultpath = join(
activity.getFilesDir().getAbsolutePath(),
'app',
Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

@inclement
Copy link
Member

Seems good to me. I'll test it to check, but other than that I'm all for making the change.

@KeyWeeUsr
Copy link
Contributor Author

@inclement ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Notes: API-break API was broken with backwards incompatibality Platform: Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants