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

Keep sort order when filtering lists sorted by date #3282

Merged
merged 3 commits into from
Feb 16, 2024

Conversation

stefanhaller
Copy link
Collaborator

@stefanhaller stefanhaller commented Jan 29, 2024

  • PR Description

Keep the sort order stable when filtering lists of things sorted by date (branches, stashes, reflog entries).

  • Please check if the PR fulfills these requirements
  • Cheatsheets are up-to-date (run go generate ./...)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • Docs (specifically docs/Config.md) have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

@stefanhaller stefanhaller force-pushed the keep-sort-order-when-filtering branch from f7efa9a to 3a9a7b4 Compare February 4, 2024 18:32
@stefanhaller stefanhaller changed the title Keep sort order when filtering Keep sort order when filtering lists sorted by date Feb 4, 2024
@stefanhaller stefanhaller force-pushed the keep-sort-order-when-filtering branch from 3a9a7b4 to 2c7dc31 Compare February 4, 2024 18:45
@stefanhaller stefanhaller added the enhancement New feature or request label Feb 4, 2024
@stefanhaller stefanhaller force-pushed the keep-sort-order-when-filtering branch 2 times, most recently from 7a8ebab to be13508 Compare February 4, 2024 18:51
Copy link

codacy-production bot commented Feb 4, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
-0.01% (target: -2.00%) 90.00%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (c4eedec) 48114 40223 83.60%
Head commit (9f0b4d0) 48120 (+6) 40225 (+2) 83.59% (-0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#3282) 20 18 90.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

You may notice some variations in coverage metrics with the latest Coverage engine update. For more details, visit the documentation

@stefanhaller
Copy link
Collaborator Author

The argument of seeing the best match first in the suggestions view convinced me that it's not desirable to retain the sort order in all lists, but only in those where things are sorted by date.

However, suggestion lists are not affected by this change at all, as they are not using the FilteredListViewModel; they call FuzzySearch themselves (in suggestions_helper.go). Nevertheless, it convinced me that when things are sorted alphabetically, it makes little sense to retain this ordering, and sorting by best match is better.

So I made this configurable, and chose to retain the sort order only in the local and remote branches views (but only when sorting by date or recency), and in the stashes and reflog views. And in the menu view, because we can then keep the section headers in the keybindings menu, which is very useful. All other views now sort by best match as before.

@stefanhaller stefanhaller marked this pull request as ready for review February 4, 2024 18:59
We'll need this to use the slices.Sort function in the next commit. It would
also be possible to use sort.Ints instead, but it's slower.
For some lists it is useful to keep the same sort order when filtering (rather
than sorting by best match like we usually do). Add an optional function to
FilteredList to make this possible, and use it whenever we show lists of things
sorted by date (branches, stashes, reflog entries), as well as menu items
because this allows us to keep the section headers in the keybindings menu,
which is useful for understanding what you are looking at when filtering.
@stefanhaller stefanhaller force-pushed the keep-sort-order-when-filtering branch from be13508 to 9f0b4d0 Compare February 16, 2024 12:52
@stefanhaller
Copy link
Collaborator Author

@jesseduffield When we tested this today, it didn't work correctly when changing the sort order for local branches. The reason was that I was testing my local integration branch that had a much older version of this PR, which didn't make the distinction of alphabetical vs. by date yet. I tested this one again, works as expected.

Apart from that, we decided to go with this version and see if people complain, so I'm going to merge now.

@stefanhaller stefanhaller merged commit a2ff2e6 into master Feb 16, 2024
@stefanhaller stefanhaller deleted the keep-sort-order-when-filtering branch February 16, 2024 12:57
@stefanhaller stefanhaller added ignore-for-release This will exclude the PR from release notes and removed enhancement New feature or request labels Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ignore-for-release This will exclude the PR from release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant