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

WIP: Read-write ROSAF - to replace SAF #376

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lmagyar
Copy link
Contributor

@lmagyar lmagyar commented Sep 3, 2024

Note: This is currently based on the previous ROSAF PR #373 just to not cause merge conflict, I will rebase as it is necessary.

fixes #178

So I've started to experiment with it:

  • Uncommented your code
  • Added a minor non-working fix

Currently:

  • It can overwrite files and delete files and folders without any issue.
  • Renaming files and folders works, this was an emulator issue! Renaming fails, but executed in reality, after a refresh I can see the new name (somewhere something calls a getName() on the old filename Uri, my fix should have fixed it, it needs some investigation).
  • Creating new files/folders doesn't work.
  • Directory mode is rx not rwx.

My questions are:

  • Does this worth to pursue? You have nearly implemented it, still remained RO. Is there some show-stopper issue that blocks it? It would be a killer feature, a fast RW SAF!
  • RoSafSshFile.getParentFile() returns null, isn't this a problem somewhere???

Note for myself:

  • Modify RoSafFile.setLastModified() also.

@wolpi
Copy link
Owner

wolpi commented Sep 7, 2024

Ah, it is mainly about using DocumentContract. Back then as I wrote this I didn't find that.

In longer-term I would drop existing SAF code, keep "ROSAF" and call that one SAF. The only reason for calling it "ROSAF" as I could not find out back then how to implement wrting operations with "url based API".

@wolpi
Copy link
Owner

wolpi commented Sep 7, 2024

I see you had the same thought as I had 😄

@wolpi wolpi mentioned this pull request Sep 7, 2024
@lmagyar
Copy link
Contributor Author

lmagyar commented Sep 8, 2024

OK, it seems that writing also works, creating new files/folders fails "only", I will check it when I have time, this seems to be sooo close to work.

@lmagyar lmagyar force-pushed the pr-read-write-rosaf branch from 68eeb06 to 618a605 Compare September 26, 2024 15:26
@lmagyar lmagyar force-pushed the pr-read-write-rosaf branch from 618a605 to 31adacf Compare October 5, 2024 20:41
wolpi added a commit that referenced this pull request Oct 19, 2024
…oid calls to listFiles() which is very slow, but use cursor to list children, see GH issues #372 and #376
lmagyar pushed a commit to lmagyar/prim-ftpd that referenced this pull request Oct 19, 2024
…oid calls to listFiles() which is very slow, but use cursor to list children, see GH issues wolpi#372 and wolpi#376
@wolpi
Copy link
Owner

wolpi commented Oct 26, 2024

Note the changes to SAF in commit d0a8dc8.

That leads us to a situation where this PR would not be necessary anymore.

For now I leave RoSAF. To have that code still available for reference. If sombody has used to gain read-only access that would still be possible.

@lmagyar
Copy link
Contributor Author

lmagyar commented Nov 7, 2024

(I'm back. I was abroad and only the phone had with me.)

So. I've made some test, digged into Android source, and sadly that SAF change to replace listFiles() doesn't have too much effect. :(

  • roSAF is fast, because it uses 1 query/cursor and caches all the information in RoSafFile
  • old SAF is slow, because
    • listFiles() uses a query/cursor to query the IDs, then creates DocumentFile for each row (listFiles() source), then pftpd creates SafFile for each DocFile, and we access the properties (mode, size, mtime, etc) through DocFile's API, that uses separate queries for each property of each file (source)
    • in my test case it means 1000s of queries to list a folder vs. RoSaf's 1 query
  • new SAF is slow, because it does nearly exactly the same

The only difference I've found, that new SAF queries all the columns, and it seems that Android caches the results, so new SAF is a tiny little bit faster. Test results (Camera folder with ~500 files):

  • roSAF: ~1-2 sec (manual measurement, blazing fast, like poFS)
  • old SAF: 2:29 (yeah, 2 and half minutes)
  • new SAF: 2:12

So in my opinion to speed up SAF for folder listing, we have to cache the query's columns and don't use DocumentFile for the properties (size, etc.), only for delete(), createFile(), createDirectory(), renameTo() etc. It is quite hacky, and eg. renameTo() can only rename a file, can't move it, though it is possible (if I remember correctly) with the lower level API. In this case we have a code, that uses, let say 15% DocFile API and 85% DocContract API, and is 85% idetical with a refactored roSAF, so it doesn't look too great to me. :/

So I still prefer to refactor roSAF.

@wolpi
Copy link
Owner

wolpi commented Nov 9, 2024

ok, makes sense.
Would have been too easy 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fail to list folder with 600+ files on SAF R/W mode on external storage. [workaround found]
2 participants