feat(mobile): enhance album sorting functionality with order handling #24816
Conversation
|
Label error. Requires exactly 1 of: changelog:.*. Found: 📱mobile. A maintainer will add the required label. |
There was a problem hiding this comment.
Pull request overview
This PR enhances the album sorting functionality by introducing explicit default sort orders for each sort mode and improving the UI indicators to accurately reflect the effective sort order. The changes address a bug where the "Oldest Photo" sort mode displayed misleading order indicators and ensures that switching between sort modes results in intuitive default behavior.
Key Changes:
- Added
defaultOrderfield toAlbumSortModeenum specifying whether each mode defaults to ascending or descending order - Introduced
effectiveOrder()method that calculates the actual sort order based on the default order and reverse flag - Updated album selector widget to reset the reverse flag when switching sort modes and display accurate order indicators
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| mobile/lib/extensions/sort_order_extensions.dart | Adds utility extension to get the opposite of a SortOrder value |
| mobile/lib/providers/album/album_sort_by_options.provider.dart | Adds defaultOrder field to each AlbumSortMode and implements effectiveOrder method |
| mobile/lib/domain/services/remote_album.service.dart | Updates sorting logic to use defaultOrder and effectiveOrder for consistent behavior |
| mobile/lib/presentation/widgets/album/album_selector.widget.dart | Updates UI to reset reverse state on mode switch and display correct order indicators based on effectiveOrder |
| mobile/test/domain/services/album.service_test.dart | Updates test expectations to match new default sort orders for each mode |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
|
|
||
| return sorted.reversed.toList(); | ||
| return sorted.toList(); |
There was a problem hiding this comment.
Inconsistency with return type. The _sortByNewestAsset method (line 195) returns sorted directly, which is an Iterable, while this method returns sorted.toList(), which is a List. For consistency, both methods should return List<RemoteAlbum>. Consider adding .toList() to line 195 as well.
| AlbumSortMode sortMode, { | ||
| bool isReverse = false, | ||
| }) async { | ||
| // list of albums sorted ascendingly according to the selected sort mode |
There was a problem hiding this comment.
The comment states "list of albums sorted ascendingly according to the selected sort mode", but this is misleading. For modes like mostRecent (which calls _sortByNewestAsset), the albums are indeed sorted in ascending order by their newest asset timestamp. However, the defaultOrder for mostRecent is desc, which means the list will be reversed by default to show the most recent albums first. Consider clarifying this comment to explain that the sorted list is in ascending order by the comparison key, and that the defaultOrder and isReverse logic is applied afterwards to determine the final order.
| // list of albums sorted ascendingly according to the selected sort mode | |
| // albums sorted in ascending order by the comparison key for the selected sort mode; | |
| // defaultOrder and isReverse are applied below to determine the final returned order |
1 participant
Description
This PR improves the albums selector sorting logic and its visual representation to better match the behaviour of the web interface and to fix inconsistencies in the current mobile implementation.
Background / Problem
Previously, album sorting implicitly assumed a ascending default order for all sort modes. While this makes sense for modes like "Album Title" or "Oldest Photo", it leads to unintuitive behavior for others such as "Album Size" or "Most Recent".
In addition, there was a bug in the Oldest Photo sort mode:
What this PR changes
defaultOrderfor eachAlbumSortModeisReverse == falseisReverse == trueasc/descalso in conjunction with the reverse flag) instead of relying on the reverse flag alone.Please let me know what you think about these changes :)
I myself don't like reflecting ascending/descending in the up/down arrow. But thats just how it's done in the web and I guess how it makes sense to memorise it.
How Has This Been Tested?
should flip order when isReverse is true for all modesChecklist:
src/services/uses repositories implementations for database calls, filesystem operations, etc.src/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/)Please describe to which degree, if any, an LLM was used in creating this pull request.
Partly to generate this documentation.