Conversation
|
You can test this PR using the following package version. |
344b20e to
c07d2c3
Compare
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
bb3444b to
9823926
Compare
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
…enu show behavior for selection handles
1523fc7 to
45b9fe2
Compare
|
You can test this PR using the following package version. |
|
|
||
| public void Dispose() | ||
| { | ||
| _sinceLastSample.Stop(); |
There was a problem hiding this comment.
Note: it's not technically required to Stop a Stopwatch, they don't do anything until measured.
|
|
||
| private void UpdateHandleClasses() | ||
| { | ||
| Classes.Remove("caret"); |
There was a problem hiding this comment.
Standard classes should not be set directly, these are theme-specific. However, I see no issues in converting those to pseudo-classes instead.
| flyout.Hide(); | ||
| private List<Visual> _attachedVisuals = new List<Visual>(); | ||
| private TextPresenter? _presenter; | ||
| private object _lock = new object(); |
There was a problem hiding this comment.
The lock should not be needed. Everything here is dispatcher-bound, and accessing the controls from another thread is not supported.
| { | ||
| if (visual is Layoutable layoutable) | ||
| { | ||
| layoutable.EffectiveViewportChanged += Visual_EffectiveViewportChanged; |
There was a problem hiding this comment.
Why do we need to listen to every parent? First, this is usually very brittle (it won't work correctly when the visual tree changes). Plus, if the viewport for the presenter itself hasn't actually changed, why is the invalidation needed?
| /// <summary> | ||
| /// The radius for touch input. Used to determine if selection should change from moving a touch pointer. | ||
| /// </summary> | ||
| private readonly static int s_touchRadius = (int)((AvaloniaLocator.Current?.GetService<IPlatformSettings>()?.GetTapSize(PointerType.Touch).Height ?? 10) / 2) + 5; |
There was a problem hiding this comment.
We should not access the locator in a static initializer. There is no guarantee that the locator will be correctly set, the runtime is free to run static initializers at any point. Consider making that lazy instead.
| private static readonly bool s_shouldWrapAroundSelection; | ||
|
|
||
| private const int ContextMenuPadding = 16; | ||
| private static bool s_isInTouchMode; |
There was a problem hiding this comment.
This is static but updated by instance specific code. Consider making it non-static.
| if(!e.Handled) | ||
| { | ||
| // Gesture may cause an overscroll so we mark the event as handled if it did. | ||
| e.Handled = canXScroll || canYScroll; |
There was a problem hiding this comment.
I'm not sure I understand the logic here, won't that prevent scroll chaining in this case?
There was a problem hiding this comment.
Confirmed while testing, scroll chaining does not work correctly anymore.
|
There's a regression when a selection is active with the context menu open. Previously, double-tapping another word would select it in this case. Now, you need 3 taps instead (one to close the popup, then two for the normal double-tap selection to activate). Please add a test for this once fixed. |
|
I've tested this with a touch screen, and overall, the changes are quite good :) We need more unit tests overall for this. This is not the easiest area to test, but we should be able to validate most of the behaviors added by this PR. They are too easy to break with future changes otherwise. |
What does the pull request do?
This PR adds a lot of improvements for touch controls for textbox. Pointer handling for Touch input has been reworked to feel more natural for touch based devices. New behavior closely matches Android's default touch behavior for text inputs.
The following changes and improvements have been made;
PathIconreplaced withImageWhat is the current behavior?
What is the updated/expected behavior with this PR?
How was the solution implemented (if it's not obvious)?
Checklist
Breaking changes
TextSelectionHandlecontrol theme is updated.Obsoletions / Deprecations
Fixed issues