Skip to content

Commit fbe6b20

Browse files
mattleibowCopilot
andcommitted
fix: PinchDetected event leak, false DoubleTapDetected, MinScale/MaxScale ordering; convert duration properties to TimeSpan
- Fix PinchDetected firing when IsPinchEnabled=false but IsPanEnabled=true - Reset _tapCount/_lastTapTicks when entering Pinching state to prevent false double-tap - Add SetScaleRange() method for atomic min/max scale configuration - Convert LongPressDuration, ZoomAnimationDuration, FlingFrameInterval, ZoomAnimationInterval from int (ms) to TimeSpan - Update all usages in implementation, tests, and MAUI sample - Add tests for all bug fixes and SetScaleRange edge cases (411 total, all passing) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ab159a7 commit fbe6b20

File tree

10 files changed

+159
-56
lines changed

10 files changed

+159
-56
lines changed

samples/SkiaSharpDemo/Demos/Gestures/GesturePage.xaml.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -508,20 +508,20 @@ private async void OnSettingsClicked(object? sender, EventArgs e)
508508
AddSlider(layout, "Touch Slop", "px", 1, 50, _tracker.Options.TouchSlop, v => _tracker.Options.TouchSlop = v);
509509
AddSlider(layout, "Double Tap Slop", "px", 10, 200, _tracker.Options.DoubleTapSlop, v => _tracker.Options.DoubleTapSlop = v);
510510
AddSlider(layout, "Fling Threshold", "px/s", 50, 1000, _tracker.Options.FlingThreshold, v => _tracker.Options.FlingThreshold = v);
511-
AddSliderInt(layout, "Long Press Duration", "ms", 100, 2000, _tracker.Options.LongPressDuration, v => _tracker.Options.LongPressDuration = v);
511+
AddSliderInt(layout, "Long Press Duration", "ms", 100, 2000, (int)_tracker.Options.LongPressDuration.TotalMilliseconds, v => _tracker.Options.LongPressDuration = TimeSpan.FromMilliseconds(v));
512512

513513
// --- Fling Settings ---
514514
layout.Children.Add(new Label { Text = "Fling Settings", FontAttributes = FontAttributes.Bold, FontSize = 16, Margin = new Thickness(0, 10, 0, 0) });
515515

516516
AddSlider(layout, "Friction", "", 0.01f, 0.5f, _tracker.Options.FlingFriction, v => _tracker.Options.FlingFriction = v, "F3");
517517
AddSlider(layout, "Min Velocity", "px/s", 1, 50, _tracker.Options.FlingMinVelocity, v => _tracker.Options.FlingMinVelocity = v);
518-
AddSliderInt(layout, "Frame Interval", "ms", 8, 50, _tracker.Options.FlingFrameInterval, v => _tracker.Options.FlingFrameInterval = v);
518+
AddSliderInt(layout, "Frame Interval", "ms", 8, 50, (int)_tracker.Options.FlingFrameInterval.TotalMilliseconds, v => _tracker.Options.FlingFrameInterval = TimeSpan.FromMilliseconds(v));
519519

520520
// --- Zoom Settings ---
521521
layout.Children.Add(new Label { Text = "Zoom Settings", FontAttributes = FontAttributes.Bold, FontSize = 16, Margin = new Thickness(0, 10, 0, 0) });
522522

523523
AddSlider(layout, "Double Tap Zoom", "x", 1.5f, 5, _tracker.Options.DoubleTapZoomFactor, v => _tracker.Options.DoubleTapZoomFactor = v, "F1");
524-
AddSliderInt(layout, "Zoom Animation", "ms", 50, 1000, _tracker.Options.ZoomAnimationDuration, v => _tracker.Options.ZoomAnimationDuration = v);
524+
AddSliderInt(layout, "Zoom Animation", "ms", 50, 1000, (int)_tracker.Options.ZoomAnimationDuration.TotalMilliseconds, v => _tracker.Options.ZoomAnimationDuration = TimeSpan.FromMilliseconds(v));
525525
AddSlider(layout, "Scroll Zoom Factor", "", 0.01f, 0.5f, _tracker.Options.ScrollZoomFactor, v => _tracker.Options.ScrollZoomFactor = v, "F3");
526526
AddSlider(layout, "Min Scale", "x", 0.1f, 1, _tracker.Options.MinScale, v => _tracker.Options.MinScale = v, "F2");
527527
AddSlider(layout, "Max Scale", "x", 2, 20, _tracker.Options.MaxScale, v => _tracker.Options.MaxScale = v, "F1");

source/SkiaSharp.Extended/Gestures/SKGestureDetector.cs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,8 @@ public bool ProcessTouchDown(long id, SKPoint location, bool isMouse = false)
235235
if (touchPoints.Length >= 2)
236236
{
237237
StopLongPressTimer();
238+
_tapCount = 0;
239+
_lastTapTicks = 0;
238240
_pinchState = PinchState.FromLocations(touchPoints);
239241
_gestureState = GestureState.Pinching;
240242
}
@@ -367,7 +369,7 @@ public bool ProcessTouchUp(long id, SKPoint location, bool isMouse = false)
367369
{
368370
var distance = SKPoint.Distance(location, _initialTouch);
369371
var duration = ticks - _touchStartTicks;
370-
var maxTapDuration = storedIsMouse ? ShortClickTicks : Options.LongPressDuration * TimeSpan.TicksPerMillisecond;
372+
var maxTapDuration = storedIsMouse ? ShortClickTicks : Options.LongPressDuration.Ticks;
371373

372374
if (distance < Options.TouchSlop && duration < maxTapDuration && !_longPressTriggered)
373375
{
@@ -523,7 +525,7 @@ private void StartLongPressTimer()
523525
{
524526
StopLongPressTimer();
525527
var token = Interlocked.Increment(ref _longPressToken);
526-
var timer = new Timer(OnLongPressTimerTick, token, Options.LongPressDuration, Timeout.Infinite);
528+
var timer = new Timer(OnLongPressTimerTick, token, (int)Options.LongPressDuration.TotalMilliseconds, Timeout.Infinite);
527529
_longPressTimer = timer;
528530
}
529531

source/SkiaSharp.Extended/Gestures/SKGestureDetectorOptions.cs

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ public class SKGestureDetectorOptions
1616
private float _touchSlop = 8f;
1717
private float _doubleTapSlop = 40f;
1818
private float _flingThreshold = 200f;
19-
private int _longPressDuration = 500;
19+
private TimeSpan _longPressDuration = TimeSpan.FromMilliseconds(500);
2020

2121
/// <summary>
2222
/// Gets or sets the minimum movement distance, in pixels, before a touch is considered a pan gesture.
@@ -72,17 +72,16 @@ public float FlingThreshold
7272
}
7373

7474
/// <summary>
75-
/// Gets or sets the duration, in milliseconds, a touch must be held stationary before
76-
/// a long press gesture is recognized.
75+
/// Gets or sets the duration a touch must be held stationary before a long press gesture is recognized.
7776
/// </summary>
78-
/// <value>The long press duration in milliseconds. The default is <c>500</c>. Must be positive.</value>
77+
/// <value>The long press duration. The default is <c>500 ms</c>. Must be positive.</value>
7978
/// <exception cref="ArgumentOutOfRangeException"><paramref name="value"/> is zero or negative.</exception>
80-
public int LongPressDuration
79+
public TimeSpan LongPressDuration
8180
{
8281
get => _longPressDuration;
8382
set
8483
{
85-
if (value <= 0)
84+
if (value <= TimeSpan.Zero)
8685
throw new ArgumentOutOfRangeException(nameof(value), value, "LongPressDuration must be positive.");
8786
_longPressDuration = value;
8887
}

source/SkiaSharp.Extended/Gestures/SKGestureTracker.cs

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -472,8 +472,8 @@ public void ZoomTo(float factor, SKPoint focalPoint)
472472
_zoomTimer = new Timer(
473473
OnZoomTimerTick,
474474
token,
475-
Options.ZoomAnimationInterval,
476-
Options.ZoomAnimationInterval);
475+
(int)Options.ZoomAnimationInterval.TotalMilliseconds,
476+
(int)Options.ZoomAnimationInterval.TotalMilliseconds);
477477
}
478478

479479
/// <summary>Stops any active zoom animation immediately.</summary>
@@ -669,7 +669,7 @@ private void OnEnginePanDetected(object? s, SKPanGestureEventArgs e)
669669

670670
private void OnEnginePinchDetected(object? s, SKPinchGestureEventArgs e)
671671
{
672-
if (IsPinchEnabled || IsPanEnabled)
672+
if (IsPinchEnabled)
673673
PinchDetected?.Invoke(this, e);
674674

675675
// Apply center movement as pan
@@ -804,8 +804,8 @@ private void StartFlingAnimation(float velocityX, float velocityY)
804804
_flingTimer = new Timer(
805805
OnFlingTimerTick,
806806
token,
807-
Options.FlingFrameInterval,
808-
Options.FlingFrameInterval);
807+
(int)Options.FlingFrameInterval.TotalMilliseconds,
808+
(int)Options.FlingFrameInterval.TotalMilliseconds);
809809
}
810810

811811
private void OnFlingTimerTick(object? state)
@@ -853,7 +853,7 @@ private void HandleFlingFrame()
853853
TransformChanged?.Invoke(this, EventArgs.Empty);
854854

855855
// Apply time-scaled friction so deceleration is consistent regardless of frame rate
856-
var nominalDtMs = (float)Options.FlingFrameInterval;
856+
var nominalDtMs = (float)Options.FlingFrameInterval.TotalMilliseconds;
857857
var decay = nominalDtMs > 0
858858
? (float)Math.Pow(1.0 - Options.FlingFriction, actualDtMs / nominalDtMs)
859859
: 1f - Options.FlingFriction;
@@ -900,7 +900,7 @@ private void HandleZoomFrame()
900900
return;
901901

902902
var elapsed = TimeProvider() - _zoomStartTicks;
903-
var duration = Options.ZoomAnimationDuration * TimeSpan.TicksPerMillisecond;
903+
var duration = Options.ZoomAnimationDuration.Ticks;
904904
var t = duration > 0 ? Math.Min(1.0, (double)elapsed / duration) : 1.0;
905905

906906
// CubicOut easing: 1 - (1 - t)^3

source/SkiaSharp.Extended/Gestures/SKGestureTrackerOptions.cs

Lines changed: 41 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,12 @@ public class SKGestureTrackerOptions : SKGestureDetectorOptions
1616
private float _minScale = 0.1f;
1717
private float _maxScale = 10f;
1818
private float _doubleTapZoomFactor = 2f;
19-
private int _zoomAnimationDuration = 250;
19+
private TimeSpan _zoomAnimationDuration = TimeSpan.FromMilliseconds(250);
2020
private float _scrollZoomFactor = 0.1f;
2121
private float _flingFriction = 0.08f;
2222
private float _flingMinVelocity = 5f;
23-
private int _flingFrameInterval = 16;
24-
private int _zoomAnimationInterval = 16;
23+
private TimeSpan _flingFrameInterval = TimeSpan.FromMilliseconds(16);
24+
private TimeSpan _zoomAnimationInterval = TimeSpan.FromMilliseconds(16);
2525

2626
/// <summary>
2727
/// Gets or sets the minimum allowed zoom scale.
@@ -59,6 +59,30 @@ public float MaxScale
5959
}
6060
}
6161

62+
/// <summary>
63+
/// Sets both <see cref="MinScale"/> and <see cref="MaxScale"/> atomically,
64+
/// avoiding ordering-dependent validation errors when the desired range lies
65+
/// entirely outside the current default range of [0.1, 10].
66+
/// </summary>
67+
/// <param name="minScale">The minimum scale value. Must be positive and less than <paramref name="maxScale"/>.</param>
68+
/// <param name="maxScale">The maximum scale value. Must be positive and greater than <paramref name="minScale"/>.</param>
69+
/// <exception cref="ArgumentOutOfRangeException">
70+
/// Thrown when <paramref name="minScale"/> is less than or equal to zero, <paramref name="maxScale"/> is less than
71+
/// or equal to zero, or <paramref name="minScale"/> is greater than or equal to <paramref name="maxScale"/>.
72+
/// </exception>
73+
public void SetScaleRange(float minScale, float maxScale)
74+
{
75+
if (minScale <= 0)
76+
throw new ArgumentOutOfRangeException(nameof(minScale), minScale, "MinScale must be positive.");
77+
if (maxScale <= 0)
78+
throw new ArgumentOutOfRangeException(nameof(maxScale), maxScale, "MaxScale must be positive.");
79+
if (minScale >= maxScale)
80+
throw new ArgumentOutOfRangeException(nameof(minScale), minScale, "MinScale must be less than MaxScale.");
81+
82+
_minScale = minScale;
83+
_maxScale = maxScale;
84+
}
85+
6286
/// <summary>
6387
/// Gets or sets the multiplicative zoom factor applied when a double-tap is detected.
6488
/// </summary>
@@ -80,16 +104,16 @@ public float DoubleTapZoomFactor
80104
}
81105

82106
/// <summary>
83-
/// Gets or sets the duration of the double-tap zoom animation, in milliseconds.
107+
/// Gets or sets the duration of the double-tap zoom animation.
84108
/// </summary>
85-
/// <value>The animation duration in milliseconds. The default is <c>250</c>. A value of <c>0</c> applies the zoom instantly.</value>
109+
/// <value>The animation duration. The default is <c>250 ms</c>. A value of <see cref="TimeSpan.Zero"/> applies the zoom instantly.</value>
86110
/// <exception cref="ArgumentOutOfRangeException"><paramref name="value"/> is negative.</exception>
87-
public int ZoomAnimationDuration
111+
public TimeSpan ZoomAnimationDuration
88112
{
89113
get => _zoomAnimationDuration;
90114
set
91115
{
92-
if (value < 0)
116+
if (value < TimeSpan.Zero)
93117
throw new ArgumentOutOfRangeException(nameof(value), value, "ZoomAnimationDuration must not be negative.");
94118
_zoomAnimationDuration = value;
95119
}
@@ -150,38 +174,38 @@ public float FlingMinVelocity
150174
}
151175

152176
/// <summary>
153-
/// Gets or sets the fling animation frame interval, in milliseconds.
177+
/// Gets or sets the fling animation frame interval.
154178
/// </summary>
155179
/// <value>
156-
/// The timer interval between fling animation frames in milliseconds.
157-
/// The default is <c>16</c> (approximately 60 FPS). Must be positive.
180+
/// The timer interval between fling animation frames.
181+
/// The default is <c>16 ms</c> (approximately 60 FPS). Must be positive.
158182
/// </value>
159183
/// <exception cref="ArgumentOutOfRangeException"><paramref name="value"/> is zero or negative.</exception>
160-
public int FlingFrameInterval
184+
public TimeSpan FlingFrameInterval
161185
{
162186
get => _flingFrameInterval;
163187
set
164188
{
165-
if (value <= 0)
189+
if (value <= TimeSpan.Zero)
166190
throw new ArgumentOutOfRangeException(nameof(value), value, "FlingFrameInterval must be positive.");
167191
_flingFrameInterval = value;
168192
}
169193
}
170194

171195
/// <summary>
172-
/// Gets or sets the zoom animation frame interval, in milliseconds.
196+
/// Gets or sets the zoom animation frame interval.
173197
/// </summary>
174198
/// <value>
175-
/// The timer interval between zoom animation frames in milliseconds.
176-
/// The default is <c>16</c> (approximately 60 FPS). Must be positive.
199+
/// The timer interval between zoom animation frames.
200+
/// The default is <c>16 ms</c> (approximately 60 FPS). Must be positive.
177201
/// </value>
178202
/// <exception cref="ArgumentOutOfRangeException"><paramref name="value"/> is zero or negative.</exception>
179-
public int ZoomAnimationInterval
203+
public TimeSpan ZoomAnimationInterval
180204
{
181205
get => _zoomAnimationInterval;
182206
set
183207
{
184-
if (value <= 0)
208+
if (value <= TimeSpan.Zero)
185209
throw new ArgumentOutOfRangeException(nameof(value), value, "ZoomAnimationInterval must be positive.");
186210
_zoomAnimationInterval = value;
187211
}

tests/SkiaSharp.Extended.Tests/Gestures/SKGestureDetectorTapTests.cs

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ public void DoubleTap_TapCountIsTwo()
118118
public async Task LongTouch_RaisesLongPressDetected()
119119
{
120120
var engine = new SKGestureDetector();
121-
engine.Options.LongPressDuration = 100; // Short duration for testing
121+
engine.Options.LongPressDuration = TimeSpan.FromMilliseconds(100); // Short duration for testing
122122
var longPressRaised = false;
123123
engine.LongPressDetected += (s, e) => longPressRaised = true;
124124

@@ -133,7 +133,7 @@ public async Task LongTouch_RaisesLongPressDetected()
133133
public async Task LongPress_DoesNotRaiseTapOnRelease()
134134
{
135135
var engine = new SKGestureDetector();
136-
engine.Options.LongPressDuration = 100;
136+
engine.Options.LongPressDuration = TimeSpan.FromMilliseconds(100);
137137
var tapRaised = false;
138138
var longPressRaised = false;
139139
engine.TapDetected += (s, e) => tapRaised = true;
@@ -152,7 +152,7 @@ public async Task LongPress_DoesNotRaiseTapOnRelease()
152152
public async Task LongPressDuration_CanBeCustomized()
153153
{
154154
var engine = new SKGestureDetector();
155-
engine.Options.LongPressDuration = 300;
155+
engine.Options.LongPressDuration = TimeSpan.FromMilliseconds(300);
156156
var longPressRaised = false;
157157
engine.LongPressDetected += (s, e) => longPressRaised = true;
158158

@@ -200,5 +200,34 @@ public void TouchHeld_WithSmallMoves_BeyondLongPressDuration_DoesNotFireTap()
200200
Assert.False(tapRaised, "Touch held too long should not fire tap");
201201
}
202202

203+
[Fact]
204+
public void TapThenPinchThenTap_DoesNotFireDoubleTap()
205+
{
206+
// Bug fix: _tapCount was not cleared when entering Pinching state, causing
207+
// a false DoubleTapDetected on the subsequent single tap.
208+
var engine = CreateEngine();
209+
var doubleTapRaised = false;
210+
engine.DoubleTapDetected += (s, e) => doubleTapRaised = true;
211+
212+
// First tap
213+
engine.ProcessTouchDown(1, new SKPoint(100, 100));
214+
engine.ProcessTouchUp(1, new SKPoint(100, 100));
215+
216+
// Immediately start a pinch (within double-tap time window)
217+
AdvanceTime(100); // < 300ms double-tap delay
218+
engine.ProcessTouchDown(1, new SKPoint(100, 100));
219+
engine.ProcessTouchDown(2, new SKPoint(200, 100));
220+
engine.ProcessTouchMove(1, new SKPoint(80, 100));
221+
engine.ProcessTouchMove(2, new SKPoint(220, 100));
222+
engine.ProcessTouchUp(2, new SKPoint(220, 100));
223+
engine.ProcessTouchUp(1, new SKPoint(80, 100));
224+
225+
// Third tap shortly after pinch completes (still within original 300ms window from first tap)
226+
AdvanceTime(50);
227+
engine.ProcessTouchDown(1, new SKPoint(100, 100));
228+
engine.ProcessTouchUp(1, new SKPoint(100, 100));
229+
230+
Assert.False(doubleTapRaised, "DoubleTapDetected must not fire after tap → pinch → single tap sequence");
231+
}
203232

204233
}

tests/SkiaSharp.Extended.Tests/Gestures/SKGestureDetectorTests.cs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -749,7 +749,7 @@ public void Options_FlingThreshold_Negative_Throws()
749749
public void Options_LongPressDuration_ZeroOrNegative_Throws(int value)
750750
{
751751
var options = new SKGestureDetectorOptions();
752-
Assert.Throws<ArgumentOutOfRangeException>(() => options.LongPressDuration = value);
752+
Assert.Throws<ArgumentOutOfRangeException>(() => options.LongPressDuration = TimeSpan.FromMilliseconds(value));
753753
}
754754

755755
[Fact]
@@ -766,14 +766,14 @@ public void Options_ValidValues_PassThrough()
766766
TouchSlop = 16f,
767767
DoubleTapSlop = 80f,
768768
FlingThreshold = 400f,
769-
LongPressDuration = 1000,
769+
LongPressDuration = TimeSpan.FromSeconds(1),
770770
};
771771
var engine = new SKGestureDetector(options);
772772

773773
Assert.Equal(16f, engine.Options.TouchSlop);
774774
Assert.Equal(80f, engine.Options.DoubleTapSlop);
775775
Assert.Equal(400f, engine.Options.FlingThreshold);
776-
Assert.Equal(1000, engine.Options.LongPressDuration);
776+
Assert.Equal(TimeSpan.FromSeconds(1), engine.Options.LongPressDuration);
777777
}
778778

779779

@@ -929,7 +929,7 @@ public void LongPressTimer_NotRestarted_OnSecondFingerDown()
929929
// Regression: StartLongPressTimer() was called for every ProcessTouchDown,
930930
// including the 2nd finger, resetting the long-press timer during pinch start.
931931
var engine = CreateEngine();
932-
engine.Options.LongPressDuration = 200; // Short for test
932+
engine.Options.LongPressDuration = TimeSpan.FromMilliseconds(200); // Short for test
933933

934934
var longPressCount = 0;
935935
engine.LongPressDetected += (s, e) => longPressCount++;

tests/SkiaSharp.Extended.Tests/Gestures/SKGestureTrackerFlingTests.cs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ public void FastSwipe_FiresFlingDetected()
5454
public async Task Fling_FiresFlingUpdatedEvents()
5555
{
5656
var tracker = CreateTracker();
57-
tracker.Options.FlingFrameInterval = 16;
57+
tracker.Options.FlingFrameInterval = TimeSpan.FromMilliseconds(16);
5858
var flingUpdatedCount = 0;
5959
tracker.FlingUpdated += (s, e) => flingUpdatedCount++;
6060

@@ -70,7 +70,7 @@ public async Task Fling_FiresFlingUpdatedEvents()
7070
public async Task Fling_UpdatesOffset()
7171
{
7272
var tracker = CreateTracker();
73-
tracker.Options.FlingFrameInterval = 16;
73+
tracker.Options.FlingFrameInterval = TimeSpan.FromMilliseconds(16);
7474
var flingUpdatedFired = false;
7575
tracker.FlingUpdated += (s, e) => flingUpdatedFired = true;
7676

@@ -118,7 +118,7 @@ public async Task Fling_EventuallyCompletes()
118118
{
119119
// Use real TimeProvider so fling frame timing advances with wall-clock time
120120
var tracker = new SKGestureTracker();
121-
tracker.Options.FlingFrameInterval = 16;
121+
tracker.Options.FlingFrameInterval = TimeSpan.FromMilliseconds(16);
122122
tracker.Options.FlingFriction = 0.5f;
123123
tracker.Options.FlingMinVelocity = 100f;
124124
var flingCompleted = false;

0 commit comments

Comments
 (0)