Adds Temporary Files and Temporary Directory #77907
Conversation
ScorpionInc
force-pushed
the
Add_Temporary_Directory_and_File_Access
branch
from
2047e3e to
170d092
Compare
Adds Temporary Directory to Godot via tmp://. Uses wide character type for path defined in OS class. Makes temporary files actually temporary(via mode_string and unlink()). Adds and changes mode flags for temporary file modes. Adds mode bit fields for ease of use. Changed mode_string to allow for use of new temporary flag. Uses platform specific functions to get temporary directory to remove usage of std::filesystem thus lowering required minimum C++ version.
ScorpionInc
force-pushed
the
Add_Temporary_Directory_and_File_Access
branch
from
e09a4cd to
658da47
Compare
|
All suggested changes have been applied. I have done a commit squash and would love to hear more feedback good or bad. There may be edge cases I missed, for example IDK if this would work properly for WASM builds, but as far as I can tell it should work and I'm ready for another review whenever available. Thanks everyone o/ |
|
The first answer I read on Stack Overflow on Android temporary directory lead me to the line ret = "/data/local/tmp"; in the os_unix.cpp file, however diving more into the api I think the getcachedir function call sounds more apt. I'll look into it thank you for the review. |
Implements Temporary Dir for Android using the cache path.
|
Looks like the hard part(Getting the cache path) is already implemented by os_android :) yay! Assuming this works like described here: https://developer.android.com/reference/android/content/Context.html#getCacheDir%28%29 then this little change should work. After the tests are verified good build for android I'll squash this commit. Any other changes or improvements out there? Thanks as always. |
|
I had thought that the android file access class inherited from unix like it's os class does. This doesn't seem to be the case however, so it looks like there's still some work left to do to properly implement android. I was going to use unlink() again as I see no reason it shouldn't work but I'll have to figure out this AndroidAssetManager first. If unlink is a problem or to use more platform specific calls I may use stuff from https://stackoverflow.com/questions/3425906/creating-temporary-files-in-android to flag opened files as temporary but IDK yet. Sorry if I'm commenting to much just wanted to share that I'm still working on this. |
|
So it seems (as far as my noob self can tell) file_access for the Android platform is only able to read assets which are read-only compressed files packed with the .apk. Editing assets is by definition uneditable. Which is fine but I don't see any way to access files in the source that are not an "Asset". Do Android builds just not have access to file storage? I don't mind implementing something but I'm sure I'm missing something. |
Add method bind.
|
Android has two file access classes, one for APK resources, and one for generic files (wrapper over Java code). |
|
I think I get it its this other one the file_access_filesystem_jandroid right? How did I not see there are two. Thank you for your help. |
Update Documentation.
Adds native integer Kotlin Temporary flags and fields implementation.
I'm still a dumb. Should fix.
Java uses periods not double colons silly me.
Uses enum to int value function because enum is not castable to int.
Co-authored-by: bruvzg <7645683+bruvzg@users.noreply.github.com>
I didn't think this needed to be public as all instances are defined inside the enum but still getting errors so I'l try it.
Fixed tab usage instead of 4 spaces to maintain consistancy with the rest of the file. Changed to int instead of Int due to errors.
Perhaps its not the variable type its unhappy with but the lack of default value due to its final identifier? I was hoping to be able to define its value in the constructor. May have to make it mutable.
Complaining about the type again. IDK Maybe Int was fine? Might have to look something up.
I read that this may work and if it does would be way simpler.
Adds the new temporary access mode flags to MediaStoreData for file creation based upon mode. Also added to comments for Append mode to note that it does create files if they don't exist. Created function to return if files should be created if needed, this is to keep access mode relevant settings in the same file.
As per documentation: https://developer.android.com/reference/kotlin/android/content/ContentResolver#openfiledescriptor Should be wa not a for mode string
Attempts to make files actually temporary Android via the File.deleteOnExit() Function.
First is android 2nd is Java.
I'm so dumb
import the thingy.
|
Do you have CI set up on your repository? In order not to tax the CI here so heavily it'd be better in that case to do these rapid updates there on a separate branch and then push this one when ready, you've made 16 pushes in about 2 hours For example, create a branch for your own testing, rebase it on this one, and update this branch without pushing and rebase that other branch as needed and push it to test on your own CI, I've done this on several occasions where I don't want to tax CI here with rapid updates that depend on solving things I can't test locally |
Some formatting for legibility.
|
Sorry I didn't know it was a problem, I'm more or less done for now as I think I got the Android version working. I don't know what CI(CL?) is I assume its the build test things? Sorry I'm still a novice in several things I don't mean to be a hassle. |
|
It's okay! It can be hard to know It's Continuous Integration, it's the checks thing, if you look at your own branch you'll see a little yellow circle, cross, or checkmark, and you seem to have it running on your end so this should work to avoid this See here for the recent actions here on godot: https://github.com/godotengine/godot/actions |
|
I think I got it. I'm not to good at rebasing yet but I can make a fork of the fork to run the CI stuff and copy the changes over when it's done and that should do the same thing I think. Anyways I appreciate your understanding and patience. |
|
Of course! I did the same mistake when I was new with this |
Fixes a typo.
|
I believe with #98397 merged, this is obsolete now? |
Adds Temporary Directory to Godot via "tmp://".
Uses wide character type for paths defined in OS class.
Changed mode_string for Windows and Unix which makes temporary files actually temporary.
Adds and changes mode flags for temporary file modes. TEMPORARY_READ_WRITE ect.
Adds mode bit fields for ease of use. READ_FIELD, WRITE_FIELD, ect.
Uses a Macro define test as Clang lies about C++ version. So it just skips if clang(compile issues for iOS and MacOS).
Something to note:
Files created using the path header tmp:// may not be deleted immediately. tmp:// uses the OS path for temporary data. Files deletion using just the tmp:// header is OS specific.
Like wise temporary files can be created in user space user:// by using and access mode TEMPORARY_* these files will always be closed after all handles call close(), or on program close.
I had an issue with Apple builds not implementing C++17 code. This has been worked around but perhaps should be corrected in the build process?
Implements proposal:
#22363
This seems to be an older proposal that I didn't find reopened in the new godot-proposals project. However it was one that I liked and here it is for review and critiqe.
Shout out to the Godot team. Love you guys.