image sorting support #89
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds image sorting support to the Rewind app, allowing users to sort images by date (ascending or descending) or shuffle them randomly. The feature integrates sorting into the image list view with a toolbar menu picker and persists the user's preference in settings.
Changes:
- Added new
ImageSortingenum with three sorting options: date ascending, date descending, and shuffle - Integrated sorting UI into the image list toolbar with a menu picker
- Added sorting preference to app settings with persistence
- Updated localization files with new sorting-related strings in English, Russian, Serbian (Latin), and Ukrainian
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| Rewind/Model/ImageSorting.swift | New file defining sorting enum and array extension for sorting images |
| Rewind/View/ImageList.swift | Added toolbar with sorting menu picker and localized labels for sorting options |
| Rewind/Model/SettingsViewModel.swift | Added sorting property to settings state with default value and decoding logic |
| Rewind/Model/MapModel.swift | Integrated sorting into map model to sort current region images |
| Rewind/Model/ImageListModel.swift | Added sorting state and action handling to image list model |
| Rewind/Model/AppModel.swift | Updated to pass sorting property to image list models |
| Rewind/Model/AppGraph.swift | Wired up sorting property and added observer to update previews on sorting changes |
| Rewind/Localizable.xcstrings | Added localized strings for sorting options and menu in 4 languages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Rewind/Model/ImageSorting.swift
Outdated
| case dateAccending | ||
| case dateDescending | ||
| case shuffle | ||
| } | ||
|
|
||
| extension [Model.Image] { | ||
| func sorted(by sorting: ImageSorting) -> [Model.Image] { | ||
| switch sorting { | ||
| case .dateAccending: sorted { $0.date.year < $1.date.year } |
There was a problem hiding this comment.
The spelling of "Ascending" is incorrect. It should be "Ascending" instead of "Accending" in the enum case name. This typo appears in the enum case definition and will propagate throughout the codebase.
| case dateAccending | |
| case dateDescending | |
| case shuffle | |
| } | |
| extension [Model.Image] { | |
| func sorted(by sorting: ImageSorting) -> [Model.Image] { | |
| switch sorting { | |
| case .dateAccending: sorted { $0.date.year < $1.date.year } | |
| case dateAscending | |
| case dateDescending | |
| case shuffle | |
| } | |
| extension [Model.Image] { | |
| func sorted(by sorting: ImageSorting) -> [Model.Image] -> [Model.Image] { | |
| switch sorting { | |
| case .dateAscending: sorted { $0.date.year < $1.date.year } |
Rewind/View/ImageList.swift
Outdated
|
|
||
| fileprivate var title: LocalizedStringKey { | ||
| switch self { | ||
| case .dateAccending: "Date Ascending" |
There was a problem hiding this comment.
The spelling of "Ascending" is incorrect. It should be "Ascending" instead of "Accending". This typo is used consistently with the misspelled enum case name.
Rewind/View/ImageList.swift
Outdated
| listUpdates: .empty, | ||
| imageDetailsFactory: imageDetailsFactoryMock | ||
| imageDetailsFactory: imageDetailsFactoryMock, | ||
| sorting: .constant(.dateAccending) |
There was a problem hiding this comment.
The spelling of "Ascending" is incorrect. It should be "Ascending" instead of "Accending". This typo is used consistently with the misspelled enum case name.
Rewind/Model/ImageSorting.swift
Outdated
| extension [Model.Image] { | ||
| func sorted(by sorting: ImageSorting) -> [Model.Image] { | ||
| switch sorting { | ||
| case .dateAccending: sorted { $0.date.year < $1.date.year } |
There was a problem hiding this comment.
The spelling of "Ascending" is incorrect. It should be "Ascending" instead of "Accending". This typo is used consistently with the misspelled enum case name.
| sorting?.value = newSorting | ||
| state.images = state.images.sorted(by: newSorting) |
There was a problem hiding this comment.
The Picker's selection binding creates a new ImageSorting value on each change, but the guard statement on line 57 checks if the sorting hasn't changed before proceeding. However, since enums are value types and the comparison should work correctly, this is acceptable. Still, consider whether the setter should be called before or after the guard check to ensure proper state synchronization.
| sorting?.value = newSorting | |
| state.images = state.images.sorted(by: newSorting) | |
| state.images = state.images.sorted(by: newSorting) | |
| sorting?.value = newSorting |
Rewind/View/ImageList.swift
Outdated
|
|
||
| fileprivate var iconName: String { | ||
| switch self { | ||
| case .dateAccending: "arrow.up" |
There was a problem hiding this comment.
The spelling of "Ascending" is incorrect. It should be "Ascending" instead of "Accending". This typo is used consistently with the misspelled enum case name.
Rewind/View/ImageList.swift
Outdated
| listUpdates: .empty, | ||
| imageDetailsFactory: imageDetailsFactoryMock | ||
| imageDetailsFactory: imageDetailsFactoryMock, | ||
| sorting: .constant(.dateAccending) |
There was a problem hiding this comment.
The spelling of "Ascending" is incorrect. It should be "Ascending" instead of "Accending". This typo is used consistently with the misspelled enum case name.
Rewind/Model/SettingsViewModel.swift
Outdated
| showYearColorInClusters: true, | ||
| openClusterPreviews: false | ||
| openClusterPreviews: false, | ||
| sorting: .dateAccending |
There was a problem hiding this comment.
The spelling of "Ascending" is incorrect. It should be "Ascending" instead of "Accending". This typo is used consistently with the misspelled enum case name.
Rewind/Model/SettingsViewModel.swift
Outdated
| self.openClusterPreviews = try container.decode(Bool.self, forKey: .openClusterPreviews) | ||
|
|
||
| self.sorting = try container | ||
| .decodeIfPresent(ImageSorting.self, forKey: .sorting) ?? .dateAccending |
There was a problem hiding this comment.
The spelling of "Ascending" is incorrect. It should be "Ascending" instead of "Accending". This typo is used consistently with the misspelled enum case name.
Rewind/Model/ImageSorting.swift
Outdated
| case .dateAccending: sorted { $0.date.year < $1.date.year } | ||
| case .dateDescending: sorted { $0.date.year > $1.date.year } |
There was a problem hiding this comment.
The sorting implementation only considers the start year (year) and not the end year (year2). For images with date ranges, this may lead to inaccurate sorting. Consider comparing year2 as a tiebreaker when year values are equal, or using the average/midpoint of the range for more accurate chronological ordering.
| case .dateAccending: sorted { $0.date.year < $1.date.year } | |
| case .dateDescending: sorted { $0.date.year > $1.date.year } | |
| case .dateAccending: | |
| sorted { | |
| let lhs = ($0.date.year, $0.date.year2 ?? $0.date.year) | |
| let rhs = ($1.date.year, $1.date.year2 ?? $1.date.year) | |
| return lhs < rhs | |
| } | |
| case .dateDescending: | |
| sorted { | |
| let lhs = ($0.date.year, $0.date.year2 ?? $0.date.year) | |
| let rhs = ($1.date.year, $1.date.year2 ?? $1.date.year) | |
| return lhs > rhs | |
| } |
No description provided.