Guard FileView recursion against file nodes

[lobre]

This is an attempt to fix #1654.

Fixes #1662

[jeremypw]

This seems more complicated than necessary? I tried just checking that the item is a FolderItem in original line 224 and it seems to fix the problem. Is there a structure that fails with just this?

[lobre]

To prevent the crash, I suppose you are right. But to me, if I understand the code correctly, there might be situations where we overwalk the tree.

Imagine this tree:

/home/me/project/
└── backend/
    ├── aaaa/            (folder)
    ├── aaaa.test/       (folder)
    │   └── backend.test
    └── aaaa             (file)

Say find_path() is called with the target path /home/me/project/backend/aaaa.test/backend.test.

It iterates over the children of backend/. The first child is aaaa/.

Now, we wonder if the target path starts with this child's path. As /home/me/project/backend/aaaa is a textual prefix of /home/me/project/backend/aaaa.test/backend.test, I believe the answer is yes, even though aaaa/ is not an ancestor of aaaa.test/backend.test.

Because of that prefix match, we expand aaaa/, call load_children() on it, and recurse down into it, only to discover later that the target path doesn’t actually live there. That's unnecessary work every time two siblings share the same starting characters.

The extra separator-aware check I proposed insists on folder.path + / being the prefix, so /home/me/project/backend/aaaa/ does not match /home/me/project/backend/aaaa.test/....

With that in place, we skip aaaa/ entirely and go straight to aaaa.test/, which is the true ancestor.

I should admit that the code is a bit more complex, though.

[jeremypw]

Ah, I see - good point. I wonder whether we could use GLib.File.get_relative_path () rather than has_prefix () and check whether null is returned? Hopefully that avoids needing to check for DIR_SEPARATOR_S.

[jeremypw]

Using

                    if (expander.path != code_item.path &&
                        expander.file.file.get_relative_path (code_item.file.file) == null) {

                        continue;
                    }

seems to work. Unfortunately the get_relative_path function returns null when comparing a file to itself so we need an extra condition.

[lobre]

Good catch! get_relative_path is cleaner.

I have updated the PR with that approach.

Also, for optimisation, I have added the optional target_file parameter, which lets the initial call create the GLib.File once, and then pass the same instance down into each recursive call.

Don't hesitate if you see other possible improvements!

[jeremypw]

Nice optimization! LGTM now.

[jeremypw]

Nothing in the diff that would affect flatpak and it builds as flatpak OK locally so merging.