Feature/support numeric file sorting

[ej-sanmartin]

I'm implementing #4180 since I do prefer numeric sorting. Also, would love to help where I can!

These changes are backwards compatible and defaults to existing sorting behavior.

I was not to sure where in the preferences I should place this option so i opted to putting it under the General -> Saving and Loading since it was relatively a general preference and I didn't want to make another section just for File / Folder related settings.

Let me know any changes I'd need to make, looking forward to your team's feedback.

[bjorn]

Let me know any changes I'd need to make, looking forward to your team's feedback.

Ah, I wish I had a team providing feedback sometimes, but for now it's usually just me. :-)

I think the change is quite fine, though I don't like that a re-scan is triggered when changing the option. Now of course re-scanning is generally quite fast, but we could avoid it by wrapping the ProjectModel in a sorting proxy model (a QSortFilterProxyModel subclass) in ProjectView. Is it something you'd be interested to look into?

In order to still efficiently sort directories first, a bool isDir should then be added to FolderEntry (and made available to the proxy, could be done using a custom data role either for that boolean or for getting a FolderEntry*).

[ej-sanmartin]

I see, I thought I could get away with the re-scan speed being so fast haha I'll have to look into wrapping the sorting into a proxy model, though it should be relatively straightforward considering the main functionality is working.

I'll also ensure to still efficiently sort directories first. To clarify, do directories need this numeric sorting as well (ex, with numeric sorting enabled dir2/tile would be before dir10/tile. Numeric sorting disable would have this reversed)?

[bjorn]

To clarify, do directories need this numeric sorting as well (ex, with numeric sorting enabled dir2/tile would be before dir10/tile. Numeric sorting disable would have this reversed)?

Yes, I think when numeric sorting is enabled it should apply to both directories and files.

[ej-sanmartin]

Made some incremental changes, let me know if this new Proxy class is more what you envisioned!

[bjorn]

Made some incremental changes, let me know if this new Proxy class is more what you envisioned!

Yeah that's about it, I see it's really more trouble than I had expected due to the persistence for expanded folders, sorry about that! But I think it's still a better setup than before.

A few things to check (you can if you have time, but otherwise I'll have a look at this later):

• Kludge in ProjectView::setModel.
• I think the isValid check in ProjectView::selectPath should be moved until after the mapFromSource.
• I don't think ProjectView::onRowsInserted is handling the proxy indexes correctly.
• Probably we should just always use mCollator and toggle its setNumericMode based on the preference. Calling setSortLocaleAware(true) on the proxy model doesn't do anything for us since we're overriding lessThan anyway.

[ej-sanmartin]

No worries, we want to ensure this is done well lol I'll chip at this over the next day or so.

I don't think ProjectView::onRowsInserted is handling the proxy indexes correctly.

I'll dig into this more again but how is it handling the proxy indexes incorrectly?

[ej-sanmartin]

Also, added .vscode/ to .gitignore, noticed this may have been missing since my .vscode/ dir was checked in accidentally. Hope that's okay with you!

[bjorn]

I'll dig into this more again but how is it handling the proxy indexes incorrectly?

It's connected to the rowsInserted of the proxy model, so the index it receives is a proxy index. So it calls restoreExpanded with the proxy index, which in turn uses it to look up the path from the source model. Either the index needs to be mapped to the source model somewhere or the path needs to be looked up using a role.

In addition, restoreExpanded is recursive, but it uses model()->rowCount(parent) for which it should either map the index to the source model, or it should call rowCount on the proxy model.

[ej-sanmartin]

The index issue was a bit hard to wrap my head around at first but managed.

Also to slightly reiterate the commit message for removing the setModel() override:

The override was only called once in the constructor, where both mProjectModel and mProxyModel are already explicitly initialized. The defensive type-checking logic handled scenarios that never occur in practice and becomes fragile / complicated when adding proxies like we did in this change.

This didn't cause any noticeable performance issues or crashes while running.

Below, built these changes locally to show the sorting works:

Default sorting:

With new, natural sorting:

[bjorn]

Thanks @ej-sanmartin, that looks great!

I think the only thing that could use improvement is the placement of this option, which looks out of place under "Saving and Loading". I'll see if I can find a better place. Would it be too hidden in the Projects view context menu?

I think we can also default this to true. Should be rare enough that people don't find it helpful.

[bjorn]

Ah, in testing I noticed the context menu actions are broken and I get the message "Broken filename passed to function" printed to the console. Probably another place where indexes aren't mapped to the source model.

[ej-sanmartin]

It could make sense in the Interface section of Preferences, in the Behavior group. There contains more settings about how UI elements interact and respond.

It's a tad hidden in the Project view Context menu, and it feels slightly off since the options I see there are folder related ("Add Folder to Project" and "Refresh Folders".

And that makes sense, good thing you caught that in testing. I noticed, I actually started getting more crashes as I kept using my local build with these changes, I assume it's related. I'll make sure the proxy class and indexes are properly mapped + passing tests.

[bjorn]

@ej-sanmartin Thank you for this nice improvement!