Add reverse sort order option - #1018
Conversation
nift4
left a comment
nift4
left a comment
There was a problem hiding this comment.
Thanks for the PR, just another of these million things you never get around to. I left some comments
|
|
||
| override fun sort(type: Sorter.Type) { | ||
| sortType.value = type | ||
| songAdapter.sort(type) |
There was a problem hiding this comment.
Why change the song list's sort setting when changing the folder sort setting?
There was a problem hiding this comment.
I thought both should be changed with the same button. I missed that there's a separate button for the song list. Will undo this change.
| else types | ||
| res = res + Type.NaturalOrder + Type.NaturalOrderDescending | ||
| if (res.contains(Type.ByDiscAndTrack)) | ||
| res = res + Type.ByDiscAndTrackDescending |
There was a problem hiding this comment.
right that's now redundant with the calculation below. will remove
| } | ||
| val currentSort = adapter.sortType.value | ||
| val activeEntry = buttonMap.entries.find { it.value == currentSort || Sorter.Type.inverse(it.value) == currentSort } | ||
| val defaultSort = activeEntry?.value ?: Sorter.Type.None |
There was a problem hiding this comment.
if you move this assingment below the null-check, you can get rid of the ?: None which is never used
There was a problem hiding this comment.
cleaned this up a little
| if (!menuItem.isChecked) { | ||
| adapter.sort(buttonMap[menuItem.itemId]!!) | ||
| val baseType = buttonMap[menuItem.itemId]!! | ||
| val targetType = if (reverseItem.isChecked) Sorter.Type.inverse(baseType) ?: baseType else baseType |
There was a problem hiding this comment.
I'm wondering, wouldn't it be better UX to reset "reverse order" to false when user chooses a different sort type?
Perhaps saving a per-sort option cache to disk, so that following UX is achieved (example of main song list):
- User starts with sort option "Name", not reversed -> A-Z
- User enables reverse mode for Z-A order
- User selects Modified date sort option instead, because user didn't use this option before, reverse is automatically turned off
- Later, User selects name again, and the reverse state is restored, Z-A order is loaded automatically
There was a problem hiding this comment.
I agree this would be the best behavior. But for now I'd like to keep the logic simple, especially since I don't have any experience with the preferences system.
I will implement the reset of the reverse order toggle if sorting type is changed.
Would that be fine for you?
There was a problem hiding this comment.
Ok, merging, I'll do the preference part later. Thanks for the PR! :)
|
Thanks :) should the issue be closed or do you want to keep it open for the preferences? |
|
Oh yeah I kinda expected it to auto-close, thanks for the reminder |
2 participants
Adds option to reverse the order type.
This solves #584
I kept the defaults of whether ascending or descending is used for each sort mode because I think it is helpful, and added a checkbox that causes the inverse of the default to be used (as opposed to adding an ascending <> descending toggle).
Also fixed some bugs in the sorting options.
Note: I made these changes with the help of an LLM, but I checked all of them and adapted where I saw fit.