-
Notifications
You must be signed in to change notification settings - Fork 289
Scoped storage implementation of Android 11+ and also bump older vers… #179
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
Conversation
…ions onto it if they have issues with the old code.
|
To note. I have some raw use in there for minimizing duplication. I'm sure that will totally freak out some people rofl. Its easy and quick enough to create duplication from it anyway if its not palatable. :) Just briefly tested clean build of the code pull so its all there and working. Possible someone has a device where some issues show up that I've not seen. Also did not run Firebase auto testing, monkey test, or other auto tests on this which I normally do for weeks. (Though of course I was, have been, and still am running automated backups using it.) |
|
One more thing I forgot to add. I intentionally "separated" the scoped code from what you had to not introduce any new issues and maintain backward compatibility as it is currently. Could then work on either as needed instead of dealing with even more conflicts and regressions. Since File isn't compatible with DocumentFile and scoped storage, I felt this was best. Didn't want outright full duplication so eliminated some of the worst spots for that. I don't plan on working on this or the app further. Might be possible that I will for a very short time. The code is fully working and no issues on my end. So I mostly leave this code in your hands to do as you like with it :) |
|
Great! Will look into getting this pulled in. |
Glad you did that! |
|
Now also briefly tested as good on Android 13 as well. It should be all good there anyway but now I got some actual use in :) |
|
Running new different application and seeing an app crash. Will likely look into it and tack that on. Edit: Fixed. Will send along soon-ish. There's something else I need to look into. |
I didn't expect that. Got a FTP client application rejecting the 250 but accepts the 257. The 257 is also used by a domain name registrar. Although, I see some seem to suggest that 250 can be used or both even lol. Kind of something like this: https://stackoverflow.com/questions/46201096/handling-babyftp-mkd-250-response-with-indy Edit: Looks like correct thing would be to adjust it to 257 which is of course supposed to be spec. But, it still is iffy as in 2008, Microsoft FTP server was sending MKD 250 also https://bugs.launchpad.net/bzr/+bug/224373 :) Any thoughts on that if you're not too busy? |
|
@Xavron It seems not to be working on Android 13... Tested with FileZilla |
|
Added in new commit 8fcae43 for various fixes. Edit - Forgot to add: [All tested and checks while using target of 33.] Fixed various path related issues plus checked all for conflicts and re-tested: Noted issues not fixed: |
I've been primarily testing on multiple Samsung devices and including Android 12 & 13 [local and sd card] and not sure what's happening there on your device. While you're using "write external storage", that is fully the OS there. It simply gives access and that access is obtained and the path is then automatically entered into the apps "manage users..." section since that can't be used with scoped storage. [That path wouldn't be doing anything different from Samsung device to Samsung device so it shouldn't be that.] I've not seen the other issues you mentioned either on Samsung devices using FileZilla. If you're not using "writing external storage" than that may be a reason. The scoped storage code I added only works when "writing external storage" is in use as that is scoped storage and what is required for Android 11+. The older code has various issues I've fixed on scoped storage only. If you have some specific error that can be traced back to the new code here then I can attempt to fix it. [Since there's too many possibilities of failure not involving the app, am not able to fix anything based on what you said since I can't reproduce it.] Edits: I do see scoped storage use retains access and still uses it at least on Android 13 after its disabled. May or may not be an issue on older devices. Not an issue on Android 13 anyway as it has to be used. FileZilla can fail to list folders where the file counts are too high verses its time out which can be set in FileZilla settings. LIST is fully working so am unsure what else is the issue there. Oh, this one doesn't have the notifications fix. That I'm not including with it. So there is that. The app does have an issue with the old storage perm check code on clean start with Android now denying that causing the app to exit (also app code doing that). I didn't test a clean start on Android 13 until now I guess. That's now fixed and not yet committed. [Oh, that's probably from the target of Android 13 so I shouldn't include it lol.] Not using or failing to use "write external storage" on clean app install will result in it failing to use starting directory since the app has no perm to use it. So that's correct. I do see an issue when sending 9k files. Something is happening in there. Am looking into it. |
Thank you so much for your detailed explanation. |
You welcome. Same here. Had it set to target 33 for a month or two now :) Its good timing though since I just finished fixing and testing some things today. Might as well fix another and another lol. But, still churning through the files trying to locate the cause of the last issue found. Looks like its going to take some time since its not happening often. 58 [known] times out of 8k. Edit - Found the issue. I'm not expecting any further ones in there but won't know for sure just yet. Will do extensive testing.But, apparently, my scoped storage code path traversal for getting to the proper URI location faster than Google's turtle one, kept going on the same dir once a match was found... but if that dir still contained a dir in the next sub then it would enter in there (in the wrong one) instead lol. D'oh! Guess you had a lot of those. I had none during testing of it. Should be fixed now. Have to fully test it, and if good, then probably within 24 hours sometime will commit it. |
…ever is left in the dir where its already been matched. If a dir exists at that point (that matched the next) then it would enter an incorrect dir and cause files to become incorrectly placed or to fail to transfer.
|
Commit 7d3424a Fixed:
Tests done [all using Android 13, sd card, & target of 33]: |
|
Commit ffbab8e Override wasn't being fully applied resulting in path related issues. Android 11+ were not affected and work fine on first use. Now Fixed: Affecting:
|
…ped storage code) not working on first attempt (or even until app was force ended at that).
NOT AN ISSUE. I probably didn't wait for it to all download as a second attempt was fine. HOWEVER. certain files do see as eg 240 bytes but downloaded are 246 bytes. Although they function fine, I don't like that and will look into it.Possible issue (needing to be looked into):
May be a specific issue with file bytes or very specific files in there somewhere. Further testing done:
|
|
Issue found: Android 13 target of 13 (not necessarily being from the target; not tested) FileZilla transfer down slows down to an incredible slow crawl with the screen off (even with swiftp CPU keep awake setting enabled and OS unrestricted battery enabled for the app). |
Maybe wake locks can do the job? Or |
I thought pre-Android 13 that unrestricted battery solved that so not really sure what's going on. However, the app already has the CPU wake lock and its still slow:
|
hmmmm, unrestricted battery or bettery optimization??
|
|
[SOLVED]
Looks like it no longer matters other than maybe to force not allow it lol. Enabled is full wake lock and slow. Disabled is partial and fast. {SOLVED} You were right on the pulse of it. Just the setting throws a mind bender :) People are going to get confused on that one if the setting remains as it is. |
I'm still unable to list directory Will attach screen recording below within 10 minutes :) |
At least this one Android no longer allows. Android's location picker [used with] write external storage will not allow that location directly. [That one is a lame part of Google's recent changes. I had to pull an app from that.] [There's nothing to fix there unless one creates a new branch not to be used with the Play Store and enables All File Access.]
This one works for me immediately on Samsung Android 13 in FileZilla when it has been selected during write external storage location chooser picker. So I can't reproduce it. |
|
[SOLVED] Bytes sometimes not the same on FileZilla file transfer down. FileZilla uses ASCII which adds some bytes where FileZilla settings > Transfer > FTP: File Types Binary choice transfer equals the same amount. So nothing to fix there. |
But it works very fine through this app -> https://github.com/wolpi/prim-ftpd on |
|
Also when adding new FTP user we are unable to change starting directory for the new user. Different users may use different starting location ? |
Normally, ones not on Play Store such as this one there, use all files access. That's the only way to get that exact location used now. I think dropping the target down low also may. But, I can't remember that one since I don't do things that way and the Play Store doesn't allow that anyway really. FYI, its no longer scoped storage when that is used. It has access to everything. That's not what scoped storage is and is more of a security risk. |
This one is that you pick the target location using write external storage. Then in each client app you set the path you want. Eg: or it could be: FileZilla has that under manage sites > site > advanced and there you can work with the paths. |
After.Enabling.External.Storage.mp4This video shows that even when enabling |
Unfortunately, I can't reproduce it. Scoped storage set to "/storage/emulated/0/DCIM/" gives that path back with permission. Its the exact path on all Samsung devices and works on my test Android 13 device immediately. I can't even think of any reason why it wouldn't produce files there :'( It could be possible that your FileZilla is dirty. Changing write external storage can cause FileZilla to mess up the paths. I have that noted way above. When you change write external storage you need to exit FileZilla. If it was a saved site then you'd also have to make sure the paths in there were correct too but it looks like you're using quick connect so only the former could be an issue. Although testing this now again and am not seeing an issue that way. |
I'm able to reproduce the issue. So:
After that FileZilla is unable to list folder and files again to the same folder where we upload the file from PC. |
|
Connecting to 192.168.192.209:2121... Also it doesn't work anymore even if we delete the previously uploaded file to that directory from phone file manager and that directory becomes unavailable forever. |
Thank you. But sadly, its not enough. I see the paths duplicating in your video but have no idea why. It could be new a variation needing to be dealt with in inputPathToChrootedFile() but there's not enough to go on unless I just randomly upload test code and I can't do that here lol. I will try yet another Samsung. If it still doesn't do it there then I got nothing. This one will be a Note 8. |
Something goes wrong after we upload the file to the phone and it make the directory unaccessible forever. |
|
Alright. You have an MSLD error in the video. That does show up just before a listing of a directory. So does files to list, get working directory variable, violation of chroot, and a while lot more that could cause that failure there. There's just not enough to go on and the note 8 is also fine. I have to be able to run it through the debugger. Or you need to provide what's happening in logcat. I did note the issue I've seen there previously in December though: which is the MLSD error string which is I did see it when write external storage wasn't being used. |
|
|
I was expecting that to help but its only showing MLSD is fine there :\ Until I can reproduce it, I can't do anything to fix it. Sorry :( Can't do certain other things here because of the branch being a pull request. [I will keep thinking about it and trying to find something. If there's anything to find.] |
|
This is another view on how i can reproduce the bug:
Any idea how can i debug it to find the root cause of why is this happening? BUG.mp4 |
Thanks :) But, I just reproduced it as well. Had to push a file to DCIM folder to do it. Its not done it with any other folder [I've used and did a bunch of testing on internal in Dec] so I'm wondering if its the app or Android thing. Have to debug it... will get back once I do that and determine what it actually is. Edit: It won't take long lol. Already got it working. Indeed, though the Google way didn't work for some reason. Since that was something that should not be called anyway here, and yet it was, it caused an issue. Will commit it in some minutes (in a bit).Thanks for your patience there and help :) |
|
Commit f3bb897 Fixed:
Tests: |
|
Guess I should have tested further :\ Test: Will look at that later. Can't do it right now. |
|
Another Bug(maybe). Everything seems to works fine except that after uploading something to a directory it also creates a folder named |
Yeah, its all path stuff. Technically, its all fixed now (without conflict or further extensive testing anyway) but won't be committing it for some time because I no longer like the code and need to test a lot. That will take a while to properly do. Update as an edit. Going great. Done in a short bit? I think so =) |
Yeah, good point 😜 |
…ode that's now working fine with it after things have been fixed in past commits and a couple more in this one. Didn't see that one coming.
|
commit 47efb9c Simplified path code with return to some older code that's now working fine with it after things have been fixed in past commits and a couple more in this one. Didn't see that one coming. |
|
Lot of internal changes! Scoped storage will be the way forward. |
|
Also made some little changes to setup to make Android 13 work out of the box. See |
|
I'll be continuing to monitor for any unforeseen issues and fix them as I can. |
…ions onto it if they have issues with the old code.
All final tests:
[x] == passed
[] == not tested; not looked into
[*] == couldn't connect on this device for unknown reason and not debugging this; not tested
Known issues:
Random notes: