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.

Sign up for GitHub

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

[SuperEditor][SuperReader][Android] Fix scroll physics (Resolves #1539) #1541

Merged
merged 13 commits into from
Nov 21, 2023
Merged

[SuperEditor][SuperReader][Android] Fix scroll physics (Resolves #1539) #1541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f6a8d9b
[SuperEditor][SuperReader][Android] Fix scroll physics (Resolves #1539)
angelosilvestre Oct 21, 2023
f4c2f36
User ScrollPosition.drag to perform scrolling
angelosilvestre Oct 24, 2023
1648990
Fix merge conflicts
angelosilvestre Oct 24, 2023
97cd440
PR updates
angelosilvestre Oct 24, 2023
5dcc192
PR updates
angelosilvestre Oct 25, 2023
e4ed3c5
Refactor iOS pan end
angelosilvestre Oct 30, 2023
1e8f286
Create extension for drag gestures
angelosilvestre Oct 31, 2023
3a86384
PR Updates
angelosilvestre Nov 2, 2023
e0cf920
Update comments
angelosilvestre Nov 7, 2023
7097060
Add test configuration to use a CustomScrollView
angelosilvestre Nov 7, 2023
9ab59dc
Add docs
angelosilvestre Nov 7, 2023
8c19084
PR updates
angelosilvestre Nov 13, 2023
5378373
Use withScrollController to define ancestor scrollable controller
angelosilvestre Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 27 additions & 9 deletions super_editor/lib/src/default_editor/document_gestures_touch_android.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,10 @@ class _AndroidDocumentTouchInteractorState extends State<AndroidDocumentTouchInt
double? _dragStartScrollOffset;
Offset? _globalDragOffset;
Offset? _dragEndInInteractor;

/// Holds the drag gesture that scrolls the document.
Drag? _scrollingDrag;

SelectionHandleType? _selectionType;

Timer? _tapDownLongPressTimer;
Expand Down Expand Up @@ -740,6 +744,11 @@ class _AndroidDocumentTouchInteractorState extends State<AndroidDocumentTouchInt

if (!_isLongPressInProgress) {
// We only care about starting a pan if we're long-press dragging.
_scrollingDrag = scrollPosition.drag(details, () {
// Allows receiving touches while scrolling due to scroll momentum.
// This is needed to allow the user to stop scrolling by tapping down.
scrollPosition.context.setIgnorePointer(false);
});
return;
}

Expand Down Expand Up @@ -784,8 +793,10 @@ class _AndroidDocumentTouchInteractorState extends State<AndroidDocumentTouchInt
return;
}

// The user is trying to scroll the document. Change the scroll offset.
scrollPosition.jumpTo(scrollPosition.pixels - details.delta.dy);
if (_scrollingDrag != null) {
// The user is trying to scroll the document. Change the scroll offset.
_scrollingDrag!.update(details);
}
}

void _updateLongPressSelection(DocumentSelection newSelection) {
Expand Down Expand Up @@ -816,10 +827,10 @@ class _AndroidDocumentTouchInteractorState extends State<AndroidDocumentTouchInt
return;
}

final pos = scrollPosition;
if (pos is ScrollPositionWithSingleContext) {
pos.goBallistic(-details.velocity.pixelsPerSecond.dy);
pos.context.setIgnorePointer(false);
if (_scrollingDrag != null) {
// The user was performing a drag gesture to scroll the document.
// End the scroll activity and let the document scrolling with momentum.
_scrollingDrag!.end(details);
}
}

Expand All @@ -828,6 +839,12 @@ class _AndroidDocumentTouchInteractorState extends State<AndroidDocumentTouchInt
_onLongPressEnd();
return;
}

if (_scrollingDrag != null) {
// The user was performing a drag gesture to scroll the document.
// End the drag gesture.
_scrollingDrag!.cancel();
}
}

void _onLongPressEnd() {
Expand Down Expand Up @@ -1251,10 +1268,11 @@ class _AndroidDocumentTouchInteractorState extends State<AndroidDocumentTouchInt
..gestureSettings = gestureSettings;
},
),
PanGestureRecognizer: GestureRecognizerFactoryWithHandlers<PanGestureRecognizer>(
() => PanGestureRecognizer(),
(PanGestureRecognizer recognizer) {
VerticalDragGestureRecognizer: GestureRecognizerFactoryWithHandlers<VerticalDragGestureRecognizer>(
() => VerticalDragGestureRecognizer(),
(VerticalDragGestureRecognizer recognizer) {
recognizer
..dragStartBehavior = DragStartBehavior.down
..onStart = _onPanStart
..onUpdate = _onPanUpdate
..onEnd = _onPanEnd
Expand Down
71 changes: 52 additions & 19 deletions super_editor/lib/src/default_editor/document_gestures_touch_ios.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,9 @@ class _IosDocumentTouchInteractorState extends State<IosDocumentTouchInteractor>
// not collapsed/upstream/downstream. Change the type once it's working.
HandleType? _dragHandleType;

/// Holds the drag gesture that scrolls the document.
Drag? _scrollingDrag;

Timer? _tapDownLongPressTimer;
Offset? _globalTapDownOffset;
bool get _isLongPressInProgress => _longPressStrategy != null;
Expand Down Expand Up @@ -810,6 +813,8 @@ class _IosDocumentTouchInteractorState extends State<IosDocumentTouchInteractor>
// bit of slop might be the problem.
final selection = widget.selection.value;
if (selection == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

By removing this null check, you've allowed all following behaviors to execute with a null selection. Is that really what you intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Those behaviors won't be executed. _isOverBaseHandle and _isOverExtentHandle return false if there isn't a selection.

Also, I added a guard clause after all of the if statements to return early if there isn't a selection.

Copy link
Contributor

Choose a reason for hiding this comment

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

The change seems likely to create confusion when reading the code. We have if-statements that check if the user is pressing a handle, but moving this selection check creates the possibility that there is no handle, which probably isn't expected when you're reading code that checks for it.

Is there are more expectable order of code that won't lead to surprises?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated.

// There isn't a selection, the user is dragging to scroll the document.
_startDragScrolling(details);
return;
}

Expand All @@ -827,6 +832,10 @@ class _IosDocumentTouchInteractorState extends State<IosDocumentTouchInteractor>
_dragMode = DragMode.extent;
_dragHandleType = HandleType.downstream;
} else {
// The user isn't dragging over a handle.
// Start scrolling the document.
_startDragScrolling(details);

return;
}

Expand Down Expand Up @@ -909,10 +918,9 @@ class _IosDocumentTouchInteractorState extends State<IosDocumentTouchInteractor>
}

void _onPanUpdate(DragUpdateDetails details) {
// If the user isn't dragging a handle, then the user is trying to
// scroll the document. Scroll it, accordingly.
if (_dragMode == null) {
scrollPosition.jumpTo(scrollPosition.pixels - details.delta.dy);
if (_dragMode == DragMode.scroll) {
// The user is trying to scroll the document. Scroll it, accordingly.
_scrollingDrag!.update(details);
return;
}

Expand Down Expand Up @@ -990,27 +998,39 @@ class _IosDocumentTouchInteractorState extends State<IosDocumentTouchInteractor>
..hideMagnifier()
..blinkCaret();

if (_dragMode == null) {
// User was dragging the scroll area. Go ballistic.
if (scrollPosition is ScrollPositionWithSingleContext) {
(scrollPosition as ScrollPositionWithSingleContext).goBallistic(-details.velocity.pixelsPerSecond.dy);

if (_activeScrollPosition != scrollPosition) {
// We add the scroll change listener again, because going ballistic
// seems to switch out the scroll position.
_activeScrollPosition = scrollPosition;
}
}
} else {
// The user was dragging a selection change in some way, either with handles
// or with a long-press. Finish that interaction.
_onDragSelectionEnd();
switch (_dragMode) {
case DragMode.scroll:
// The user was performing a drag gesture to scroll the document.
// End the scroll activity and let the document scrolling with momentum.
_scrollingDrag!.end(details);
_scrollingDrag = null;
_dragMode = null;
break;
case DragMode.collapsed:
case DragMode.base:
case DragMode.extent:
case DragMode.longPress:
// The user was dragging a selection change in some way, either with handles
// or with a long-press. Finish that interaction.
_onDragSelectionEnd();
break;
case null:
// The user wasn't dragging over a selection. Do nothing.
break;
}
}

void _onPanCancel() {
_magnifierOffset.value = null;

if (_scrollingDrag != null) {
// The user was performing a drag gesture to scroll the document.
// Cancel the drag gesture.
_scrollingDrag!.cancel();
_scrollingDrag = null;
return;
}

if (_dragMode != null) {
_onDragSelectionEnd();
}
Expand Down Expand Up @@ -1207,6 +1227,17 @@ class _IosDocumentTouchInteractorState extends State<IosDocumentTouchInteractor>
return ancestorScrollable;
}

/// Starts a drag activity to scroll the document.
void _startDragScrolling(DragStartDetails details) {
_dragMode = DragMode.scroll;

_scrollingDrag = scrollPosition.drag(details, () {
// Allows receiving touches while scrolling due to scroll momentum.
// This is needed to allow the user to stop scrolling by tapping down.
scrollPosition.context.setIgnorePointer(false);
});
}

@override
Widget build(BuildContext context) {
if (widget.scrollController.hasClients) {
Expand Down Expand Up @@ -1299,6 +1330,8 @@ enum DragMode {
// Dragging after a long-press, which selects by the word
// around the selected word.
longPress,
// Dragging to scroll the document.
scroll
}

/// Adds and removes an iOS-style editor toolbar, as dictated by an ancestor
Expand Down
34 changes: 26 additions & 8 deletions super_editor/lib/src/super_reader/read_only_document_android_touch_interactor.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,9 @@ class _ReadOnlyAndroidDocumentTouchInteractorState extends State<ReadOnlyAndroid
AndroidDocumentLongPressSelectionStrategy? _longPressStrategy;
final _longPressMagnifierGlobalOffset = ValueNotifier<Offset?>(null);

/// Holds the drag gesture that scrolls the document.
Drag? _scrollingDrag;

@override
void initState() {
super.initState();
Expand Down Expand Up @@ -674,6 +677,11 @@ class _ReadOnlyAndroidDocumentTouchInteractorState extends State<ReadOnlyAndroid

if (!_isLongPressInProgress) {
// We only care about starting a pan if we're long-press dragging.
_scrollingDrag = scrollPosition.drag(details, () {
// Allows receiving touches while scrolling due to scroll momentum.
// This is needed to allow the user to stop scrolling by tapping down.
scrollPosition.context.setIgnorePointer(false);
});
return;
}

Expand Down Expand Up @@ -718,7 +726,10 @@ class _ReadOnlyAndroidDocumentTouchInteractorState extends State<ReadOnlyAndroid
return;
}

scrollPosition.jumpTo(scrollPosition.pixels - details.delta.dy);
if (_scrollingDrag != null) {
// The user is trying to scroll the document. Change the scroll offset.
_scrollingDrag!.update(details);
}
}

void _updateLongPressSelection(DocumentSelection newSelection) {
Expand Down Expand Up @@ -749,10 +760,10 @@ class _ReadOnlyAndroidDocumentTouchInteractorState extends State<ReadOnlyAndroid
return;
}

final pos = scrollPosition;
if (pos is ScrollPositionWithSingleContext) {
pos.goBallistic(-details.velocity.pixelsPerSecond.dy);
pos.context.setIgnorePointer(false);
if (_scrollingDrag != null) {
// The user was performing a drag gesture to scroll the document.
// End the scroll activity and let the document scrolling with momentum.
_scrollingDrag!.end(details);
}
}

Expand All @@ -761,6 +772,12 @@ class _ReadOnlyAndroidDocumentTouchInteractorState extends State<ReadOnlyAndroid
_onLongPressEnd();
return;
}

if (_scrollingDrag != null) {
// The user was performing a drag gesture to scroll the document.
// Cancel the drag gesture.
_scrollingDrag!.cancel();
}
}

void _onLongPressEnd() {
Expand Down Expand Up @@ -1042,10 +1059,11 @@ class _ReadOnlyAndroidDocumentTouchInteractorState extends State<ReadOnlyAndroid
..gestureSettings = gestureSettings;
},
),
PanGestureRecognizer: GestureRecognizerFactoryWithHandlers<PanGestureRecognizer>(
() => PanGestureRecognizer(),
(PanGestureRecognizer recognizer) {
VerticalDragGestureRecognizer: GestureRecognizerFactoryWithHandlers<VerticalDragGestureRecognizer>(
() => VerticalDragGestureRecognizer(),
(VerticalDragGestureRecognizer recognizer) {
recognizer
..dragStartBehavior = DragStartBehavior.down
..onStart = _onPanStart
..onUpdate = _onPanUpdate
..onEnd = _onPanEnd
Expand Down
Loading
Loading