-
Notifications
You must be signed in to change notification settings - Fork 27
[draft] special external storage permission #68
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
base: main
Are you sure you want to change the base?
Conversation
d7406ce to
7d48fcd
Compare
- bump SDL to 2.32.0
- bump NDK to r27c
- bump AGP to 8.8.0
- bump Gradle to 8.10.2
- bump CMake to 3.31.5
- bump targetSdkVersion to 34
- add GitHub Actions Release support (Debug)
- add Special External Storage Permission support
- fix "assets not found" error on first launch
- drop Android 4 support
| // I tend to just copy all assets on every app launch, overwriting the old ones, | ||
| // so that I don't have to change appVersion every time I need to make sure that the newest assets | ||
| // are guaranteed to always be present. Also, when /storage/emulated/0/com.dishii.soh is used, the assets | ||
| // could be from a whole different version of the app and that wouldn't be detected (at least by this | ||
| // particular SharedPreferences), so my way always overwrites those. | ||
| // My way is also very slow to load at every startup, which is ok for | ||
| // apps that have only a few external assets, but for apps like this one that have | ||
| // a lot of external assets, the slowness is pretty severe. Let me know if that is not desirable | ||
| // and you would prefer it to work differently. | ||
| //deleteOutdatedAssets(); |
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.
Testing the changes now, the slowdown does seem to be pretty severe. I'm fine with just keeping it how it was before unless you had a better way. When SOH updates we also need to prompt the user to regenerate the otr file which is why I was deleting the old one.
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.
the slowdown does seem to be pretty severe
yes, i agree this is not ideal so i'll try to think of a way to improve this.
The problem is, that when using the special external folder, the assets are not deleted when the app is uninstalled. That means that users can hypothetically uninstall the app and install an older version of the app, exposing it to assets from the uninstalled newer version. Even if storedVersion were changed to also be stored in the special external folder instead of a SharedPreferences, to make it also persistent, if (currentVersion > storedVersion) would no longer be sufficient to guarantee the assets always match the current running app, so it would need to be if (currentVersion != storedVersion), or something else.
when SOH updates we also need to prompt the user to regenerate the otr file
I haven't tested an older and a newer version of SOH itself with my patches applied yet, just the current release, so I haven't tested whether the following codepath works on Android yet, but there is a codepath in SOH upstream that does that for oot.otr (the soh.otr currently gets overwritten every app launch by my version of setupFiles()).
Do you think we can just allow that codepath to activate and let it prompt the users to regenerate their oot.otr?
I'll test that in a while.
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.
In fact, instead of using a SharedPreferences to detect whether all the assets need to be re-extracted, do you think we could call a modified version of the CheckSohOTRVersion() function here (through JNI from Java -> to C/C++),
https://github.com/HarbourMasters/Shipwright/blob/eb6c0d9d29da4abdfe23ead81d53d7b765aba6ee/soh/soh/OTRGlobals.cpp#L953
and just change it so that instead of creating an error, on any failure it tells our Java code to run setupFiles(), re-extracting all the assets from the AssetManager including the correct soh.otr?
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.
Yeah that's a good idea.
Uh oh!
There was an error while loading. Please reload this page.