Include selected folders in slide show #62
Conversation
WalkthroughSlide show logic now accepts checked items in addition to files, SlideShowWindow consumes a slideShowItems input and expands directories into an internal pictures list, thumbnail loading is converted to async with cancellation, mouse/keyboard preview handlers were added, and PictureItemViewModel constructor allows a nullable PropertyChanged handler. Changes
Sequence Diagram(s)sequenceDiagram
participant M as MainViewModel
participant S as SlideShowWindow
participant U as UpdateFolders
participant A as UpdatePictureAsync
participant T as ThumbnailLoader
M->>S: new(slideShowItems, selectedPicture|null, selectedMapLayerName, settings)
S->>S: store _slideShowItems
S->>U: UpdateFolders()
U->>S: populate _pictures (expand dirs -> files)
S->>S: set SelectedPicture (input or first _pictures)
rect rgb(240,248,255)
Note over S,A: On PictureIndex change / timer tick
S->>A: UpdatePictureAsync()
A->>A: cancel prior resample if running
alt _pictures not empty
A->>T: load thumbnail async (if needed)
T-->>A: thumbnail ready
A->>S: update display, title, metadata
else Empty _pictures
A-->>S: early return
end
end
rect rgb(255,250,240)
Note over S: Input handling
S->>S: HandlePreviewMouseDown (double-click -> Close)
S->>S: HandlePreviewKeyUp (Escape or 'Q' -> Close)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
PhotoLocator/SlideShowWindow.xaml.cs (2)
55-66: Consider case-insensitive extension matching consistency.The extension check uses
ToLowerInvariant()which is correct. However, consider whetherOrdinalIgnoreCasecomparison would be more appropriate for file extensions on Windows to match system behavior.Also,
_picturesis cleared and rebuilt on each call, which means any state on those items (like loaded thumbnails) is lost whenUpdateFolders()is called during slideshow playback.
110-112: Fire-and-forget async call.The setter invokes
UpdatePictureAsync().WithExceptionLogging()without awaiting. WhileWithExceptionLogginghandles exceptions, this pattern can lead to subtle race conditions ifPictureIndexis set multiple times in quick succession before the previous async operation completes.The cancellation on line 120 mitigates this, but the pattern could be cleaner.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
PhotoLocator/MainViewModel.csPhotoLocator/PictureItemViewModel.csPhotoLocator/SlideShowWindow.xamlPhotoLocator/SlideShowWindow.xaml.cs
🧰 Additional context used
🧬 Code graph analysis (2)
PhotoLocator/MainViewModel.cs (2)
PhotoLocator/SlideShowWindow.xaml.cs (3)
SlideShowWindow(29-291)SlideShowWindow(38-53)Dispose(281-290)PhotoLocator/MainWindow.xaml.cs (4)
GetSelectedMapLayerName(135-138)MainWindow(22-543)MainWindow(34-48)Dispose(537-542)
PhotoLocator/SlideShowWindow.xaml.cs (4)
PhotoLocator/PictureItemViewModel.cs (8)
PictureItemViewModel(35-43)PictureItemViewModel(46-54)BitmapSource(252-273)BitmapSource(275-321)BitmapSource(323-344)Task(234-250)Task(346-360)Task(373-406)PhotoLocator/Helpers/UserMessageException.cs (3)
UserMessageException(9-11)UserMessageException(13-15)UserMessageException(17-19)PhotoLocator/Settings/SettingsExtensions.cs (1)
CleanPhotoFileExtensions(27-39)PhotoLocator/Helpers/TaskExtensions.cs (1)
WithExceptionLogging(14-18)
🔇 Additional comments (8)
PhotoLocator/PictureItemViewModel.cs (1)
46-53: Nullable handler parameter looks good.Making the
handleFilePropertyChangedparameter nullable and conditionally subscribing is a clean approach for scenarios where property change notifications aren't needed (e.g., slideshow items created from folder contents).PhotoLocator/SlideShowWindow.xaml (1)
23-23: LGTM!Adding
PreviewMouseDownhandler for double-click-to-close is a nice UX improvement that complements the existing keyboard shortcuts.PhotoLocator/MainViewModel.cs (2)
246-248: LGTM!Minor style update using pattern matching.
567-583: Slideshow item selection logic looks good.The changes appropriately:
- Include both files and checked items (folders) in the slideshow source
- Handle the case where no qualifying items exist by adding the selected item
- Pass
nullforselectedPicturewhen the selected item is not a file- Guard the post-slideshow selection by verifying the item still exists in
ItemsPhotoLocator/SlideShowWindow.xaml.cs (4)
31-53: Initialization and folder handling look good.The separation of
_slideShowItems(input) from_pictures(derived working list) is a clean design. The[MemberNotNull]attribute correctly documents the guarantee.
115-162: Async update flow looks solid.Good defensive programming with the early return when
_picturesis empty (line 117-118), and proper cancellation handling before starting new work (line 120). Loading the thumbnail on-demand (lines 122-123) is a nice optimization.
224-236: LGTM!Adding 'Q' as an additional close key is a nice UX touch for users who expect this common shortcut in slideshow applications.
238-245: LGTM!Double-click to close is intuitive and well-implemented with proper event handling (
e.Handled = true).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
PhotoLocator/SlideShowWindow.xaml.cs (2)
62-62: Optimize HashSet creation outside the loop.The
HashSet<string>is recreated on every directory iteration. Move it outside the loop for better performance.🔎 Proposed optimization
[MemberNotNull(nameof(_pictures))] private void UpdateFolders() { _pictures = _slideShowItems.Where(item => item.IsFile).ToList(); - HashSet<string>? extensions = null; + var directories = _slideShowItems.Where(item => item.IsDirectory).ToList(); + if (directories.Count == 0) + return; + var extensions = Settings.CleanPhotoFileExtensions().ToHashSet(); - foreach (var folder in _slideShowItems.Where(item => item.IsDirectory)) + foreach (var folder in directories) { - extensions ??= Settings.CleanPhotoFileExtensions().ToHashSet(); foreach (var folderPicture in Directory.EnumerateFiles(folder.FullPath).Where(fn => extensions.Contains(Path.GetExtension(fn).ToLowerInvariant()))) _pictures.Add(new PictureItemViewModel(folderPicture, false, null, Settings)); } }
122-123: Consider accepting a CancellationToken parameter.Using
defaultas the cancellation token means thumbnail/metadata loading cannot be interrupted if the user navigates quickly through pictures or closes the window. Consider accepting aCancellationTokenparameter or using a field-level token.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
PhotoLocator/PictureItemViewModel.csPhotoLocator/SlideShowWindow.xaml.cs
🧰 Additional context used
🧬 Code graph analysis (1)
PhotoLocator/SlideShowWindow.xaml.cs (2)
PhotoLocator/Helpers/UserMessageException.cs (3)
UserMessageException(9-11)UserMessageException(13-15)UserMessageException(17-19)PhotoLocator/Settings/SettingsExtensions.cs (1)
CleanPhotoFileExtensions(27-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (6)
PhotoLocator/PictureItemViewModel.cs (1)
46-52: LGTM! Nullable handler enables flexible instantiation.The optional
PropertyChangedEventHandler?parameter and conditional subscription correctly support scenarios where property change tracking is unnecessary, such as the slide-show picture expansion logic.PhotoLocator/SlideShowWindow.xaml.cs (5)
38-53: LGTM! Constructor correctly initializes from slideShowItems.The refactored constructor properly accepts a mix of files and directories, expands them via
UpdateFolders(), and provides clear error messaging when no pictures are available.
117-118: LGTM! Guard prevents index out-of-range errors.The early return for empty
_picturescorrectly prevents crashes when the collection becomes empty after folder updates.
219-226: Verify handling when UpdateFolders results in empty collection.When wrapping to the beginning,
UpdateFolders()is called andPictureIndexis reset to 0. If the folder contents changed and_picturesbecomes empty, the slideshow continues with an invalid state. AlthoughUpdatePictureAsync()has a guard at lines 117-118, explicitly checking here would be clearer.🔎 Suggested defensive check
if (PictureIndex + 1 >= _pictures.Count) { UpdateFolders(); + if (_pictures.Count == 0) + { + Close(); + return; + } PictureIndex = 0; }
230-230: LGTM! Added convenient quit key.Adding
Key.Qprovides an intuitive additional way to close the slideshow.
242-249: LGTM! Double-click to close improves UX.The implementation correctly detects double-clicks on the left mouse button and provides an intuitive way to exit the slideshow.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.