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

PathColumn : Allow sorting of FileIconPathColumn #6012

Merged

Conversation

murraystevenson
Copy link
Contributor

This makes the FileIconPathColumn sortable via the CellData::sortValue property. This sorts by file extension, which is an approximation of sorting by file type, but is likely enough for our purposes.

@ericmehl In light of #6006, do you think we will need similar special handling here for non-english characters on Windows?

@murraystevenson murraystevenson self-assigned this Aug 22, 2024
Copy link
Member

@johnhaddon johnhaddon left a comment

Choose a reason for hiding this comment

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

I think this is fine (pending Eric's input on the Windows side), but I wanted to mention one thing in case it appeals at all : you could use the Qt API internally here if it's easier or better. The reason we couldn't do that for FileIconColumn is that it would have meant passing Qt types through the CellData API, but that's not necessarily a problem here if we can distill an int or string from the Qt query.

Sorting by file extension is an approximation of sorting by file type, but is likely enough for our purposes.

`std::filesystem::path::extension()` returns the last part of the filename from the rightmost `.`, which means "foo.tar.gz" will sort with "foo.gz", but this is preferable to a more greedy approach that would treat "foo.0001.exr" as having an extension of "0001.exr".
@murraystevenson
Copy link
Contributor Author

I wanted to mention one thing in case it appeals at all : you could use the Qt API internally here if it's easier or better...

Thanks, definitely worth considering. We could potentially use QMimeDatabase for this, though sorting by MIME type may be less useful than sorting by extension: .gfr, .txt, and .usda files would all sort together as they'd all likely identify as "text/plain". The most robust might be some combined sort value like extension:mimetype:stem, but the current extension:stem is likely useful enough...

@ericmehl any Windows concerns that we should address before merging? The latest push is just a rebase to resolve the Changes.md conflict.

@ericmehl
Copy link
Collaborator

ericmehl commented Sep 4, 2024

@ericmehl In light of #6006, do you think we will need similar special handling here for non-english characters on Windows?

I think we should be fine on this PR. Sorting non-English characters on Windows will work out the same as on Linux (once #6006 is solved and merged) since we'll be converting to UTF-8 (same as Linux uses throughout) in FileSystemPath itself, before the PathColumn sees the value.

So LGTM from that perspective.

@johnhaddon johnhaddon merged commit 6a6b20c into GafferHQ:1.4_maintenance Sep 6, 2024
6 checks passed
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.

3 participants