-
Notifications
You must be signed in to change notification settings - Fork 249
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
Update SuperEditor and SuperReader layer system to provide app-level composition (iOS)(Relates to #424, #893, #1166)(Resolves #1508) #1470
Update SuperEditor and SuperReader layer system to provide app-level composition (iOS)(Relates to #424, #893, #1166)(Resolves #1508) #1470
Conversation
…ystem (Resolves #424) - Applied floating cursor expanded selection fix from another PR. Fixed floating cursor auto-scroll bottom boundary position bug. Also debugging a floating cursor document mapping error when moving between nodes. - iOS controls configurability from outside Super Editor - Moving complexity out of IosDocumentTouchInteractor. Currently trying to remove all possible dependencies on the shared and centralized IosDocumentGestureEditingController. All tests pass. - Removed a bunch of unneeded properties from IosDocumentTouchInteractor. All tests pass. - Refactored SuperReader to use new layer controls approach. All tests pass.
… follow_the_leader updated to fix arrow alignment. All tests pass.
- merged in long-press selection for iOS and Android
…Link within the iOS controls scope.
… to eventually migrate everything to LeaderLinks.
…kenly using the Interactors context. Changed iOS toolbar overlay manager to require the focal point because the Overlay can't ever be a descendent of the iOS controls scope.
…ixed an issue where collapsed selections were reported as expanded because text positions for the same offset with different affinities reported as unequal.
…preparation to cleanup how the floating cursor is managed across various widgets and classes.
…t responsibility into EditorFloatingCursor.
…ow the magnifier builder works - also added default toolbar builders to SuperEditor and SuperReader, which are used when the toolbar builder is null.
- recovered iOS controls hidden on web for SuperEditor - added inspector for overlay controls in SuperEditor - added tests for hidden controls on web for SuperEditor
…andlesDocumentLayerBuilder Renamed IosControlsDocumentLayer to IosHandlesDocumentLayer
@angelosilvestre this is a really big PR. If you want to review it in an unusual way, let me know. We can review by behavior, or by small groups of files. We can also jump on a call at some point if you think that's necessary for the review. I'd like to get this merged ASAP, but I also know that this review is gonna be a lot of work. |
CC @miguelcmedeiros @brian-superlist @Jethro87 @jmatth - this PR changes the architecture for how |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look through all of the code yet, but I left some initial comments.
super_editor/example/lib/demos/editor_configs/demo_mobile_editing_ios.dart
Outdated
Show resolved
Hide resolved
super_editor/example/lib/demos/interaction_spot_checks/toolbar_following_content_in_layer.dart
Outdated
Show resolved
Hide resolved
super_editor/example/lib/demos/interaction_spot_checks/toolbar_following_content_in_layer.dart
Outdated
Show resolved
Hide resolved
super_editor/example/lib/demos/interaction_spot_checks/toolbar_following_content_in_layer.dart
Outdated
Show resolved
Hide resolved
super_editor/lib/src/default_editor/document_gestures_touch_ios.dart
Outdated
Show resolved
Hide resolved
super_editor/lib/src/default_editor/document_gestures_touch_ios.dart
Outdated
Show resolved
Hide resolved
_controlsContext!.floatingCursorController.cursorGeometryInViewport | ||
.removeListener(_onFloatingCursorGeometryChange); | ||
} | ||
_controlsContext = SuperEditorIosControlsScope.rootOf(context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we be looking for the nearest InheritedWidget
? I assume SuperEditor
is always adding its own SuperEditorIosControlsScope
in the widget tree. If that's the case, it could add the SuperEditorIosControlsScope
only if there isn't an ancestor one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we'd use the nearest because that's more performant. You've described a solution for the case of two scopes, one inside SuperEditor
and one outside SuperEditor
. But what about when there's 3, 4, or 5 scopes? Which one do we use? The nearest or the root? I think the assumption of such a scope is that the outermost scope is the one that takes priority...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, when working with InheritedWidget
s I think the assumption is that the nearest is always used...
But what about when there's 3, 4, or 5 scopes? Which one do we use?
My guess is that we should always use the nearest. If we always look for the outermost scope, then it doesn't matter if we have 2 scopes or 10.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But how does that policy facilitate our users? Imagine that Package includes a SuperEditor
and wraps it with an iOS controls scope to customize the controls. Then App uses Package which uses SuperEditor. Now the app wants to further change those controls by wrapping with another iOS controls scope. That won't have any effect if we look for the nearest ancestor.
What would App do to customize the controls within Package, which customized the controls within SuperEditor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, with the given example it makes sense to pick the outermost scope.
super_editor/lib/src/default_editor/document_gestures_touch_ios.dart
Outdated
Show resolved
Hide resolved
… show/hide/toggle for clarity
… and dontBlinkCaret()
…d SuperReader and deprecated them to avoid unexpected compilation issues
_controlsContext!.floatingCursorController.cursorGeometryInViewport | ||
.removeListener(_onFloatingCursorGeometryChange); | ||
} | ||
_controlsContext = SuperEditorIosControlsScope.rootOf(context); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, when working with InheritedWidget
s I think the assumption is that the nearest is always used...
But what about when there's 3, 4, or 5 scopes? Which one do we use?
My guess is that we should always use the nearest. If we always look for the outermost scope, then it doesn't matter if we have 2 scopes or 10.
super_editor/lib/src/infrastructure/platforms/ios/ios_document_controls.dart
Show resolved
Hide resolved
super_editor/lib/src/super_reader/read_only_document_ios_touch_interactor.dart
Outdated
Show resolved
Hide resolved
super_editor/lib/src/test/super_editor_test/supereditor_inspector.dart
Outdated
Show resolved
Hide resolved
super_editor/test/super_editor/mobile/super_editor_ios_overlay_controls_test.dart
Outdated
Show resolved
Hide resolved
* Brought back iOS SuperTextField toolbar focus arrow, but now using Leader/Follower instead of direct offsets
super_editor/lib/src/default_editor/document_gestures_touch_ios.dart
Outdated
Show resolved
Hide resolved
super_editor/lib/src/default_editor/document_gestures_touch_ios.dart
Outdated
Show resolved
Hide resolved
super_editor/lib/src/default_editor/document_ime/supereditor_ime_interactor.dart
Outdated
Show resolved
Hide resolved
super_editor/lib/src/infrastructure/platforms/ios/ios_document_controls.dart
Show resolved
Hide resolved
super_editor/lib/src/infrastructure/platforms/ios/ios_document_controls.dart
Outdated
Show resolved
Hide resolved
super_editor/lib/src/infrastructure/platforms/ios/ios_document_controls.dart
Outdated
Show resolved
Hide resolved
super_editor/lib/src/super_reader/read_only_document_ios_touch_interactor.dart
Show resolved
Hide resolved
…also updated that method to check if 'mounted'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@angelosilvestre do you feel like you have a good idea about what this PR is trying to achieve and how it does it? I want to make sure that you really feel good about what's here before merging and moving forward with the other platforms. |
I'm gonna go ahead and merge this to move on to Android. @angelosilvestre please let me know if you have any architectural concerns about this approach. |
…composition (iOS)(Relates to superlistapp#424, superlistapp#893, superlistapp#1166)(Resolves superlistapp#1508) (superlistapp#1470)
This PR includes changes to a number of different areas, which are all somewhat related to working with document layers in
SuperEditor
andSuperReader
. We need to ensure that our layer system makes it possible to achieve obvious goals. This PR stress tests our layers by reworking our iOS caret, handles, magnifier, toolbar, and floating cursor display. This PR also includes some changes to the content layer system to achieve that goal.Proving Layer Capabilities by Adjusting iOS Editing Controls
Super Editor displays a number of controls on iOS: caret, drag handles, magnifier, toolbar, and a floating cursor. All of these controls were primarily managed by
IosDocumentTouchInteractor
. This interactor was constantly scheduling frames to inspect the document layout and figure out where to place the various controls. This coordination was complicated, convoluted, fragile, and non-performant.To prove the viability of our layer system, I aimed to do a few things:
SuperEditor
properties for that purpose.SuperEditor
properties to build it.When breaking apart the iOS controls to be more composable, it became clear that relying on
SuperEditor
properties for all such controls was getting out of hand. We'd need to also expose similar controls for Android, and possibly other platforms. Therefore, I shifted the approach for shared control. I created a widget calledSuperEditorIosControlsScope
, which provides access to aSuperEditorIosControlsController
, which holds the status of the magnifier, the toolbar, and the floating cursor.SuperEditor
internally adds aSuperEditorIosControlsScope
above the document layers in the widget tree, so that all document layers have access to a shared context. Also, an app can surround itsSuperEditor
with its ownSuperEditorIosControlsScope
to further share visibility into those controls. This allows apps, for example, to build their own toolbar widget.New Floating Cursor Architecture
The previous floating cursor behavior was spread through widgets in a confusing way. This was primarily due to two factors. First, the floating cursor changes originate within the IME system, which is otherwise dedicated to text editing. So it's a weird starting point (but not in our control). Second, our iOS touch interactor was calculating the visual size and offset for the floating cursor, as well as updating the document selection when it moved. But the floating cursor had nothing to do with Flutter gestures, so this was a weird widget to handle those responsibilities.
As of this PR, the responsibility breakdown is as follows:
Ideally, the auto-scrolling behavior in
IosDocumentTouchInteractor
should probably be handled byEditorFloatingCursor
. However, I think this change would require that we put the concept ofSuperEditor
scrolling in its own widget, so that all descendent widgets have an opportunity to auto-scroll, or to change the scroll offset. I filed a ticket for this: #1495Composable toolbar and magnifier
The iOS toolbar and magnifier are now configurable.
Example of overriding the default iOS toolbar and magnifier:
Editors vs Reader Controls Scope
Editors have some controls that readers don't have, e.g., caret and floating cursor. Therefore,
SuperEditor
andSuperReader
should use different types of controllers, and therefore different types of scopes for overlay controls.Super Editor has:
Super Reader has:
Additions and Changes to Help with Editor/Reader Document Layers
In theory, all apps could use existing content layer widgets, like
ContentLayerStatelessWidget
andContentLayerStatefulWidget
to implement document layers. However, all such layers would need to obtain a reference to the contentStatefulElement
, obtain itsState
, and then type cast it to aDocumentLayout
. This PR introducesDocumentLayoutLayerStatelessWidget
andDocumentLayoutLayerStatefulWidget
, which do this work on the app's behalf and make it easy to quickly position content based on aDocumentLayout
.I utilized the new
DocumentLayoutLayerStatefulWidget
to simplify the existingSelectionLeadersDocumentLayer
implementation.Content Layer Changes
During work on this ticket, I discovered that my
StatefulWidget
layers were constantly re-inflating their widget on every frame. This rendered theState
object useless. I alteredContentLayersElement
to useforgetChild()
on the layers instead ofdeactivateChild()
, which allowsContentLayersElement
to avoid building those widgets, while also retaining the associatedElement
andState
of each layer so they can be re-added to the tree later in the frame.A
ContentLayerState
can query its cached layout information, instead of being forced to only receive it indoBuild()
. This allows for event callbacks, such as user interactions, to operate on that same cached layout data.ContentLayerState
used to provide its associatedRenderObject
tocomputeLayoutData()
but now it provides both itsElement
andRenderObject
. This change is used in this PR, for example, to access theState
of aStatefulElement
and then interact with thatState
as aDocumentLayout
.Changes to text selection bounds calculations
Previously, calculating a selection rectangle at a collapsed selection (e.g., at a caret location) returned a zero-sized rectangle. In general, it's more useful to return a zero-width rectangle that's as tall as the nearest character. I changed the calculation to return the latter.The above statement is no longer true, because I copied that position rectangle behavior into a long-press selection PR that's already been merged. So the behavior exists, but this PR isn't the first place that it was added.
Minor Tasks in this PR:
LayerLink
tofollow_the_leader
LeaderLink
.