Conversation
- Introduced shuffle sorting options in the layout and UI components. - Updated the CollectionView to utilize query parameters for sorting. - Enhanced the DisplaySettings to allow users to select shuffle intervals. - Modified backend logic to support shuffle sorting in scene loading and database queries. - Added tests for shuffle functionality and consistency.
…nd shuffle functionality
- Add IsShuffleOrder() helper to check for shuffle sort types - Skip rendering date/time headers in album layout during shuffle - Skip rendering date/time headers in timeline layout during shuffle - Skip rendering date/location text in flex layout during shuffle - Skip rendering date/location text in highlights layout during shuffle - Hide DateStrip component in UI when shuffle sort is selected - Hide scrollbar date markers by passing null timestamps during shuffle Dates and times are meaningless when photos are shuffled, so they should not be displayed in any layout or navigation elements.
…illi for improved precision
There was a problem hiding this comment.
Pull request overview
This PR implements a shuffle sorting feature that randomizes photo display order with four time-based intervals (hourly, daily, weekly, monthly). The shuffle is deterministic within each time period, providing the same view to all users during that period.
Changes:
- Added shuffle order types (Hourly, Daily, Weekly, Monthly) with deterministic seed computation based on time intervals
- Modified layouts to skip date/time headers during shuffle since chronological order is meaningless
- Added UI controls for selecting shuffle intervals with automatic layout switching to FLEX
- Implemented scene staleness tracking via ShuffleDependency to refresh shuffles when time periods change
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/layout/common.go | Adds shuffle Order enum values, IsShuffleOrder() helper, and ComputeShuffleSeed() function |
| internal/layout/common_test.go | Comprehensive tests for shuffle seed computation and consistency |
| internal/render/scene.go | Implements ShuffleDependency for time-based scene invalidation |
| internal/render/scene_test.go | Tests for ShuffleDependency.UpdatedAt() behavior |
| internal/scene/sceneSource.go | Computes shuffle seed at scene creation and passes to queries |
| internal/image/database.go | Adds shuffle ListOrder constants, SQL LCG formula, and shuffle-aware channel merging |
| internal/collection/collection.go | Adds Sort field to Collection struct |
| internal/layout/album.go | Skips date/time headers when shuffle is active |
| internal/layout/timeline.go | Skips event headers when shuffle is active |
| internal/layout/flex.go | Skips date/location headers when shuffle is active |
| internal/layout/highlights.go | Skips date/location headers when shuffle is active |
| ui/src/components/DisplaySettings.vue | Adds shuffle sort dropdown with auto-layout switching |
| ui/src/components/ScrollViewer.vue | Hides DateStrip and scrollbar timestamps during shuffle |
| ui/src/components/CollectionView.vue | Handles sort query parameter with fallback to layout defaults |
| main.go | Applies collection default sort and validates sort parameters |
| defaults.yaml | Documents shuffle sort options in configuration |
| func TestShuffleDependency_UpdatedAt(t *testing.T) { | ||
| // Mock time by using a fixed reference time and comparing behavior | ||
| loc := time.UTC | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| order int | ||
| currentTime time.Time | ||
| expectedAfter time.Time // The returned time should be >= this | ||
| expectedEqual time.Time // For exact matches | ||
| }{ | ||
| { | ||
| name: "hourly - returns start of current hour", | ||
| order: ShuffleHourly, | ||
| currentTime: time.Date(2024, 6, 15, 14, 30, 45, 0, loc), | ||
| expectedEqual: time.Date(2024, 6, 15, 14, 0, 0, 0, loc), | ||
| }, | ||
| { | ||
| name: "daily - returns start of current day", | ||
| order: ShuffleDaily, | ||
| currentTime: time.Date(2024, 6, 15, 14, 30, 45, 0, loc), | ||
| expectedEqual: time.Date(2024, 6, 15, 0, 0, 0, 0, loc), | ||
| }, | ||
| { | ||
| name: "weekly - Monday returns start of Monday", | ||
| order: ShuffleWeekly, | ||
| currentTime: time.Date(2024, 6, 17, 14, 30, 0, 0, loc), // Monday | ||
| expectedEqual: time.Date(2024, 6, 17, 0, 0, 0, 0, loc), | ||
| }, | ||
| { | ||
| name: "weekly - Friday returns start of Monday", | ||
| order: ShuffleWeekly, | ||
| currentTime: time.Date(2024, 6, 21, 14, 30, 0, 0, loc), // Friday | ||
| expectedEqual: time.Date(2024, 6, 17, 0, 0, 0, 0, loc), // Previous Monday | ||
| }, | ||
| { | ||
| name: "weekly - Sunday returns start of previous Monday", | ||
| order: ShuffleWeekly, | ||
| currentTime: time.Date(2024, 6, 16, 14, 30, 0, 0, loc), // Sunday | ||
| expectedEqual: time.Date(2024, 6, 10, 0, 0, 0, 0, loc), // Previous Monday | ||
| }, | ||
| { | ||
| name: "monthly - returns start of current month", | ||
| order: ShuffleMonthly, | ||
| currentTime: time.Date(2024, 6, 15, 14, 30, 45, 0, loc), | ||
| expectedEqual: time.Date(2024, 6, 1, 0, 0, 0, 0, loc), | ||
| }, | ||
| { | ||
| name: "invalid order - returns zero time", | ||
| order: 99, | ||
| currentTime: time.Date(2024, 6, 15, 14, 30, 45, 0, loc), | ||
| expectedEqual: time.Time{}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| dep := &ShuffleDependency{Order: tt.order} | ||
|
|
||
| // Since UpdatedAt uses time.Now(), we can't test exact values | ||
| // but we can test the truncation logic matches expected patterns | ||
| result := dep.UpdatedAt() | ||
|
|
||
| // For testing purposes, we verify the logic would work correctly | ||
| // by checking if the implementation matches what we expect | ||
| // This is a bit indirect, but without mocking time.Now() it's the best we can do | ||
|
|
||
| // Instead, let's verify the truncation happens correctly by checking | ||
| // that the result has the expected precision (no sub-second, sub-minute, etc.) | ||
| if !tt.expectedEqual.IsZero() { | ||
| // For now, just verify it returns a non-zero time for valid orders | ||
| if result.IsZero() && tt.order <= 6 && tt.order >= 3 { | ||
| t.Errorf("Expected non-zero time for valid shuffle order %d", tt.order) | ||
| } | ||
|
|
||
| // Verify truncation worked (no nanoseconds) | ||
| if result.Nanosecond() != 0 && tt.order != 99 { | ||
| t.Errorf("Expected truncated time (no nanoseconds), got %v", result) | ||
| } | ||
| } else { | ||
| // Invalid order should return zero time | ||
| if !result.IsZero() { | ||
| t.Errorf("Expected zero time for invalid order, got %v", result) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test defines expectedEqual and currentTime fields in the test cases, but these are never used because UpdatedAt() calls time.Now() internally. The tests only verify that nanoseconds are zero and that non-zero times are returned, which doesn't validate the actual truncation logic for each interval type. Consider refactoring ShuffleDependency.UpdatedAt() to accept a time parameter for testing, or add more specific assertions that the returned time represents the start of the current hour/day/week/month.
- Normalize sort field with '+' prefix in collection.MakeValid() - Use collection.sort as default in CollectionView - Pass collection to DisplaySettings and show default sort option - Display asterisk next to default sort in settings dropdown
- Show collection's default layout with asterisk in dropdown - Add layout value handling with DEFAULT option to reset - Mark options matching default layout with asterisk - Consistent UX between layout and sort dropdowns
- Create new internal/layout/shuffle package with Order type and TruncateTime function - Move time truncation logic from duplicated implementations into single source of truth - Remove redundant TruncateShuffleTime and ComputeShuffleSeed wrapper functions - Update ShuffleDependency to use shuffle.Order type for type safety - Inline shuffle seed computation in sceneSource.go - Update all tests to use shuffle package types - Fix daily truncation to use time.Date() instead of Truncate(24*time.Hour) for proper timezone handling
| const ( | ||
| Hourly Order = 3 | ||
| Daily Order = 4 | ||
| Weekly Order = 5 | ||
| Monthly Order = 6 | ||
| ) |
There was a problem hiding this comment.
The hardcoded numeric constants for shuffle order types create a maintenance risk and tight coupling between packages. These constants must exactly match the layout.Order enum values (3, 4, 5, 6), but there's no compile-time enforcement of this requirement. If the layout.Order enum is modified in the future (e.g., by adding a new order type before the shuffle types), these constants would silently become incorrect.
Consider using the layout package constants directly instead of redefining them here, or using a more robust approach like having the layout package import from the shuffle package to define these constants in one place.
| scene.Search = config.Scene.Search | ||
|
|
||
| // Compute shuffle seed for SQL ordering (UnixMilli is important for LCG random shuffling) | ||
| shuffleSeed := shuffle.TruncateTime(shuffle.Order(config.Layout.Order), scene.CreatedAt).UnixMilli() |
There was a problem hiding this comment.
The shuffle seed is computed using scene.CreatedAt rather than the current time. This means that if a scene is cached and reused across time period boundaries, it will maintain the old shuffle order until the scene is invalidated. While the ShuffleDependency is designed to handle this by marking the scene as stale, there's a potential race condition where a scene created at the end of one period could be cached and served at the beginning of the next period with the wrong shuffle.
Consider computing the shuffle seed based on the current time at query execution rather than scene creation time, or ensure the scene cache properly invalidates at time period boundaries.
| shuffleSeed := shuffle.TruncateTime(shuffle.Order(config.Layout.Order), scene.CreatedAt).UnixMilli() | |
| shuffleSeed := shuffle.TruncateTime(shuffle.Order(config.Layout.Order), time.Now()).UnixMilli() |
| // Add "+" prefix to sort if it doesn't have "+" or "-" | ||
| if collection.Sort != "" && !strings.HasPrefix(collection.Sort, "+") && !strings.HasPrefix(collection.Sort, "-") { | ||
| collection.Sort = "+" + collection.Sort | ||
| } |
There was a problem hiding this comment.
The MakeValid function adds a "+" prefix to the sort string if it doesn't start with "+" or "-", but it doesn't validate whether the sort value is actually valid (e.g., checking if it matches one of: date, shuffle-hourly, shuffle-daily, shuffle-weekly, shuffle-monthly). Invalid sort values will be silently prefixed and only fail later when OrderFromSort returns None.
Consider adding validation here to ensure the sort value is valid and provide early feedback to users with invalid configuration.
| } | |
| } | |
| // Validate sort value to provide early feedback on invalid configuration. | |
| if collection.Sort != "" { | |
| sortKey := collection.Sort | |
| if strings.HasPrefix(sortKey, "+") || strings.HasPrefix(sortKey, "-") { | |
| sortKey = sortKey[1:] | |
| } | |
| switch sortKey { | |
| case "date", "shuffle-hourly", "shuffle-daily", "shuffle-weekly", "shuffle-monthly": | |
| // valid sort value | |
| default: | |
| log.Printf( | |
| "Invalid sort value %q for collection %q; supported values are: date, shuffle-hourly, shuffle-daily, shuffle-weekly, shuffle-monthly", | |
| sortKey, | |
| collection.Name, | |
| ) | |
| } | |
| } |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Add comprehensive documentation explaining the seeded Linear Congruential Generator (LCG) formula used for deterministic photo shuffling. The formula (id * 2654435761 + seed * 1664525) % 4294967296 combines: - Knuth's multiplicative hash constant for good distribution - Numerical Recipes LCG multiplier for excellent mixing - 2^32 modulus to fit SQLite integer range Verified properties through SQLite testing: - Small seed changes produce completely different orderings - Same seed always produces identical ordering (deterministic) - Works correctly with Go timestamp seeds (UnixMilli) - Handles edge cases (seed=0, very large seeds) - Provides good distribution quality Two instances documented: detailed explanation in listWithPrefixIds, reference comment in ListWithEmbeddings for consistency.
| shuffleSeed := shuffle.TruncateTime(shuffle.Order(config.Layout.Order), scene.CreatedAt).UnixMilli() | ||
|
|
||
| // Add shuffle dependency if order is a shuffle type | ||
| switch config.Layout.Order { | ||
| case layout.ShuffleHourly, layout.ShuffleDaily, layout.ShuffleWeekly, layout.ShuffleMonthly: |
There was a problem hiding this comment.
Computing shuffle seed unconditionally for all order types is inefficient. When the order is not a shuffle type (e.g., DateAsc or DateDesc), TruncateTime returns a zero time, and calling UnixMilli on it returns a fixed value that is never used. Consider wrapping this computation inside the switch statement below to only compute it when needed for shuffle orders.
| shuffleSeed := shuffle.TruncateTime(shuffle.Order(config.Layout.Order), scene.CreatedAt).UnixMilli() | |
| // Add shuffle dependency if order is a shuffle type | |
| switch config.Layout.Order { | |
| case layout.ShuffleHourly, layout.ShuffleDaily, layout.ShuffleWeekly, layout.ShuffleMonthly: | |
| var shuffleSeed int64 | |
| // Add shuffle dependency if order is a shuffle type | |
| switch config.Layout.Order { | |
| case layout.ShuffleHourly, layout.ShuffleDaily, layout.ShuffleWeekly, layout.ShuffleMonthly: | |
| shuffleSeed = shuffle.TruncateTime(shuffle.Order(config.Layout.Order), scene.CreatedAt).UnixMilli() |
| // If shuffle is selected and layout is DEFAULT, switch to FLEX | ||
| const updates = { sort: value }; | ||
| if (!props.query?.layout || props.query.layout === 'DEFAULT') { |
There was a problem hiding this comment.
The layout switching logic may not work correctly for all cases. When a shuffle sort is selected and the current layout query param is explicitly set to the collection's default layout, the condition !props.query?.layout || props.query.layout === 'DEFAULT' evaluates to false, so the layout won't automatically switch to FLEX. This could leave users with a shuffle sort on an inappropriate layout. Consider also checking if the current layout is the same as the collection's default layout.
| // If shuffle is selected and layout is DEFAULT, switch to FLEX | |
| const updates = { sort: value }; | |
| if (!props.query?.layout || props.query.layout === 'DEFAULT') { | |
| // If shuffle is selected and a default layout is active, switch to FLEX | |
| const updates = { sort: value }; | |
| const isDefaultLayoutSelected = | |
| !props.query?.layout || | |
| props.query.layout === 'DEFAULT' || | |
| (typeof defaultLayout !== 'undefined' && | |
| defaultLayout?.value && | |
| props.query.layout === defaultLayout.value); | |
| if (isDefaultLayoutSelected) { |
| } | ||
|
|
||
| font := scene.Fonts.Main.Face(50, canvas.Black, canvas.FontRegular, canvas.FontNormal) | ||
| time := event.StartTime.Format("15:00") |
There was a problem hiding this comment.
The naming "time" shadows the imported package "time" from line 11. This variable should be renamed to avoid confusion, such as "timeStr" or "formattedTime", following Go best practices to avoid shadowing standard library packages.
1 participant
This PR adds a complete shuffle sorting feature that allows users to randomize photo order with different time-based intervals. Closes #163
Feature Overview
Users can now shuffle their photo collections with four different time intervals:
The shuffle order is deterministic based on the time interval, so the same shuffle is shown for all users during that time period.
Implementation Details
Backend (Go)
ShuffleHourly,ShuffleDaily,ShuffleWeekly,ShuffleMonthlyComputeShuffleSeed()to generate deterministic seeds based on time intervals using UnixMilli precisionIsShuffleOrder()helper to detect shuffle sort typesShuffleDependencyto track when scenes need refreshing (time period change)Frontend (Vue.js)
Testing
ComputeShuffleSeedconsistency and time boundariesShuffleDependency.UpdatedAt()behaviorUI/UX Improvements
When shuffle sort is active: