Fix orientation after edit #65
Conversation
WalkthroughAdds a Rotate0 command and UI item to reset JPEG rotation, centralizes thumbnail/metadata reset via PictureItemViewModel.ResetThumbnailAndMetadata, makes JPEG-metadata parameters nullable, introduces ExifHandler.ResetOrientation to normalize EXIF orientation before saving, and updates tests and helpers accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant UI as MainWindow/MainViewModel
participant Cmd as JpegTransformCommands
participant VM as PictureItemViewModel
participant Exif as ExifHandler
participant FS as GeneralFileFormatHandler
Note over User,UI: User selects "Reset rotation tag"
User->>UI: Invoke Rotate0Command
UI->>Cmd: Execute Rotate0Command(selectedItem)
Cmd->>VM: ResetThumbnailAndMetadata()
Cmd->>Exif: ResetOrientation(metadata?)
Exif-->>Cmd: normalizedMetadata (or null)
Cmd->>FS: SaveToFile(imageStream, normalizedMetadata)
FS-->>Cmd: SaveResult
Cmd->>VM: trigger metadata/thumbnail reload
VM-->>UI: UI updates (cleared thumbnail / normalized EXIF)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2024-12-07T15:05:08.244ZApplied to files:
🧬 Code graph analysis (1)PhotoLocator/PictureItemViewModel.cs (1)
⏰ 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)
🔇 Additional comments (1)
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 |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Here are the copyable unit test edits: Copyable EditsPhotoLocatorTest/JpegTransformCommandsTest.csThis is a new file. PhotoLocatorTest/JpegTransformIntegrationTest.csThis is a new file. PhotoLocatorTest/Metadata/ExifHandlerTest.cs@@ -262,3 +262,107 @@
}
}
}
+
+ [TestMethod]
+ public void ResetOrientation_ShouldResetOrientationToOne_WhenOrientationIsSet()
+ {
+ using var stream = GetType().Assembly.GetManifestResourceStream(@"PhotoLocator.TestData.2022-06-17_19.03.02.jpg")
+ ?? throw new FileNotFoundException("Resource not found");
+ var metadata = ExifHandler.LoadMetadata(stream) ?? throw new Exception("Unable to load metadata");
+
+ // Set orientation to a non-default value
+ metadata = metadata.Clone();
+ metadata.SetQuery(ExifHandler.OrientationQuery1, (ushort)6);
+
+ var result = ExifHandler.ResetOrientation(metadata);
+
+ Assert.IsNotNull(result);
+ var orientationValue = result.GetQuery(ExifHandler.OrientationQuery1) as ushort?;
+ Assert.AreEqual((ushort)1, orientationValue);
+ }
+
+ [TestMethod]
+ public void ResetOrientation_ShouldCloneMetadata_WhenFrozen()
+ {
+ using var stream = GetType().Assembly.GetManifestResourceStream(@"PhotoLocator.TestData.2022-06-17_19.03.02.jpg")
+ ?? throw new FileNotFoundException("Resource not found");
+ var metadata = ExifHandler.LoadMetadata(stream) ?? throw new Exception("Unable to load metadata");
+
+ metadata = metadata.Clone();
+ metadata.SetQuery(ExifHandler.OrientationQuery1, (ushort)3);
+ metadata.Freeze();
+
+ var result = ExifHandler.ResetOrientation(metadata);
+
+ Assert.IsNotNull(result);
+ Assert.IsFalse(ReferenceEquals(metadata, result));
+ var orientationValue = result.GetQuery(ExifHandler.OrientationQuery1) as ushort?;
+ Assert.AreEqual((ushort)1, orientationValue);
+ }
+
+ [TestMethod]
+ public void ResetOrientation_ShouldReturnNull_WhenMetadataIsNull()
+ {
+ var result = ExifHandler.ResetOrientation(null);
+
+ Assert.IsNull(result);
+ }
+
+ [TestMethod]
+ public void ResetOrientation_ShouldNotModify_WhenOrientationIsAlreadyOne()
+ {
+ using var stream = GetType().Assembly.GetManifestResourceStream(@"PhotoLocator.TestData.2022-06-17_19.03.02.jpg")
+ ?? throw new FileNotFoundException("Resource not found");
+ var metadata = ExifHandler.LoadMetadata(stream) ?? throw new Exception("Unable to load metadata");
+
+ metadata = metadata.Clone();
+ metadata.SetQuery(ExifHandler.OrientationQuery1, (ushort)1);
+
+ var result = ExifHandler.ResetOrientation(metadata);
+
+ Assert.IsNotNull(result);
+ var orientationValue = result.GetQuery(ExifHandler.OrientationQuery1) as ushort?;
+ Assert.AreEqual((ushort)1, orientationValue);
+ }
+
+ [TestMethod]
+ public void ResetOrientation_ShouldResetBothOrientationQueries()
+ {
+ using var stream = GetType().Assembly.GetManifestResourceStream(@"PhotoLocator.TestData.2022-06-17_19.03.02.jpg")
+ ?? throw new FileNotFoundException("Resource not found");
+ var metadata = ExifHandler.LoadMetadata(stream) ?? throw new Exception("Unable to load metadata");
+
+ metadata = metadata.Clone();
+ metadata.SetQuery(ExifHandler.OrientationQuery1, (ushort)6);
+ metadata.SetQuery(ExifHandler.OrientationQuery2, (ushort)8);
+
+ var result = ExifHandler.ResetOrientation(metadata);
+
+ Assert.IsNotNull(result);
+ var orientation1 = result.GetQuery(ExifHandler.OrientationQuery1) as ushort?;
+ var orientation2 = result.GetQuery(ExifHandler.OrientationQuery2) as ushort?;
+ Assert.AreEqual((ushort)1, orientation1);
+ Assert.AreEqual((ushort)1, orientation2);
+ }
+
+ [TestMethod]
+ public void ResetOrientation_ShouldHandleAllRotationValues()
+ {
+ ushort[] rotationValues = { 3, 6, 8 };
+
+ foreach (var rotationValue in rotationValues)
+ {
+ using var stream = GetType().Assembly.GetManifestResourceStream(@"PhotoLocator.TestData.2022-06-17_19.03.02.jpg")
+ ?? throw new FileNotFoundException("Resource not found");
+ var metadata = ExifHandler.LoadMetadata(stream) ?? throw new Exception("Unable to load metadata");
+
+ metadata = metadata.Clone();
+ metadata.SetQuery(ExifHandler.OrientationQuery1, rotationValue);
+
+ var result = ExifHandler.ResetOrientation(metadata);
+
+ Assert.IsNotNull(result);
+ var orientationValue = result.GetQuery(ExifHandler.OrientationQuery1) as ushort?;
+ Assert.AreEqual((ushort)1, orientationValue, $"Failed for rotation value {rotationValue}");
+ }
+ }PhotoLocatorTest/PictureItemViewModelTest.cs@@ -22,3 +22,92 @@
Assert.IsTrue(File.Exists(@"sidecar\rename2.jpg.cop"));
}
}
+
+ [TestMethod]
+ public void ResetThumbnailAndMetadata_ShouldResetAllProperties()
+ {
+ var file = new PictureItemViewModel("test.jpg", false, null, null);
+
+ // Set up some initial state (using reflection since properties are private set)
+ var thumbnailProperty = typeof(PictureItemViewModel).GetProperty("ThumbnailImage");
+ var metadataProperty = typeof(PictureItemViewModel).GetProperty("MetadataString");
+ var orientationProperty = typeof(PictureItemViewModel).GetProperty("Orientation");
+
+ // Create a dummy image source
+ var dummyImage = System.Windows.Media.Imaging.BitmapSource.Create(
+ 1, 1, 96, 96, System.Windows.Media.PixelFormats.Bgr24, null, new byte[3], 3);
+ thumbnailProperty?.SetValue(file, dummyImage);
+
+ // Set metadata string through field since it's privately set
+ var metadataField = typeof(PictureItemViewModel).GetField("_metadataString",
+ System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
+ metadataField?.SetValue(file, "Test metadata");
+
+ // Set orientation through property
+ var orientationField = typeof(PictureItemViewModel).GetField("<Orientation>k__BackingField",
+ System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
+ orientationField?.SetValue(file, System.Windows.Controls.Rotation.Rotate90);
+
+ file.ResetThumbnailAndMetadata();
+
+ Assert.IsNull(file.ThumbnailImage);
+ Assert.IsNull(file.MetadataString);
+ Assert.AreEqual(System.Windows.Controls.Rotation.Rotate0, file.Orientation);
+ }
+
+ [TestMethod]
+ public void ResetThumbnailAndMetadata_ShouldBeIdempotent()
+ {
+ var file = new PictureItemViewModel("test.jpg", false, null, null);
+
+ file.ResetThumbnailAndMetadata();
+ file.ResetThumbnailAndMetadata();
+
+ Assert.IsNull(file.ThumbnailImage);
+ Assert.IsNull(file.MetadataString);
+ Assert.AreEqual(System.Windows.Controls.Rotation.Rotate0, file.Orientation);
+ }
+
+ [TestMethod]
+ public void Orientation_ShouldBePrivateSet()
+ {
+ var file = new PictureItemViewModel("test.jpg", false, null, null);
+ var orientationProperty = typeof(PictureItemViewModel).GetProperty("Orientation");
+
+ Assert.IsNotNull(orientationProperty);
+ Assert.IsTrue(orientationProperty.CanRead);
+ Assert.IsFalse(orientationProperty.CanWrite || orientationProperty.SetMethod?.IsPublic == true);
+ }
+
+ [TestMethod]
+ public void ThumbnailImage_ShouldBePrivateSet()
+ {
+ var file = new PictureItemViewModel("test.jpg", false, null, null);
+ var property = typeof(PictureItemViewModel).GetProperty("ThumbnailImage");
+
+ Assert.IsNotNull(property);
+ Assert.IsTrue(property.CanRead);
+ Assert.IsFalse(property.SetMethod?.IsPublic == true);
+ }
+
+ [TestMethod]
+ public void MetadataString_ShouldBePrivateSet()
+ {
+ var file = new PictureItemViewModel("test.jpg", false, null, null);
+ var property = typeof(PictureItemViewModel).GetProperty("MetadataString");
+
+ Assert.IsNotNull(property);
+ Assert.IsTrue(property.CanRead);
+ Assert.IsFalse(property.SetMethod?.IsPublic == true);
+ }
+
+ [TestMethod]
+ public void TimeStamp_ShouldBePrivateSet()
+ {
+ var file = new PictureItemViewModel("test.jpg", false, null, null);
+ var property = typeof(PictureItemViewModel).GetProperty("TimeStamp");
+
+ Assert.IsNotNull(property);
+ Assert.IsTrue(property.CanRead);
+ Assert.IsFalse(property.SetMethod?.IsPublic == true);
+ } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
PhotoLocatorTest/PictureFileFormats/JpegliEncoderTest.cs (1)
11-12: LGTM! Standard MSTest pattern for test context injection.The TestContext property follows the standard MSTest convention and will be initialized by the test framework before any test methods execute.
If nullable reference types are enabled in the project, consider adding a nullable annotation or null-forgiving operator to avoid potential CS8618 warnings:
Optional: Nullability annotation patterns
Option 1 (nullable):
-public TestContext TestContext { get; set; } +public TestContext? TestContext { get; set; }Option 2 (null-forgiving operator):
-public TestContext TestContext { get; set; } +public TestContext TestContext { get; set; } = null!;PhotoLocatorTest/BitmapOperations/TimeSliceOperationTest.cs (1)
11-96: Consider clarifying the relationship to PR objectives.These test improvements (cancellation support, assertions, code reuse) are valuable but don't appear directly related to the PR title "Fix orientation after edit." If these are supporting changes for the orientation fix or general test infrastructure improvements, consider mentioning that in the PR description for clarity.
PhotoLocatorTest/Metadata/ExifHandlerTest.cs (2)
264-290: Test logic looks good; consider adding file cleanup.The test correctly verifies that
ResetOrientationresets the orientation tag to 1 when saving a processed image. However, the test file is not deleted after execution, which could lead to test pollution or issues in CI/CD environments.🔎 Suggested cleanup pattern
[TestMethod] public void ResetOrientation_ShouldBeApplied_WhenSavingProcessedImage() { const string TestFileName = "orientation_reset_test.jpg"; + try + { + using var sourceStream = GetType().Assembly.GetManifestResourceStream(@"PhotoLocator.TestData.2022-06-17_19.03.02.jpg") + ?? throw new FileNotFoundException("Resource not found"); + var metadata = ExifHandler.LoadMetadata(sourceStream) ?? throw new Exception("Unable to load metadata"); - using var sourceStream = GetType().Assembly.GetManifestResourceStream(@"PhotoLocator.TestData.2022-06-17_19.03.02.jpg") - ?? throw new FileNotFoundException("Resource not found"); - var metadata = ExifHandler.LoadMetadata(sourceStream) ?? throw new Exception("Unable to load metadata"); - - // Set orientation to simulate rotated image - metadata = metadata.Clone(); - metadata.SetQuery(ExifHandler.OrientationQuery1, (ushort)6); - - // Create a test bitmap - var bitmap = BitmapSource.Create(10, 10, 96, 96, PixelFormats.Bgr24, null, new byte[10 * 10 * 3], 10 * 3); - - // Apply ResetOrientation and save - var resetMetadata = ExifHandler.ResetOrientation(metadata); - GeneralFileFormatHandler.SaveToFile(bitmap, TestFileName, resetMetadata, 90); - - // Verify orientation was reset - using var targetStream = File.OpenRead(TestFileName); - var savedMetadata = ExifHandler.LoadMetadata(targetStream)!; - var orientationValue = savedMetadata.GetQuery(ExifHandler.OrientationQuery1) as ushort?; - - Assert.AreEqual((ushort)1, orientationValue); + // Set orientation to simulate rotated image + metadata = metadata.Clone(); + metadata.SetQuery(ExifHandler.OrientationQuery1, (ushort)6); + + // Create a test bitmap + var bitmap = BitmapSource.Create(10, 10, 96, 96, PixelFormats.Bgr24, null, new byte[10 * 10 * 3], 10 * 3); + + // Apply ResetOrientation and save + var resetMetadata = ExifHandler.ResetOrientation(metadata); + GeneralFileFormatHandler.SaveToFile(bitmap, TestFileName, resetMetadata, 90); + + // Verify orientation was reset + using var targetStream = File.OpenRead(TestFileName); + var savedMetadata = ExifHandler.LoadMetadata(targetStream)!; + var orientationValue = savedMetadata.GetQuery(ExifHandler.OrientationQuery1) as ushort?; + + Assert.AreEqual((ushort)1, orientationValue); + } + finally + { + if (File.Exists(TestFileName)) + File.Delete(TestFileName); + } }
292-323: Test logic looks good; consider adding file cleanup.The test correctly verifies that
ResetOrientationpreserves other metadata (camera model, geotag) when resetting orientation. However, the test file is not deleted after execution.🔎 Suggested cleanup pattern
[TestMethod] public void ResetOrientation_ShouldPreserveOtherMetadata() { const string TestFileName = "metadata_preservation_test.jpg"; + try + { + using var sourceStream = GetType().Assembly.GetManifestResourceStream(@"PhotoLocator.TestData.2022-06-17_19.03.02.jpg") + ?? throw new FileNotFoundException("Resource not found"); + var metadata = ExifHandler.LoadMetadata(sourceStream) ?? throw new Exception("Unable to load metadata"); - using var sourceStream = GetType().Assembly.GetManifestResourceStream(@"PhotoLocator.TestData.2022-06-17_19.03.02.jpg") - ?? throw new FileNotFoundException("Resource not found"); - var metadata = ExifHandler.LoadMetadata(sourceStream) ?? throw new Exception("Unable to load metadata"); - - var originalCameraModel = metadata.CameraModel; - var originalGeotag = ExifHandler.GetGeotag(metadata); - Assert.IsNotNull(originalGeotag); - - // Set orientation and reset it - metadata = metadata.Clone(); - metadata.SetQuery(ExifHandler.OrientationQuery1, (ushort)3); - var resetMetadata = ExifHandler.ResetOrientation(metadata); - - // Create and save bitmap - var bitmap = BitmapSource.Create(10, 10, 96, 96, PixelFormats.Bgr24, null, new byte[10 * 10 * 3], 10 * 3); - GeneralFileFormatHandler.SaveToFile(bitmap, TestFileName, resetMetadata, 90); - - // Verify other metadata is preserved - using var targetStream = File.OpenRead(TestFileName); - var savedMetadata = ExifHandler.LoadMetadata(targetStream)!; - - Assert.AreEqual(originalCameraModel, savedMetadata.CameraModel); - var savedGeotag = ExifHandler.GetGeotag(savedMetadata); - Assert.IsNotNull(savedGeotag); - Assert.AreEqual(originalGeotag.Latitude, savedGeotag.Latitude, 0.001); - Assert.AreEqual(originalGeotag.Longitude, savedGeotag.Longitude, 0.001); + var originalCameraModel = metadata.CameraModel; + var originalGeotag = ExifHandler.GetGeotag(metadata); + Assert.IsNotNull(originalGeotag); + + // Set orientation and reset it + metadata = metadata.Clone(); + metadata.SetQuery(ExifHandler.OrientationQuery1, (ushort)3); + var resetMetadata = ExifHandler.ResetOrientation(metadata); + + // Create and save bitmap + var bitmap = BitmapSource.Create(10, 10, 96, 96, PixelFormats.Bgr24, null, new byte[10 * 10 * 3], 10 * 3); + GeneralFileFormatHandler.SaveToFile(bitmap, TestFileName, resetMetadata, 90); + + // Verify other metadata is preserved + using var targetStream = File.OpenRead(TestFileName); + var savedMetadata = ExifHandler.LoadMetadata(targetStream)!; + + Assert.AreEqual(originalCameraModel, savedMetadata.CameraModel); + var savedGeotag = ExifHandler.GetGeotag(savedMetadata); + Assert.IsNotNull(savedGeotag); + Assert.AreEqual(originalGeotag.Latitude, savedGeotag.Latitude, 0.001); + Assert.AreEqual(originalGeotag.Longitude, savedGeotag.Longitude, 0.001); + } + finally + { + if (File.Exists(TestFileName)) + File.Delete(TestFileName); + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
PhotoLocatorTest/BitmapOperations/TimeSliceOperationTest.csPhotoLocatorTest/Helpers/VideoProcessingTest.csPhotoLocatorTest/Metadata/ExifHandlerTest.csPhotoLocatorTest/PictureFileFormats/JpegliEncoderTest.csPhotoLocatorTest/PictureFileFormats/PhotoshopFileFormatHandlerTest.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-07T15:05:08.244Z
Learnt from: meesoft
Repo: meesoft/PhotoLocator PR: 9
File: PhotoLocator/PictureFileFormats/JpegliEncoder.cs:10-10
Timestamp: 2024-12-07T15:05:08.244Z
Learning: In the `PhotoLocator.PictureFileFormats` namespace, classes like `JpegliEncoder` are intended for internal use only and should remain internal.
Applied to files:
PhotoLocatorTest/PictureFileFormats/JpegliEncoderTest.cs
🧬 Code graph analysis (1)
PhotoLocatorTest/PictureFileFormats/JpegliEncoderTest.cs (1)
PhotoLocator/PictureFileFormats/GeneralFileFormatHandler.cs (1)
GeneralFileFormatHandler(9-78)
⏰ 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)
- GitHub Check: build
- GitHub Check: Analyze (csharp)
🔇 Additional comments (8)
PhotoLocatorTest/PictureFileFormats/PhotoshopFileFormatHandlerTest.cs (1)
16-16: LGTM! Code quality improvement for discard pattern.The change to explicitly assign the
FloatBitmapconstructor result to the discard variable_is correct and follows C# best practices. This likely addresses a static analyzer warning about unused values while maintaining the test's intent to verify that bothLoadFromStreamand theFloatBitmapconstructor succeed without throwing exceptions.Note: This change appears orthogonal to the PR's main objective of fixing orientation after edit—it's a code quality improvement.
Also applies to: 24-24, 32-32, 40-40
PhotoLocatorTest/PictureFileFormats/JpegliEncoderTest.cs (1)
26-26: Excellent improvement for test cancellation support.Passing
TestContext.CancellationTokenallows the test framework to properly cancel long-running operations, improving test reliability and respecting timeout configurations.PhotoLocatorTest/Helpers/VideoProcessingTest.cs (1)
11-11: LGTM! Good refactoring to enable code reuse.Making
SourceVideoPathpublic allows other test classes to reference it, eliminating duplication. This is consumed byTimeSliceOperationTestin the codebase.PhotoLocatorTest/BitmapOperations/TimeSliceOperationTest.cs (5)
11-11: LGTM! Standard MSTest pattern.Adding the
TestContextproperty is the standard MSTest approach for accessing test execution context, including the cancellation token used throughout the test methods.
17-18: Verify the duplicate image paths are intentional.Both array entries reference the same test image. If this is intentional for testing time-slice behavior with identical frames, consider adding a comment to clarify. Otherwise, ensure these should be distinct test images.
32-32: LGTM! Excellent improvement for test cancellation support.Passing
TestContext.CancellationTokento all async operations enables MSTest to cancel long-running tests, improving test reliability and developer experience.Also applies to: 34-34, 59-59, 86-86, 91-91
38-39: LGTM! Good additions to verify frame processing metrics.The new assertions confirm that the expected number of frames were used and none were skipped, strengthening the test validation.
Also applies to: 68-69, 95-96
58-58: LGTM! Good refactoring to eliminate duplication.Referencing
VideoProcessingTest.SourceVideoPathcentralizes the test video path definition, making it easier to maintain.Also applies to: 85-85
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
PhotoLocator/PictureItemViewModel.cs (1)
238-238: Consider reducing log verbosity.This log message will fire for every file during metadata loading, which could be verbose when processing many files or scrolling through a large directory. Consider logging only on errors or using a debug-level log that can be filtered.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
PhotoLocator/JpegTransformCommands.csPhotoLocator/PictureItemViewModel.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-12-07T15:05:08.244Z
Learnt from: meesoft
Repo: meesoft/PhotoLocator PR: 9
File: PhotoLocator/PictureFileFormats/JpegliEncoder.cs:10-10
Timestamp: 2024-12-07T15:05:08.244Z
Learning: In the `PhotoLocator.PictureFileFormats` namespace, classes like `JpegliEncoder` are intended for internal use only and should remain internal.
Applied to files:
PhotoLocator/PictureItemViewModel.cs
🧬 Code graph analysis (1)
PhotoLocator/PictureItemViewModel.cs (3)
PhotoLocator/RenameWindow.xaml.cs (1)
SetProperty(76-83)PhotoLocator/MainViewModel.cs (1)
SetProperty(57-64)PhotoLocator/Helpers/Log.cs (2)
Log(7-36)Write(14-24)
⏰ 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 (5)
PhotoLocator/PictureItemViewModel.cs (1)
150-150: LGTM! Improved encapsulation.Making these property setters private is a good practice, ensuring these properties are only modified internally by the class.
Also applies to: 157-157, 170-170, 193-193
PhotoLocator/JpegTransformCommands.cs (4)
157-157: LGTM! Nullable metadata parameters are appropriate.Making the
metadataparameters nullable is a good change since metadata may not always be available. The subsequent calls toExifHandler.ResetOrientationhandle null correctly according to the PR summary.Also applies to: 177-177
170-171: LGTM! Consistent orientation reset before saving.The addition of
ExifHandler.ResetOrientationcalls before allGeneralFileFormatHandler.SaveToFileoperations ensures that EXIF orientation is consistently normalized across both single-file and batch processing paths. This aligns well with the PR objective.Also applies to: 187-188, 194-195
32-37: Verify thatJpegTransformations.Rotatewith angle=0 resets EXIF orientation as intended.The
Rotate0CommandcallsRotateSelectedAsync(0), which delegates toJpegTransformations.Rotate(item.FullPath, item.GetProcessedFileName(), 0). TheRotatemethod simply converts the angle to a string and passes it to an externalJpegTransform.exeexecutable. The UI message promises to "reset any rotation EXIF data," but the C# code shows no special handling for angle=0. The actual behavior depends entirely on what the external executable does with a "0" argument—whether it truly resets the EXIF orientation tag to 1, or if it's a no-op that leaves the tag unchanged.
39-55: Verify thumbnail/metadata reset after rotation operations.After
RotateSelectedAsynccompletes, the rotated files are detected by the file system watcher, which triggers the file change handler inMainViewModel(lines 1218-1228). This handler callsResetThumbnailAndMetadata()for items whose thumbnails have been loaded, ensuring the UI reflects the new orientation.However, note that unlike
CropSelectedAsync,RotateSelectedAsyncdoes not pause the file watcher during operation. Verify that this approach doesn't cause race conditions or unexpected behavior when the watcher detects file changes while rotation is in progress.
| internal void ResetThumbnailAndMetadata() | ||
| { | ||
| ThumbnailImage = null; | ||
| _metadataString = null; | ||
| Orientation = Rotation.Rotate0; | ||
| } |
There was a problem hiding this comment.
Fix PropertyChanged notification bypass.
Line 350 sets _metadataString = null directly, bypassing the SetProperty mechanism. This means PropertyChanged won't be raised for MetadataString, potentially breaking UI bindings. Lines 349 and 351 correctly use property setters.
🔎 Proposed fix
internal void ResetThumbnailAndMetadata()
{
ThumbnailImage = null;
- _metadataString = null;
+ MetadataString = null;
Orientation = Rotation.Rotate0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| internal void ResetThumbnailAndMetadata() | |
| { | |
| ThumbnailImage = null; | |
| _metadataString = null; | |
| Orientation = Rotation.Rotate0; | |
| } | |
| internal void ResetThumbnailAndMetadata() | |
| { | |
| ThumbnailImage = null; | |
| MetadataString = null; | |
| Orientation = Rotation.Rotate0; | |
| } |
🤖 Prompt for AI Agents
In PhotoLocator/PictureItemViewModel.cs around lines 347 to 352, the
ResetThumbnailAndMetadata method assigns _metadataString = null directly which
bypasses the SetProperty/PropertyChanged mechanism; change that direct field
assignment to use the MetadataString property (e.g., MetadataString = null) so
the SetProperty path runs and raises PropertyChanged for UI bindings, keeping
ThumbnailImage and Orientation assignments as-is.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.