Conversation
meesoft
force-pushed
the
features/DropdownSliders
branch
from
225123c to
f3e2914
Compare
meesoft
force-pushed
the
features/DropdownSliders
branch
from
f3e2914 to
bc2a994
Compare
Contributor
WalkthroughReorganizes several UI controls from PhotoLocator.Helpers to PhotoLocator.Controls, adds a new DropDownSlider WPF control, enhances VideoTransformCommands with delayed preview updates and slider-range handling, extends Exif timestamp parsing, tweaks filename sanitization and string parsing helpers, and sets AppendPlatformToOutputPath=false in multiple project files. Changes
Sequence Diagram(s)(omitted — changes do not introduce a new multi-component sequential flow that requires diagramming per rules) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PhotoLocator/VideoTransformCommands.cs (1)
82-96:⚠️ Potential issue | 🟡 Minor
DurationSliderMaxis not reset whenSkipTois cleared.When the user clears the
SkipTofield (empty string),double.TryParseon Line 93 fails andDurationSliderMaxretains its previously reduced value instead of resetting to the full clip duration.Proposed fix
BeginPreviewUpdate(SkipTo); if (double.TryParse(SkipTo, CultureInfo.InvariantCulture, out var skipToSeconds) && skipToSeconds <= SkipToSliderMax) DurationSliderMax = SkipToSliderMax - skipToSeconds; + else + DurationSliderMax = SkipToSliderMax;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/VideoTransformCommands.cs` around lines 82 - 96, The SkipTo setter leaves DurationSliderMax reduced when SkipTo is cleared because the double.TryParse branch only updates DurationSliderMax on success; modify the SkipTo property setter (SkipTo, SkipToSliderMax, DurationSliderMax) so that when SkipTo is empty or TryParse fails you explicitly reset DurationSliderMax to SkipToSliderMax (e.g., after BeginPreviewUpdate or in an else branch), ensuring clearing the field returns the duration slider to the full clip duration; keep the existing calls to UpdateInputArgs, UpdateOutputArgs, _localContrastSetup?.SourceBitmap = null and BeginPreviewUpdate(SkipTo).
🧹 Nitpick comments (3)
PhotoLocator/Metadata/ExifHandler.cs (1)
434-443: DJI time-correction block doesn't cover the new slash-separatedFileTimeStampformat.The DJI skew-correction on line 437 re-parses
FileTimeStampQuery1/2with only"yyyy:MM:dd HH:mm:ss". If a DJI device emits its FileTimeStamp in"yyyy/MM/dd HH:mm:ss"form,TryParseExactsilently fails and the correction is skipped. The main timestamp still parses correctly (no data loss), but the DJI-specific skew fix is silently bypassed.Consider adding the same fallback here for consistency:
🔧 Suggested fix
- if (DateTime.TryParseExact(fileTimeStampStr, "yyyy:MM:dd HH:mm:ss", CultureInfo.InvariantCulture, DateTimeStyles.AllowWhiteSpaces, out var fileTimeStamp)) + if (DateTime.TryParseExact(fileTimeStampStr, "yyyy:MM:dd HH:mm:ss", CultureInfo.InvariantCulture, DateTimeStyles.AllowWhiteSpaces, out var fileTimeStamp) || + DateTime.TryParseExact(fileTimeStampStr, "yyyy/MM/dd HH:mm:ss", CultureInfo.InvariantCulture, DateTimeStyles.AllowWhiteSpaces, out fileTimeStamp))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/Metadata/ExifHandler.cs` around lines 434 - 443, The DJI-specific timestamp correction only tries to parse FileTimeStampQuery1/2 with "yyyy:MM:dd HH:mm:ss" so any slash-separated value ("yyyy/MM/dd HH:mm:ss") will fail and skip the correction; update the parsing around metadata.CameraManufacturer so FileTimeStampQuery1/2 is parsed accepting both colon- and slash-separated date formats (e.g., call DateTime.TryParseExact with a formats array like "yyyy:MM:dd HH:mm:ss" and "yyyy/MM/dd HH:mm:ss" or attempt a second TryParseExact), keeping CultureInfo.InvariantCulture and DateTimeStyles.AllowWhiteSpaces, then proceed to compare diff and assign timeStamp as before.PhotoLocator/Metadata/MaskBasedNaming.cs (1)
128-130: Consider reusing_invalidFileNameCharsand a single-pass sanitization.Two minor inefficiencies:
Path.GetInvalidFileNameChars()is called here instead of the already-cached_invalidFileNameCharsfield (Line 12).- The
foreachloop allocates a newstringfor each of the ~41 invalid chars regardless of whether any of them actually appear invalue.♻️ Proposed single-pass replacement
- foreach (var ch in Path.GetInvalidFileNameChars()) - value = value.Replace(ch, '-'); + value = string.Concat(value.Select(ch => _invalidFileNameChars.Contains(ch) ? '-' : ch));Or, if LINQ isn't preferred, a
StringBuilderloop overvalue's characters avoids repeated allocations entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/Metadata/MaskBasedNaming.cs` around lines 128 - 130, The current code repeatedly calls Path.GetInvalidFileNameChars() and then uses a foreach that does many string.Replace allocations; replace that by using the cached _invalidFileNameChars field and perform a single-pass sanitization of the variable value (e.g., iterate the characters of value and append either the original char or '-' to a StringBuilder/char[] if the char is contained in _invalidFileNameChars) and then Append the sanitized result to result; this removes the repeated Replace calls and avoids calling Path.GetInvalidFileNameChars() again.PhotoLocator/Controls/DropDownSlider.xaml.cs (1)
10-10: Static reentrancy guards are shared across all instances.These flags are
static, meaning they are shared across everyDropDownSliderinstance in the application. While this works in practice because WPF is single-threaded, instance-level flags would be more correct and safer. Additionally, they are not reset in atry/finallyblock — if an exception is thrown mid-callback (e.g., from a binding update), the flag stays stucktrue, permanently breaking synchronization for all instances.♻️ Suggested refactor: use instance fields with try/finally
- static bool _onTextChangedRunning, _onNumericValueChangedRunning; + bool _onTextChangedRunning, _onNumericValueChangedRunning;And guard with
try/finallyin the callbacks:static void OnTextChanged(DependencyObject d, DependencyPropertyChangedEventArgs e) { - if (_onNumericValueChangedRunning || d is not DropDownSlider control) + if (d is not DropDownSlider control || control._onNumericValueChangedRunning) return; if (double.TryParse(control.Text, NumberStyles.Float, CultureInfo.InvariantCulture, out var v)) { - _onTextChangedRunning = true; - control.NumericValue = Math.Clamp(v, control.Minimum, control.Maximum); - _onTextChangedRunning = false; + control._onTextChangedRunning = true; + try + { + control.NumericValue = Math.Clamp(v, control.Minimum, control.Maximum); + } + finally + { + control._onTextChangedRunning = false; + } } }static void OnNumericValueChanged(DependencyObject d, DependencyPropertyChangedEventArgs e) { if (d is not DropDownSlider control) return; - _onNumericValueChangedRunning = true; - if (!_onTextChangedRunning) - control.Text = control.NumericValue.ToString(...); - control.UpdateFormattedValue(); - _onNumericValueChangedRunning = false; + control._onNumericValueChangedRunning = true; + try + { + if (!control._onTextChangedRunning) + control.Text = control.NumericValue.ToString("F" + control.Decimals.ToString(CultureInfo.InvariantCulture), CultureInfo.InvariantCulture); + control.UpdateFormattedValue(); + } + finally + { + control._onNumericValueChangedRunning = false; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/Controls/DropDownSlider.xaml.cs` at line 10, The two reentrancy guards _onTextChangedRunning and _onNumericValueChangedRunning are declared static and can be left stuck if an exception occurs; change them to instance fields on the DropDownSlider class (remove static) and update the callbacks (e.g., OnTextChanged, OnNumericValueChanged or similarly named handlers) to set the flag inside a try block and reset it in a finally block so the guard is always cleared even on exceptions; ensure you reference and modify the instance fields in all places that currently use the static symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PhotoLocator/Controls/DropDownSlider.xaml`:
- Line 27: The TickFrequency binding currently has no effect because the Slider
in DropDownSlider.xaml has IsSnapToTickEnabled="False" and TickPlacement is
unset; either enable snapping and optionally show tick marks or remove the
binding. Fix by updating the Slider control: set IsSnapToTickEnabled="True" and
add an appropriate TickPlacement (e.g., BottomRight) if you want discrete steps
and visible ticks, or remove the TickFrequency binding and related TickFrequency
property if the slider should remain continuous; target the Slider element and
the TickFrequency, IsSnapToTickEnabled, and TickPlacement attributes when making
the change.
In `@PhotoLocator/Controls/DropDownSlider.xaml.cs`:
- Around line 38-44: MinimumProperty and MaximumProperty don't currently notify
when they change, so NumericValue (and displayed Text) can fall outside the new
range; update the DependencyProperty registrations for MinimumProperty and
MaximumProperty to include a PropertyChangedCallback (e.g., OnMinMaxChanged)
that clamps NumericValue into the new range and refreshes Text/slider state.
Implement OnMinMaxChanged to read the new min/max via GetValue, clamp the
current NumericValue (or call CoerceValue if you have a NumericValueProperty
coercion), then SetValue/SetCurrentValue for NumericValue and update Text so the
control's numeric/text/visual state remain in sync.
In `@PhotoLocator/Helpers/DropDownSliderService.cs`:
- Around line 5-18: Delete the unused DropDownSliderService class and all its
members (the attached DependencyProperties MinimumProperty and MaximumProperty
and the accessors SetMinimum, GetMinimum, SetMaximum, GetMaximum); remove the
containing file if it becomes empty and run the build to verify there are no
remaining references to DropDownSliderService or those symbols.
In `@PhotoLocator/Helpers/IntMath.cs`:
- Around line 20-22: The doc comment on the Clamp method in IntMath.cs
incorrectly claims "Clamp without min/max check" while the implementation
performs bounds checks; update the XML doc for the Clamp method to describe the
actual behavior (that it checks and clamps against min and max), or
alternatively add a true unchecked overload (e.g., ClampUnchecked or
ClampNoChecks) with the "without min/max check" documentation and keep the
existing Clamp as the checked variant—refer to the Clamp method in class IntMath
when making the change.
In `@PhotoLocator/Helpers/RealMath.cs`:
- Around line 8-10: The XML docs for the Clamp overloads are misleading; update
the summary for both Clamp methods (the overloads in RealMath) to state that the
method omits the argument-order validation (the check that min ≤ max) rather
than skipping bounds checking on the value, and clarify that callers must ensure
min ≤ max (not that the value is already within the range); apply this corrected
wording to both overload summaries so the docs accurately describe the omitted
check.
In `@PhotoLocator/Helpers/StringExtensions.cs`:
- Around line 10-13: TrimInvariantValue currently calls str.Trim() with no null
guard which throws on null; add a null-check at the start of the method and
return null (or a sensible default) when str is null, then perform Trim() and
Replace(',','.') as before; also review and apply the same null-guard pattern to
the related TrimPath method to keep behavior consistent.
In `@PhotoLocator/VideoTransformCommands.cs`:
- Around line 865-872: GetClipDurationSeconds currently accesses
metadata["Duration"] which can throw KeyNotFoundException; change it to use
metadata.TryGetValue("Duration", out var spanStr) (or check ContainsKey) after
calling ExifTool.LoadMetadata (keep using _mainViewModel.Settings.ExifToolPath),
and if the key is missing throw a UserMessageException with a clear message like
"Duration not found in ExifTool metadata for {fileName}" so callers get a
descriptive error; preserve the existing parsing logic (TryParse then
TimeSpan.Parse) on spanStr when present.
---
Outside diff comments:
In `@PhotoLocator/VideoTransformCommands.cs`:
- Around line 82-96: The SkipTo setter leaves DurationSliderMax reduced when
SkipTo is cleared because the double.TryParse branch only updates
DurationSliderMax on success; modify the SkipTo property setter (SkipTo,
SkipToSliderMax, DurationSliderMax) so that when SkipTo is empty or TryParse
fails you explicitly reset DurationSliderMax to SkipToSliderMax (e.g., after
BeginPreviewUpdate or in an else branch), ensuring clearing the field returns
the duration slider to the full clip duration; keep the existing calls to
UpdateInputArgs, UpdateOutputArgs, _localContrastSetup?.SourceBitmap = null and
BeginPreviewUpdate(SkipTo).
---
Nitpick comments:
In `@PhotoLocator/Controls/DropDownSlider.xaml.cs`:
- Line 10: The two reentrancy guards _onTextChangedRunning and
_onNumericValueChangedRunning are declared static and can be left stuck if an
exception occurs; change them to instance fields on the DropDownSlider class
(remove static) and update the callbacks (e.g., OnTextChanged,
OnNumericValueChanged or similarly named handlers) to set the flag inside a try
block and reset it in a finally block so the guard is always cleared even on
exceptions; ensure you reference and modify the instance fields in all places
that currently use the static symbols.
In `@PhotoLocator/Metadata/ExifHandler.cs`:
- Around line 434-443: The DJI-specific timestamp correction only tries to parse
FileTimeStampQuery1/2 with "yyyy:MM:dd HH:mm:ss" so any slash-separated value
("yyyy/MM/dd HH:mm:ss") will fail and skip the correction; update the parsing
around metadata.CameraManufacturer so FileTimeStampQuery1/2 is parsed accepting
both colon- and slash-separated date formats (e.g., call DateTime.TryParseExact
with a formats array like "yyyy:MM:dd HH:mm:ss" and "yyyy/MM/dd HH:mm:ss" or
attempt a second TryParseExact), keeping CultureInfo.InvariantCulture and
DateTimeStyles.AllowWhiteSpaces, then proceed to compare diff and assign
timeStamp as before.
In `@PhotoLocator/Metadata/MaskBasedNaming.cs`:
- Around line 128-130: The current code repeatedly calls
Path.GetInvalidFileNameChars() and then uses a foreach that does many
string.Replace allocations; replace that by using the cached
_invalidFileNameChars field and perform a single-pass sanitization of the
variable value (e.g., iterate the characters of value and append either the
original char or '-' to a StringBuilder/char[] if the char is contained in
_invalidFileNameChars) and then Append the sanitized result to result; this
removes the repeated Replace calls and avoids calling
Path.GetInvalidFileNameChars() again.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
PhotoLocatorTest/Metadata/ExifHandlerTest.cs (1)
258-260:⚠️ Potential issue | 🟡 MinorHardcoded timestamp in ignored test will likely fail if re-enabled.
Line 259 asserts against a hardcoded string
"06/17/2022 19:03:02"(no UTC offset), while all other tests useJpegTestDataTimestamp— aDateTimeOffsetthat carries the local UTC offset. IfGetMetadataStringreturns a timezone-aware string, this assertion will fail on any machine whose offset produces a different suffix, or once the method's output format is updated.🛠️ Proposed fix to align with the rest of the test suite
- Assert.AreEqual("FC7303, 100.7m, 1/80s, f/2.8, 4.49mm, ISO100, 06/17/2022 19:03:02", str); + Assert.AreEqual("FC7303, 100.7m, 1/80s, f/2.8, 4.49mm, ISO100, " + JpegTestDataTimestamp, str);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocatorTest/Metadata/ExifHandlerTest.cs` around lines 258 - 260, The failing assertion uses a hardcoded timestamp string; replace that literal with the canonical test timestamp value JpegTestDataTimestamp formatted the same way other tests expect (i.e. use JpegTestDataTimestamp.ToString(...) or the same helper used elsewhere) and update the Assert.AreEqual on ExifHandler.GetMetadataString(metadata, stream) to compare against that formatted JpegTestDataTimestamp instead of "06/17/2022 19:03:02" so the test respects the DateTimeOffset (UTC offset) used throughout the suite.
🧹 Nitpick comments (2)
PhotoLocatorTest/PictureFileFormats/JpegliEncoderTest.cs (1)
24-24: PreferTestContext.WriteLineoverDebug.WriteLinefor MSTest diagnostic output.Two concerns with
Debug.WriteLinehere:
- Conditionally compiled away in Release builds —
Debug.WriteLineis guarded by[Conditional("DEBUG")], so in a Release test run all four size-reporting lines are silent no-ops.- Not captured in test results — MSTest captures
TestContext.WriteLineoutput in the.trx/HTML report alongside each test case;Debug.WriteLineonly reaches the debug trace listener and won't appear in test result artifacts.Since
TestContextis already declared on line 12, this is a straightforward swap:♻️ Proposed refactor
- Debug.WriteLine($"Source size: {new FileInfo(SourcePath).Length / 1024} kb"); + TestContext.WriteLine($"Source size: {new FileInfo(SourcePath).Length / 1024} kb"); // ... - Debug.WriteLine($"Dest size jpeg: {sizeJpeg / 1024} kb"); + TestContext.WriteLine($"Dest size jpeg: {sizeJpeg / 1024} kb"); // ... - Debug.WriteLine($"Dest size jpegli: {sizeJpegli / 1024} kb"); - Debug.WriteLine($"{100.0 * sizeJpegli / sizeJpeg:0.0}%"); + TestContext.WriteLine($"Dest size jpegli: {sizeJpegli / 1024} kb"); + TestContext.WriteLine($"{100.0 * sizeJpegli / sizeJpeg:0.0}%");With this change the
using System.Diagnostics;import can also be removed.Also applies to: 33-33, 37-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocatorTest/PictureFileFormats/JpegliEncoderTest.cs` at line 24, Replace the Debug.WriteLine calls in JpegliEncoderTest (e.g., the Debug.WriteLine($"Source size: ...") and the other instances around lines 33 and 37-38) with TestContext.WriteLine so MSTest captures the output; after swapping all Debug.WriteLine occurrences to TestContext.WriteLine, remove the unused using System.Diagnostics; ensure TestContext (declared on line 12) is used for all diagnostic writes.PhotoLocator/Controls/DropDownSlider.xaml.cs (1)
64-66:FormattedNumericValueis read-only in intent but publicly settable.This property is only ever written internally by
UpdateFormattedValue(). Exposing a public setter invites external misuse. Consider making it a read-only dependency property viaDependencyPropertyKey:Proposed fix
- public static readonly DependencyProperty FormattedNumericValueProperty = DependencyProperty.Register( - nameof(FormattedNumericValue), typeof(string), typeof(DropDownSlider), new PropertyMetadata(string.Empty)); - public string FormattedNumericValue { get => (string)GetValue(FormattedNumericValueProperty); set => SetValue(FormattedNumericValueProperty, value); } + static readonly DependencyPropertyKey FormattedNumericValuePropertyKey = DependencyProperty.RegisterReadOnly( + nameof(FormattedNumericValue), typeof(string), typeof(DropDownSlider), new PropertyMetadata(string.Empty)); + public static readonly DependencyProperty FormattedNumericValueProperty = FormattedNumericValuePropertyKey.DependencyProperty; + public string FormattedNumericValue { get => (string)GetValue(FormattedNumericValueProperty); private set => SetValue(FormattedNumericValuePropertyKey, value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/Controls/DropDownSlider.xaml.cs` around lines 64 - 66, The FormattedNumericValue should be made read-only: replace the public writable DependencyProperty with a read-only DependencyProperty by declaring a private static readonly DependencyPropertyKey (e.g., FormattedNumericValuePropertyKey) and register it via DependencyProperty.RegisterReadOnly, expose its DependencyProperty as a public static readonly DependencyProperty (FormattedNumericValueProperty = FormattedNumericValuePropertyKey.DependencyProperty), change the CLR wrapper FormattedNumericValue to a getter-only property that reads via GetValue(FormattedNumericValueProperty), and update UpdateFormattedValue() to set the value using SetValue(FormattedNumericValuePropertyKey, newValue) so only internal code can modify it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PhotoLocator/Controls/DropDownSlider.xaml.cs`:
- Around line 26-36: OnTextChanged and OnNumericValueChanged use reentrancy
flags (_onTextChangedRunning and _onNumericValueChangedRunning) that are set
before updating control.NumericValue/Text but are not reset if an exception is
thrown; wrap the guarded update sections in try/finally so the flag is always
cleared, e.g., in OnTextChanged set _onTextChangedRunning = true, perform
Math.Clamp and assign NumericValue inside the try, and set _onTextChangedRunning
= false in finally, and do the analogous try/finally for
_onNumericValueChangedRunning in OnNumericValueChanged.
- Line 10: The reentrancy guard fields _onTextChangedRunning and
_onNumericValueChangedRunning are declared static and thus shared across all
DropDownSlider instances; change them to instance fields (remove static) on the
DropDownSlider class and then in the static callbacks (OnTextChanged,
OnNumericValueChanged) cast the DependencyObject d to DropDownSlider and
reference the instance flags (e.g., control._onTextChangedRunning,
control._onNumericValueChangedRunning) so each control has its own guard and
callbacks do not interfere across instances.
---
Outside diff comments:
In `@PhotoLocatorTest/Metadata/ExifHandlerTest.cs`:
- Around line 258-260: The failing assertion uses a hardcoded timestamp string;
replace that literal with the canonical test timestamp value
JpegTestDataTimestamp formatted the same way other tests expect (i.e. use
JpegTestDataTimestamp.ToString(...) or the same helper used elsewhere) and
update the Assert.AreEqual on ExifHandler.GetMetadataString(metadata, stream) to
compare against that formatted JpegTestDataTimestamp instead of "06/17/2022
19:03:02" so the test respects the DateTimeOffset (UTC offset) used throughout
the suite.
---
Duplicate comments:
In `@PhotoLocator/Controls/DropDownSlider.xaml.cs`:
- Around line 38-44: MinimumProperty and MaximumProperty lack change callbacks
so NumericValue can remain outside new bounds; add a PropertyChangedCallback (or
CoerceValueCallback) to the DependencyProperty registration for MinimumProperty
and MaximumProperty in class DropDownSlider that, when either bound changes,
coerces/clamps the current NumericValue to Math.Max(Minimum, Math.Min(Maximum,
NumericValue)) (or call CoerceValue(NumericValueProperty) if you already
implemented a CoerceValueCallback for NumericValue) and update it via
SetCurrentValue(NumericValueProperty, clampedValue) to avoid overwriting
bindings.
In `@PhotoLocator/Helpers/RealMath.cs`:
- Around line 8-10: Update the XML summary for the two clamp overloads in
RealMath.cs to explicitly state that the method does not perform the
argument-ordering validation and that the caller must guarantee the ordering
(i.e., that min <= max or max > min); replace the ambiguous phrase "without
min/max sanity check" with wording like "does not validate that min <= max
(caller must ensure min <= max)" so it's clear which check is omitted.
---
Nitpick comments:
In `@PhotoLocator/Controls/DropDownSlider.xaml.cs`:
- Around line 64-66: The FormattedNumericValue should be made read-only: replace
the public writable DependencyProperty with a read-only DependencyProperty by
declaring a private static readonly DependencyPropertyKey (e.g.,
FormattedNumericValuePropertyKey) and register it via
DependencyProperty.RegisterReadOnly, expose its DependencyProperty as a public
static readonly DependencyProperty (FormattedNumericValueProperty =
FormattedNumericValuePropertyKey.DependencyProperty), change the CLR wrapper
FormattedNumericValue to a getter-only property that reads via
GetValue(FormattedNumericValueProperty), and update UpdateFormattedValue() to
set the value using SetValue(FormattedNumericValuePropertyKey, newValue) so only
internal code can modify it.
In `@PhotoLocatorTest/PictureFileFormats/JpegliEncoderTest.cs`:
- Line 24: Replace the Debug.WriteLine calls in JpegliEncoderTest (e.g., the
Debug.WriteLine($"Source size: ...") and the other instances around lines 33 and
37-38) with TestContext.WriteLine so MSTest captures the output; after swapping
all Debug.WriteLine occurrences to TestContext.WriteLine, remove the unused
using System.Diagnostics; ensure TestContext (declared on line 12) is used for
all diagnostic writes.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
PhotoLocator/Controls/DropDownSlider.xaml.cs (1)
68-70:FormattedNumericValuePropertyshould be registered as a read-only dependency property.The
private setaccessor prevents property-syntax assignment from external C# code, but the underlyingDependencyPropertyis still publicly writable viaSetValue(DropDownSlider.FormattedNumericValueProperty, value)and data-binding (e.g.,{Binding Mode=OneWayToSource}). The idiomatic WPF pattern for a truly read-only DP isDependencyProperty.RegisterReadOnly+ aDependencyPropertyKey.♻️ Proposed refactor
- public static readonly DependencyProperty FormattedNumericValueProperty = DependencyProperty.Register( - nameof(FormattedNumericValue), typeof(string), typeof(DropDownSlider), new PropertyMetadata(string.Empty)); - public string FormattedNumericValue { get => (string)GetValue(FormattedNumericValueProperty); private set => SetValue(FormattedNumericValueProperty, value); } + static readonly DependencyPropertyKey FormattedNumericValuePropertyKey = DependencyProperty.RegisterReadOnly( + nameof(FormattedNumericValue), typeof(string), typeof(DropDownSlider), new PropertyMetadata(string.Empty)); + public static readonly DependencyProperty FormattedNumericValueProperty = FormattedNumericValuePropertyKey.DependencyProperty; + public string FormattedNumericValue { get => (string)GetValue(FormattedNumericValueProperty); private set => SetValue(FormattedNumericValuePropertyKey, value); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PhotoLocator/Controls/DropDownSlider.xaml.cs` around lines 68 - 70, Change FormattedNumericValueProperty to a true read-only dependency property by using DependencyProperty.RegisterReadOnly to create a DependencyPropertyKey (e.g., FormattedNumericValuePropertyKey) and expose the public DependencyProperty via FormattedNumericValueProperty; keep the FormattedNumericValue CLR getter but remove the public setter and update internal assignments to call SetValue(FormattedNumericValuePropertyKey, value) (referencing DropDownSlider, FormattedNumericValue, FormattedNumericValuePropertyKey, FormattedNumericValueProperty, and DependencyProperty.RegisterReadOnly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@PhotoLocator/Controls/DropDownSlider.xaml.cs`:
- Around line 38-44: MinimumProperty and MaximumProperty registrations lack
change callbacks so NumericValue isn't re-clamped when the range changes; update
the DependencyProperty.Register calls for MinimumProperty and MaximumProperty on
the DropDownSlider to provide PropertyChangedCallback handlers that, when
invoked, clamp the current NumericValue into the new [Minimum, Maximum] range
and then refresh any dependent state (e.g., update FormattedNumericValue and the
displayed Text) to keep the control consistent. Ensure the callbacks reference
the NumericValue property/getter and the FormattedNumericValue/Text update logic
used elsewhere in the class so shrinking the range immediately enforces the new
limits.
- Around line 32-34: The reentrancy flags (_onTextChangedRunning and
_onValueChangedRunning) are left true if an exception occurs during setting
NumericValue (which calls SetValue) or while updating Text/UpdateFormattedValue;
wrap each guarded section that sets a flag around the mutating work in a
try/finally so the flag is always reset: for the block setting
control._onTextChangedRunning = true before assigning control.NumericValue =
Math.Clamp(...), ensure a try { control.NumericValue = ... } finally {
control._onTextChangedRunning = false }, and do the same for the block that sets
control._onValueChangedRunning = true before updating
control.Text/UpdateFormattedValue (also apply the same change to the similar
block around lines 76-80).
---
Nitpick comments:
In `@PhotoLocator/Controls/DropDownSlider.xaml.cs`:
- Around line 68-70: Change FormattedNumericValueProperty to a true read-only
dependency property by using DependencyProperty.RegisterReadOnly to create a
DependencyPropertyKey (e.g., FormattedNumericValuePropertyKey) and expose the
public DependencyProperty via FormattedNumericValueProperty; keep the
FormattedNumericValue CLR getter but remove the public setter and update
internal assignments to call SetValue(FormattedNumericValuePropertyKey, value)
(referencing DropDownSlider, FormattedNumericValue,
FormattedNumericValuePropertyKey, FormattedNumericValueProperty, and
DependencyProperty.RegisterReadOnly).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements