Conversation
WalkthroughChange signatures to use full file paths for selection, add a UI command and ExifTool API to set EXIF timestamps, tighten ToneMapping setter logic, make ListBox focus handling null-safe, and rename/adjust ExifTool timestamp method names. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as MainWindow
participant VM as MainViewModel
participant Dialog as TimestampPrompt
participant Exif as ExifTool
participant FS as FileSystem
User->>UI: Right-click -> "Set timestamp"
UI->>VM: Execute SetTimestampCommand
VM->>Dialog: Show timestamp input (initial suggestion)
Dialog-->>VM: Return timestamp string
VM->>VM: Start parallel tasks for each selected item
VM->>Exif: SetTimestampAsync(item.FullPath, item.FullPath, timestamp, exifToolPath, ct)
Exif->>FS: Invoke exiftool process to write EXIF timestamps
FS-->>Exif: Process exit / success
Exif-->>VM: Task(s) complete
VM->>VM: Select first affected item's FullPath
VM->>UI: Update selection / refresh
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🧰 Additional context used🧬 Code graph analysis (3)PhotoLocator/Metadata/ExifTool.cs (2)
PhotoLocatorTest/Metadata/ExifToolTest.cs (1)
PhotoLocator/MainViewModel.cs (2)
⏰ 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). (2)
🔇 Additional comments (6)
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: 1
🧹 Nitpick comments (3)
PhotoLocator/LocalContrastViewModel.cs (1)
171-175: Consider documenting the MaxStretch side effect.When
ToneMappingexceeds 1, this setter automatically setsMaxStretchto 100. This coupling between properties may not be intuitive to users. Consider adding a comment explaining the rationale for this behavior.Note: Setting
MaxStretchwill trigger its ownStartUpdateTimercall (Line 129), resulting in two timer starts whenToneMapping > 1. While this appears safe given the implementation, it's worth being aware of.</review_comment_end>
PhotoLocator/MainWindow.xaml (1)
181-181: LGTM! New menu item added correctly.The new "Set timestamp" menu item is properly placed in the Metadata / EXIF section and bound to
SetTimestampCommand.For visual consistency, consider adding an icon similar to the "Adjust timestamps" item above it.
</review_comment_end>
PhotoLocator/Metadata/ExifTool.cs (1)
28-32: Consider adding timestamp format validation.Unlike
AdjustTimeStampAsync(Line 20-21), which validates the offset format, this method doesn't validate thetimestampparameter. Consider adding validation to ensure the timestamp matches the expected ExifTool format (likely "yyyy:MM:dd HH:mm:ss") before constructing the command.Example validation:
public static async Task SetTimeStampAsync(string sourceFileName, string targetFileName, string timestamp, string exifToolPath, CancellationToken ct) { + if (string.IsNullOrWhiteSpace(timestamp)) + throw new UserMessageException("Timestamp cannot be empty"); var startInfo = new ProcessStartInfo(exifToolPath, $"\"-AllDates={timestamp}\" \"{sourceFileName}\" "); await RunExifToolAsync(sourceFileName, targetFileName, startInfo, ct); }</review_comment_end>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
PhotoLocator/IMainViewModel.cs(1 hunks)PhotoLocator/LocalContrastViewModel.cs(1 hunks)PhotoLocator/MainViewModel.cs(3 hunks)PhotoLocator/MainWindow.xaml(1 hunks)PhotoLocator/MainWindow.xaml.cs(1 hunks)PhotoLocator/Metadata/ExifTool.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
PhotoLocator/Metadata/ExifTool.cs (1)
PhotoLocator/PictureItemViewModel.cs (3)
Task(233-249)Task(345-359)Task(372-405)
🔇 Additional comments (5)
PhotoLocator/MainWindow.xaml.cs (1)
142-144: LGTM! Improved null-safety.The refactor to use a safe cast and null-conditional operators prevents potential
NullReferenceExceptionif the container is not aListBoxItemor is null.</review_comment_end>
PhotoLocator/IMainViewModel.cs (1)
17-17: LGTM! Parameter rename improves clarity.Renaming
fileNametofullPathbetter reflects that the parameter expects a complete file path rather than just a filename. The implementation inMainViewModelis correctly updated to match.</review_comment_end>
PhotoLocator/MainViewModel.cs (3)
271-275: LGTM! Parameter rename improves clarity.The rename from
fileNametofullPathaligns with the interface change and makes the code more self-documenting.</review_comment_end>
488-489: LGTM! Simplified focus handling.The change from
else-iftoelsesimplifies the logic and ensures an item is always focused after the operation completes.</review_comment_end>
959-966: LGTM! Improved file selection after timestamp adjustment.Replacing the delay with
SelectFileAsyncis better because it actively waits for the file to appear (with retry logic) rather than using a passive delay.</review_comment_end>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
PhotoLocator/MainViewModel.cs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
PhotoLocator/MainViewModel.cs (5)
PhotoLocator/IMainViewModel.cs (3)
Task(17-17)Task(19-19)Task(21-21)PhotoLocator/PictureItemViewModel.cs (4)
Task(233-249)Task(345-359)Task(372-405)GetProcessedFileName(361-370)PhotoLocator/Metadata/ExifTool.cs (1)
ExifTool(14-206)PhotoLocator/Helpers/RelayCommand.cs (2)
RelayCommand(6-32)RelayCommand(11-15)PhotoLocator/BitmapOperations/BitmapPlaneInt16.cs (1)
ToString(58-61)
⏰ 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 (3)
PhotoLocator/MainViewModel.cs (3)
271-284: FullPath-based SelectFileAsync implementation looks correctUsing
item.FullPathwith a case-insensitive comparison and a bounded wait loop cleanly aligns this helper with the new full-path API and the EXIF commands that re-select processed files. No issues from an async/selection perspective.
483-493: RunProcessWithProgressBarAsync finally-branch simplification is safeChanging the
else if (focusItem is null)to a plainelsekeeps the behavior the same (theelseonly runs whenfocusItemis null anyway) while making the code slightly clearer. The selection fallback toSelectedItemremains intact.
947-968: Adjusted-timestamp EXIF operation wiring looks consistentThe updated
AdjustTimestampsCommandcorrectly:
- Prompts for a signed offset with basic validation,
- Calls
ExifTool.AdjustTimeStampAsync(item.FullPath, item.GetProcessedFileName(), offset, ...)in parallel with the configured degree of parallelism,- And re-selects the first affected item via
SelectFileAsync(selectedItems[0].FullPath)to wait for any file-system updates.This matches the existing ExifTool usage patterns and should behave as intended.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.