Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use tap size as default size for scrolling start. reset IsGestureRecognitionSkipped when pointer is released #15524

Merged
merged 3 commits into from Apr 30, 2024

Conversation

emmauss
Copy link
Contributor

@emmauss emmauss commented Apr 26, 2024

What does the pull request do?

Sets the default ScrollStartDistance to the Platform settings' TapSize. Resets Pointer.IsGestureRecognitionSkipped of a pointer when pointer is released.

What is the current behavior?

The default value for ScrollStartDistance was 30. This is too large to detect for scrolling on most touch devices, and in some cases, controls maye be smaller than 30 and thus will force the scroll gesture recognizer to lose the pointer as the gesture may go over another control before it start to trigger scrolling.

What is the updated/expected behavior with this PR?

How was the solution implemented (if it's not obvious)?

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

Fixes #15288

@emmauss emmauss requested a review from maxkatz6 April 26, 2024 11:16
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0047794-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@@ -13,7 +14,7 @@ public class ScrollGestureRecognizer : GestureRecognizer
private bool _canHorizontallyScroll;
private bool _canVerticallyScroll;
private bool _isScrollInertiaEnabled;
private int _scrollStartDistance = 30;
private int _scrollStartDistance = (int)((AvaloniaLocator.Current?.GetService<IPlatformSettings>()?.GetTapSize(PointerType.Touch).Height ?? 10) / 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private int _scrollStartDistance = (int)((AvaloniaLocator.Current?.GetService<IPlatformSettings>()?.GetTapSize(PointerType.Touch).Height ?? 10) / 2);
private readonly static int s_defatultScrollStartDistance = ((AvaloniaLocator.Current?.GetService<IPlatformSettings>()?.GetTapSize(PointerType.Touch).Height ?? 10) / 2);
private int _scrollStartDistance = s_defatultScrollStartDistance;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -54,7 +55,7 @@ public class ScrollGestureRecognizer : GestureRecognizer
public static readonly DirectProperty<ScrollGestureRecognizer, int> ScrollStartDistanceProperty =
AvaloniaProperty.RegisterDirect<ScrollGestureRecognizer, int>(nameof(ScrollStartDistance),
o => o.ScrollStartDistance, (o, v) => o.ScrollStartDistance = v,
unsetValue: 30);
unsetValue: (int)(AvaloniaLocator.Current.GetService<IPlatformSettings>()?.GetTapSize(PointerType.Touch).Height ?? 30));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it different form _scrollStartDistance?

private int _scrollStartDistance = (int)((AvaloniaLocator.Current?.GetService<IPlatformSettings>()?.GetTapSize(PointerType.Touch).Height ?? 10) / 2);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been changed to match.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0047808-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@Meloman19
Copy link
Contributor

Meloman19 commented Apr 26, 2024

I think it's better to initialize in the constructor (not static). IPlatfromSettings may be registered later than the initialization of a static variable and the value will be irrelevant.

Or even better, read out this parameter every time before use to take the current value. Because it can change.

Something like that.
// ScrollStartDistance is nullable. Because it will be used as a custom value.
private int? _scrollStartDistance = null;

// Add local field
private Size _pointerScrollStartDistance;

public static readonly DirectProperty<ScrollGestureRecognizer, int?> ScrollStartDistanceProperty =
            AvaloniaProperty.RegisterDirect<ScrollGestureRecognizer, int?>(nameof(ScrollStartDistance),
                o => o.ScrollStartDistance, (o, v) => o.ScrollStartDistance = v,
                unsetValue: null);

public int? ScrollStartDistance
{
    get => _scrollStartDistance;
    set => SetAndRaise(ScrollStartDistanceProperty, ref _scrollStartDistance, value);
}

protected override void PointerPressed(PointerPressedEventArgs e)
{
    if (e.Pointer.Type == PointerType.Touch || e.Pointer.Type == PointerType.Pen)
    {
        // ...
        // Init pointerScrollStartDistance on each recognize start
        // First, we check custom ScrollStartDistance. If it is set, then we use it.
        // Or use PlatformSettings depend on pointer type, or some default value
        _pointerScrollStartDistance = ScrollStartDistance != null
            ? new Size(ScrollStartDistance.Value, ScrollStartDistance.Value) 
            : ((AvaloniaLocator.Current.GetService<IPlatformSettings>()?.GetTapSize(e.Pointer.Type) / 2) ?? new Size(10, 10));
    }
}

protected override void PointerMoved(PointerEventArgs e)
{
    if (e.Pointer == _tracking)
    {
        var rootPoint = e.GetPosition(_rootTarget);
        if (!_scrolling)
        {
            // The vertical and horizontal values may differ, so we check them separately.
            // Width for X axis
            if (CanHorizontallyScroll && Math.Abs(_trackedRootPoint.X - rootPoint.X) > _pointerScrollStartDistance.Width)
                _scrolling = true;
            // Height for Y axis
            if (CanVerticallyScroll && Math.Abs(_trackedRootPoint.Y - rootPoint.Y) > _pointerScrollStartDistance.Height)
                _scrolling = true;
            if (_scrolling)
            {
                _trackedRootPoint = new Point(
                    _trackedRootPoint.X - (_trackedRootPoint.X >= rootPoint.X ? _pointerScrollStartDistance.Width : -_pointerScrollStartDistance.Width),
                    _trackedRootPoint.Y - (_trackedRootPoint.Y >= rootPoint.Y ? _pointerScrollStartDistance.Height : -_pointerScrollStartDistance.Height));

                Capture(e.Pointer);
            }
        }

        // ...
    }
}

@@ -13,7 +14,8 @@ public class ScrollGestureRecognizer : GestureRecognizer
private bool _canHorizontallyScroll;
private bool _canVerticallyScroll;
private bool _isScrollInertiaEnabled;
private int _scrollStartDistance = 30;
private readonly static int s_defatultScrollStartDistance = (int)((AvaloniaLocator.Current?.GetService<IPlatformSettings>()?.GetTapSize(PointerType.Touch).Height ?? 10) / 2);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spelling s_defatultScrollStartDistance default is wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@emmauss
Copy link
Contributor Author

emmauss commented Apr 29, 2024

I think it's better to initialize in the constructor (not static). IPlatfromSettings may be registered later than the initialization of a static variable and the value will be irrelevant.

Or even better, read out this parameter every time before use to take the current value. Because it can change.
Something like that.

IPlatformSettings is not expected to be changed during runtime. Also, changing the value to a nullable int will be a breaking change at this point.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0047859-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6
Copy link
Member

IPlatformSettings isn't expected to be changed, correct. It's not a service re-definable by user either. And we don't guarantee Avalonia types usable before initialization.

But in general, it's a good point for property default value factories: #5315

@maxkatz6 maxkatz6 merged commit a17e76e into master Apr 30, 2024
12 checks passed
@maxkatz6 maxkatz6 deleted the touch_menu branch April 30, 2024 00:33
@maxkatz6 maxkatz6 added the backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch label Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Touch scrolling in menus broken in 11.0.5+
6 participants