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

Improve performance when using SAF #1

Closed
wants to merge 1 commit into from

Conversation

FooIbar
Copy link

@FooIbar FooIbar commented Apr 24, 2024

No description provided.

@raxod502
Copy link

See also #2, which improves performance further by - as opposed to optimizing the documents traversal - avoiding it entirely.

@@ -210,8 +210,7 @@ public UniFile[] listFiles(FilenameFilter filter) {
final NamedUri[] uris = DocumentsContractApi21.listFilesNamed(mContext, mUri);
final ArrayList<UniFile> results = new ArrayList<>();
for (NamedUri uri : uris) {
String name = DocumentsContractApi19.getName(mContext, uri.uri);
if (name != null && filter.accept(this, name)) {
if (name != null && filter.accept(this, uri.name)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be this? Or should we still have the getName above? I tried checking out the branch locally and it failed to compile because name isn't declared.

Suggested change
if (name != null && filter.accept(this, uri.name)) {
if (uri.name != null && filter.accept(this, uri.name)) {

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! Yes, it should be uri.name.
Anyway, Imma close this in favor of #2, those optimization can be implemented there.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok cool, I will do my best to merge in your changes to that branch.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oop, nevermind, since #2 got merged before I went back and updated things, it might make sense to re-open this PR so it can be merged separately.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #4 for that.

@FooIbar FooIbar closed this May 1, 2024
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.

2 participants