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

Add sorting support #73

Merged
merged 1 commit into from
Apr 18, 2021
Merged

Add sorting support #73

merged 1 commit into from
Apr 18, 2021

Conversation

sayanarijit
Copy link
Owner

@sayanarijit sayanarijit commented Apr 16, 2021

Closes: #58

@sayanarijit sayanarijit changed the title Add sorting support [WIP] Add sorting support Apr 16, 2021
@sayanarijit sayanarijit force-pushed the add/sorting-support branch 4 times, most recently from 84f1116 to 17c0751 Compare April 17, 2021 05:17
@sayanarijit sayanarijit changed the title [WIP] Add sorting support Add sorting support Apr 17, 2021
@sayanarijit sayanarijit marked this pull request as ready for review April 17, 2021 05:17
@sayanarijit
Copy link
Owner Author

Hey @maximbaz please review when possible.

@sayanarijit sayanarijit force-pushed the add/sorting-support branch 4 times, most recently from 676c7c4 to 4c4e743 Compare April 17, 2021 07:21
@sayanarijit
Copy link
Owner Author

image

@sayanarijit sayanarijit force-pushed the add/sorting-support branch from 4c4e743 to f318a92 Compare April 17, 2021 07:36
@maximbaz
Copy link
Contributor

Hello! Very happy to see this PR 🙂

  1. First of all, I just love the sort+filter panel on top, brilliant idea 👍

  2. I feel like icons should match:

    image

  3. With just relative search, shouldn't CODE... be after Cargo..., not before?

    image

  4. How to sort by absolute path? I see you have it in code, but not sure when that is enabled, i.e. how to test it.

  5. Sort by node type is currently looking like this:

    image

    I'm trying to think how to achieve such that directories and symlinks to directories are sorted together by relative path, and correspondingly files and symlinks to files are sorted together. It seems like we need to make sorter field in the config an array, instead of a single value, such that my sorting would become:

            messages:
              - AddNodeSorter:
                  sorter: [ByIsFile, BySymlinkIsFile, BySymlinkIsBroken]
              - AddNodeSorter:
                  sorter: [ByIsDir, BySymlinkIsDir]
              - AddNodeSorter:
                  sorter: ByRelativePath
              - Explore

    Do you agree, or am I overlooking something?

  6. What is ByIsSymlink? If we have symlinks to files, dirs and broken, what is that group?

  7. It seems I cannot override modes.builtin.sort in my config, nor add a new key in there.

Enough for now 😄

src/app.rs Outdated
NodeSorter::ByIsBroken => "§⨯",
NodeSorter::ByIsReadonly => "ro",
NodeSorter::ByMimeEssence => "mime",
NodeSorter::BySymlinkAbsolutePath => "§rel",
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a bit suspicious, relative or absolute?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Bug... Nice find...

@sayanarijit
Copy link
Owner Author

sayanarijit commented Apr 17, 2021

Hello! Very happy to see this PR 🙂

  1. First of all, I just love the sort+filter panel on top, brilliant idea 👍

Thanks. 😃

  1. I feel like icons should match:

The icons aren't tied to the app. I'll create separate customizable UI components for the sort and filter items.

  1. With just relative search, shouldn't CODE... be after Cargo..., not before?

The default sorting logic thinks all CAPS are of lower value that the small case, i.e. O < a. I think we can workaround this by first comparing case insensitively then with the proper case. But that will add performance cost. Not sure if it's worth it.

  1. How to sort by absolute path? I see you have it in code, but not sure when that is enabled, i.e. how to test it.

You can't sort nodes by abs path because alll the nodes will have the same parent. But you can sort by symlink's abs path using BySymlinkAbsolutePath. But I see what you mean i.e. to be able to sort by the node's and resolved symlink's abs path.

  1. Sort by node type is currently looking like this:

    I'm trying to think how to achieve such that directories and symlinks to directories are sorted together by relative path, and correspondingly files and symlinks to files are sorted together. It seems like we need to make sorter field in the config an array, instead of a single value, such that my sorting would become:

            messages:
              - AddNodeSorter:
                  sorter: [ByIsFile, BySymlinkIsFile, BySymlinkIsBroken]
              - AddNodeSorter:
                  sorter: [ByIsDir, BySymlinkIsDir]
              - AddNodeSorter:
                  sorter: ByRelativePath
              - Explore

    Do you agree, or am I overlooking something?

Nice grouping... I need to use more brain power.

  1. What is ByIsSymlink? If we have symlinks to files, dirs and broken, what is that group?

Haven't thought about grouping so far. So WIP.

  1. It seems I cannot override modes.builtin.sort in my config, nor add a new key in there.

Bug. Will be fixed.

Enough for now 😄

@maximbaz
Copy link
Contributor

maximbaz commented Apr 17, 2021

You can't sort nodes by abs path because alll the nodes will have the same parent. But you can sort by symlink's abs path using BySymlinkAbsolutePath. But I see what you mean i.e. to be able to sort by the node's and resolved symlink's abs path.

Aaah I see. To be honest I don't think it's needed, I simply saw that in code and was curious how to invoke that code path 🙂 Sorting by relative path is by far what I would use, personally.

The default sorting logic thinks all CAPS are of lower value that the small case, i.e. O < a. I think we can workaround this by first comparing case insensitively then with the proper case. But that will add performance cost. Not sure if it's worth it.

Aaaah I see... There is of course eq_ignore_ascii_case, but to support unicode you might want to look into lexical_sort library, e.g. I'd definitely go for natural_lexical_cmp to also have numbers sorted naturally (or natural_lexical_­only_alnum_cmp, also looks interesting)

@sayanarijit sayanarijit force-pushed the add/sorting-support branch from f318a92 to 8b52bd5 Compare April 17, 2021 17:49
@sayanarijit
Copy link
Owner Author

image

@sayanarijit
Copy link
Owner Author

sayanarijit commented Apr 17, 2021

I left the case sensitivity to default since it seems trivial. Grouping sorters will add immense complexity. So left it at flat, but added more parameters to be able to achieve more without needing to group things.

@maximbaz
Copy link
Contributor

I like the default sorting very much now 👍

I'm using exa with the arguments below (it's what I alias to la) and to me personally this is an ideal, very predictable, intuitive sorting, that's what I would like to try to get xplr to do (if it doesn't do this out of the box).

image

  • When symlink is relative, they show it as relative path
  • Intuitive sorting of "CODE" vs "Cargo" and all 20 "file_*" files
  • No need to group broken links separately

@sayanarijit
Copy link
Owner Author

Got a nice idea. I can just use exa as a library instead of duplicating all the sorting and display logic.

@sayanarijit
Copy link
Owner Author

Wow exa comes with a performance cost. Try exa in /nix/store.

@sayanarijit
Copy link
Owner Author

ogham/exa#851

@sayanarijit sayanarijit force-pushed the add/sorting-support branch from 8b52bd5 to d723b0c Compare April 18, 2021 10:01
@maximbaz
Copy link
Contributor

Interesting idea to make exa into a library! I haven't seen any noticeable impact in exa, but may be due to some hardware differences or what not... Also --git could be a slow flag. In any case, lets see what those guys reply to your suggestion!

@sayanarijit
Copy link
Owner Author

Hey @maximbaz I have reimplemented the path sorting using natord (like exa) for natural ordering and decided to publish the version with the current implementation and then look into exa to have a good base for comparison. So, need a final review.

@maximbaz
Copy link
Contributor

Nice!

  1. In the s (sorting menu), we have far less possibilities than what really exists. This is probably intentional, but I just wanted to confirm that it was not forgotten, did you add all the shortcuts that you wanted to into that list?

  2. The default initial_sorting looks like this:

general:
  show_hidden: false
  initial_sorting:
    - sorter: ByCanonicalIsDir
      reverse: true
    - sorter: ByIsBroken
      reverse: true
    - sorter: ByIRelativePath
      reverse: false

Why is it, that if I change reverse: false in ByIsBroken or even remove that block completely, broken link is always last in xplr? Some config not applying properly, or am I missing something?

  1. In default config in sorter_identifiers you are missing e.g. ByIRelativePath - if I override it, it works, so just a matter of having it in the config I guess. But ByRelativePath has rel as format, maybe ByIRelativePath should have irel or [i]rel as default instead of the current s?

@sayanarijit sayanarijit force-pushed the add/sorting-support branch from d723b0c to fb07f22 Compare April 18, 2021 10:30
Also improve filtering.

Closes: #58
@sayanarijit sayanarijit force-pushed the add/sorting-support branch from fb07f22 to 83d550e Compare April 18, 2021 10:33
@sayanarijit
Copy link
Owner Author

sayanarijit commented Apr 18, 2021

  1. Yes... This is intentional. We can add more on user demand but too much can be confusing and difficult to remove later.
  2. This is because we're sorting by directory first. Unlike exa we don't yet have a --group-directories-first option. So, using sorters to group by directory as of now. But the issue is that with broken symlinks, the sorting logic can't determine whether they're supposed to be pointing to directories or files. So it's being marked separately (neither directory, nor file, it's just broken).
  3. Thanks. Fixed. [i]rel.

@maximbaz
Copy link
Contributor

I see! I think it's all looking perfect to me! 🎉 ship it 🚀 😁

@sayanarijit sayanarijit merged commit a889674 into main Apr 18, 2021
@sayanarijit sayanarijit deleted the add/sorting-support branch May 8, 2021 14:21
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.

Add support for sorting
2 participants