Add `SelectionListener`/`SelectedContentRange` #154202

Renzo-Olivares · 2024-08-27T20:14:40Z

Screen.Recording.2024-11-25.at.4.12.47.PM.mov

Adds:

SelectionListener, allows a user to listen to selection changes under the subtree it wraps given their is an ancestor SelectionArea or SelectableRegion. These selection changes can be listened to through the SelectionListenerNotifier that is provided to a SelectionListener.
SelectionListenerNotifier, used with SelectionListener, allows a user listen to selection changes for the subtree of the SelectionListener it was provided to. Provides access to individual selection values through the SelectionDetails object selection.
SelectableRegionSelectionStatusScope, allows the user to listen to when a parent SelectableRegion is changing or finalizing the selection.
SelectedContentRange, provides information about the selection range under a SelectionHandler or Selectable through the getSelection() method. This includes a start and end offset relative to the Selectables content.
SelectionHandler.contentLength, to describe the length of the content contained by a selectable.

Original PR & Discussion: #148998

Fixes: #110594

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
I signed the [CLA].
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is [test-exempt].
I followed the [breaking change policy] and added [Data Driven Fixes] where supported.
All existing and new tests are passing.

LongCatIsLooong · 2024-08-28T17:32:03Z

packages/flutter/lib/src/rendering/selection.dart

+      startOffset: -1,
+      endOffset: -1,
+      contentStart: -1,
+      contentLength: contentLength ?? -1,


Consider documenting the defaults and why -1. What's the difference between -1 and null?

This can be another constructor right?

+1 about documenting the -1s since that has tripped us up so much elsewhere.

+1 and also convert to const constructor

Converted to const constructor and added some docs about -1.

LongCatIsLooong · 2024-08-28T17:34:12Z

packages/flutter/lib/src/rendering/selection.dart

+  /// [WidgetSpan], the [startOffset] will be 0,
+  /// and [endOffset] will be 18.
+  /// {@endtemplate}
+  final int startOffset;


nit: maybe use TextRange so you don't have to document what -1 means for this field.

I didn't use TextRange because I think it implies this would only cover and apply to text, when a Selectable can be anything and not just text.

I was wondering the same. I see what you mean but I think I don't really have an ideological problem with that. At the end of the day this does map to a String, even if parts of that String might represent something else.

Also I looked through the docs for TextRange and I didn't see anything in there that seems out of place with this.

But either way works!

LongCatIsLooong · 2024-08-28T17:39:07Z

packages/flutter/lib/src/rendering/selection.dart

+  /// end of the '?' in the [WidgetSpan], we will receive
+  /// two [SelectedContentRange]s from [SelectionHandler.getSelections].
+  /// The first range will be relative to the root text
+  /// of the [TextSpan], the [startOffset] will be 6,


The range is relative content start, this will probably be easier to understand if the example also shows how contentStart fits in.

+1. I'm really glad you have this example though, it makes understanding this a lot easier. Maybe consider pointing to [startOffset] in a "See also" elsewhere in these docs so more people who are trying to understand this class see it?

Well I guess you have used it as a template/macro elsewhere 👍

contentStart is now removed, and the Text widget and other Selectables only produce one SelectedContentRange.

LongCatIsLooong · 2024-08-28T17:40:46Z

packages/flutter/lib/src/rendering/selection.dart

+    if (identical(this, other)) {
+      return true;
+    }
+    if (other.runtimeType != runtimeType) {


Should the class be final? That way you don't have to check the runtime type.

Ah nice use case for final class. Makes sense to me.

Make sure you didn't miss this ⬆️

I wonder if this can be left not final. That way someone could extend this class and add supplementary identifiers, if they wanted to go that route. For example, someone could wrap a Text widget with their own SelectionContainer with a delegate that overrides getSelection() and returns the range with an added identifier. SelectionDetails has been made final.

we could open it when people does ask, but I am fine either way.

LongCatIsLooong · 2024-08-28T17:55:25Z

packages/flutter/lib/src/rendering/selection.dart

@@ -287,6 +419,11 @@ enum SelectionEventType {
  /// Used by [SelectionEdgeUpdateEvent].
  endEdgeUpdate,

+  /// An event to indicate the selection is finalized.
+  ///
+  /// Used by [SelectionFinalizedSelectionEvent].


Document: what does it mean for a selection to be considered finalized

+1 to this as well. Point to the description of finalized in SelectionFinalizedSelectionEvent?

selectionFinalized below also kind of explains this.

I was following the conventions of the other enums here where they link to the event, that describes when the event happens in the framework. Is Used by [SelectionFinalizedSelectionEvent]. a sufficient link?

LongCatIsLooong · 2024-08-28T18:12:49Z

packages/flutter/lib/src/rendering/selection.dart

+  /// true if the selection is finalized.
+  final bool selectionFinalized;
+
+  /// The global start offset.


I don't think this is "global" within the app right? This should probably use a more accurate term if that's the case.

Also consider using existing classes TextRange.

I agree about "global". At the very least it should be clearly documented what you mean by it.

Changed to local*Offset, but open to other suggestions.

LongCatIsLooong · 2024-08-28T18:15:40Z

packages/flutter/lib/src/rendering/selection.dart

+  /// order as the [Selectable]s contained under
+  /// this [SelectionHandler].
+  ///
+  /// Return a list of [SelectedContentRange.empty]


nit: Returns

Also: why doesn't it return an empty list?

We need to be able to add up the contentLengths of Selectables even if they are not currently selected. For example if we have [A,B,C,D], and only [B,C,D] are selected, I still need the length of A to calculate the offsets.

LongCatIsLooong · 2024-08-28T18:20:29Z

packages/flutter/lib/src/rendering/paragraph.dart

@@ -1476,6 +1481,21 @@ class _SelectableFragment with Selectable, Diagnosticable, ChangeNotifier implem
    );
  }

+  @override
+  List<SelectedContentRange> getSelections() {
+    final int contentLength = range.textInside(fullText).length;


You don't really need the fullText?

Do you mean I don't need the fullText to calculate this? I think I can also do range.end - range.start.

LongCatIsLooong · 2024-08-28T18:25:40Z

packages/flutter/lib/src/widgets/selectable_region.dart

+  /// Copies the selections of all [Selectable]s.
+  @override
+  List<SelectedContentRange> getSelections() {
+    final List<SelectedContentRange> selections = <SelectedContentRange>[];


nit: consider using the list spread opreator ...?

LongCatIsLooong · 2024-08-28T18:26:27Z

packages/flutter/lib/src/widgets/selectable_region.dart

+        true,
+        'This selection container delegate has an active selection, indicated by its currentSelectionStartIndex and currentSelectionEndIndex, but it provides no SelectedContentRanges to represent this selection.',
+      );
+      return <SelectedContentRange>[];


Why constructing a new empty list?

justinmc

Overall I like this approach a lot. My comments are all small stuff.

If I can give one general thought it's that the relationship between SelectionContentRange, SelectionDetails, SelectedContent etc. took me awhile to understand. They all seem like they cover pretty similar but not-quite-the-same kind of stuff, and there is no clear relationship between them all. But I don't have any better ideas there, and I don't think that's a blocker at all. Just writing down some thoughts as I read through this.

I think I'd be happy to approve this if the small comments I left are fixed.

justinmc · 2024-08-29T19:55:42Z

examples/api/test/material/selection_area/selection_area.1_test.dart

+    expect(find.textContaining(' This is some more text in the same text widget.'), findsOneWidget);
+    expect(find.textContaining('This is some text in another text widget.'), findsOneWidget);
+  });
+}


I think it would be really valuable to add some tests here and for 2_test.dart that make some selections, change them red, and then expect that the text is red.

Note to self to write these.

Reminder for this comment 🎗️

One last reminder for this if you still want to add tests here before merge!

justinmc · 2024-08-29T20:01:17Z

packages/flutter/lib/src/rendering/selection.dart

+  /// The order of this list follows the same
+  /// order as the [Selectable]s contained under
+  /// this [SelectionHandler].


Maybe mention that the order is based on the startOffset (assuming that's true).

This no longer returns a list.

packages/flutter/lib/src/rendering/selection.dart

justinmc · 2024-08-29T20:04:35Z

packages/flutter/lib/src/rendering/selection.dart

+    required this.startOffset,
+    required this.endOffset,


Did you consider using a TextRange for this?

Edit: This is discussed in another comment further down.

Also, maybe its better to say "startIndex" etc. instead of "startOffset" to avoid confusion with Offset.

I am more toward using offset, it is what used in web and also desktop a11y bridge in engine

Ah ok, offset sounds good to me.

packages/flutter/lib/src/rendering/selection.dart

packages/flutter/lib/src/widgets/selectable_region.dart

justinmc · 2024-08-29T20:55:37Z

packages/flutter/lib/src/widgets/selectable_region.dart

+    return true;
+  }
+
+  ({int startOffset, int endOffset}) _calculateGlobalOffsets(List<SelectedContentRange> ranges) {


Another place to maybe renamed "offset" to "index".

Also could maybe be static?

This one can't be static, it uses a few instance variables.

justinmc · 2024-08-29T21:18:43Z

examples/api/lib/material/selection_area/selection_area.1.dart

+      ),
+    );
+  }
+}


I talked offline with @Renzo-Olivares but leaving this dartpad link here as an alternative way of going about these examples. Not sure if there's anything useful in it now. https://dartpad.dev/?id=97bd3f3cec6f90e3ca3791f085ac3b2c

justinmc · 2024-08-29T21:23:32Z

packages/flutter/lib/src/rendering/selection.dart

+  /// end of the '?' in the [WidgetSpan], we will receive
+  /// two [SelectedContentRange]s from [SelectionHandler.getSelections].


At first I was confused about how this appears to be "non-flat". But it seems a SelectionListener passes you a SelectionDetails that is totally flat. So I guess these docs here are referring to a lower level part and SelectionListener is the high level interface that most users will see.

Probably nothing to really clarify about that and it's just my problem with wrapping my head around this, but I wanted to point it out in case there's any way to make that more clear.

This Text widget now produces just one SelectedContentRange.

chunhtai · 2024-08-29T22:51:54Z

packages/flutter/lib/src/rendering/selection.dart

+  /// order as the [Selectable]s contained under
+  /// this [SelectionHandler].
+  ///
+  /// Returns a list of [SelectedContentRange.empty]


any reason that we prefer this instead of a const empty list?

This now just returns one SelectedContentRange instead of a list.

packages/flutter/lib/src/rendering/selection.dart

chunhtai · 2024-08-29T22:58:37Z

packages/flutter/lib/src/rendering/selection.dart

+    required this.startOffset,
+    required this.endOffset,


I am more toward using offset, it is what used in web and also desktop a11y bridge in engine

chunhtai · 2024-08-29T22:59:39Z

packages/flutter/lib/src/rendering/selection.dart

+      startOffset: -1,
+      endOffset: -1,
+      contentStart: -1,
+      contentLength: contentLength ?? -1,


+1 and also convert to const constructor

chunhtai · 2024-08-29T23:05:42Z

packages/flutter/lib/src/rendering/selection.dart

+  /// of the [TextSpan], the [startOffset] will be 6,
+  /// and [endOffset] will be 13. The second range
+  /// will be relative to the content inside the
+  /// [WidgetSpan], the [startOffset] will be 0,


Why not use global content length? and selection start also in global offset.

The current implementation will tie us into how text are broke into piece. If we changes it in the future the the select

if we use relative content range, they have to know the widget subtree and also how text will be break into multiple selectable. It will also change SelectedContentRange

I see the concern. I think this is cleared up now, with returning one SelectedContentRange instead of a list.

chunhtai · 2024-08-29T23:08:02Z

packages/flutter/lib/src/rendering/selection.dart

+  ///
+  /// Returns an empty list only if there is no
+  /// content under this [SelectionHandler].
+  List<SelectedContentRange> getSelections();


is there a cases where the selected content range is not continuous? can we just use one range to indicate the current range?

This issue I think with this approach is that we will be tied in with how we break text.rich into smaller selectable pieces, or when we add selectioncontainer in some widgets.

I think we can just return one range, I can't think of a case where the selection would be discontinuous. This is now changed.

I ran into an edge case where the selection is discontinuous in the middle of collapsing the selection. This is because we send two edge update events, one to move the start and the other to move the end, so somewhere in between these two edge move operations we may have a discontinuous selection. This leaves the SelectionDetails in a weird state during that time, with two offsets that are not correct.

We can solve for this by creating a CollapseSelectionEvent which sets both edges at the same time in one event operation instead of the current two event implementation to collapse the selection. What do you think about this? PR that adds event for reference: #155182

cc @chunhtai, for any further thoughts on this thread.

chunhtai · 2024-08-29T23:11:19Z

packages/flutter/lib/src/rendering/selection.dart

+///     for the selection under its subtree in its [SelectionListener.onSelectionChanged]
+///     callback.
+@immutable
+class SelectionDetails {


is the SelectionDetails meant to be the public facing API? if so we should try to make getSelectionList protected and SelectedContentRange internal

Yes, SelectionDetails is what the user will be consuming. By internal do you mean private?

packages/flutter/lib/src/rendering/selection.dart

chunhtai · 2024-08-29T23:24:51Z

packages/flutter/lib/src/widgets/selectable_region.dart

+///
+/// This widget does not listen to selection changes of nested [SelectionArea]s
+/// or [SelectableRegion]s in its subtree because those widgets create their own
+/// [SelectionRegistrar]s that do not register to any ancestor registrar.


I think the doc here is reaching implementation detail. just say because the SelectableRegion is self-contain and do not bubble up selection.

Although I think at some point we want to support nested SelectableRegion, the selection start outside the SelectableRegion can still select into the SelectableRegion, but the selection starts inside the SelectableRegion can not move outside of it. Have you thought about how selectionListener should behave in this scenario?

In that scenario, I think we can just treat the nested region as a SelectionContainer (it is already) and process its selection like any other child selectable. Does this sound reasonable?

yes that was my original plan if we were to start nest selectableregion

Renzo-Olivares · 2024-09-09T17:32:24Z

This is ready for another review, the main change is that a Selectable/SelectionHandler now only return a single SelectedContentRange instead of a list of ranges and other comments have been addressed.

justinmc

LGTM with nits 👍

I think the single SelectedContentRange is an improvement.

Don't forget to write the example tests mentioned in https://github.com/flutter/flutter/pull/154202/files#r1737083169.

justinmc · 2024-09-09T22:06:26Z

examples/api/lib/material/selection_area/selection_area.1.dart

+    const String endOfBulletTree = ' This is the end of the text widget.';
+
+    int currentOffset = 0;
+    dataSourceMap[(startOffset: 0, endOffset: bulletListTitle.length + bullets.join().length + textAfterBulletList.length + emphasizedText.length + endOfBulletTree.length)] = TextSpan(


Nit: Maybe put this on multiple lines or factor endOffset out as a variable?

justinmc · 2024-09-09T22:20:50Z

packages/flutter/lib/src/rendering/selection.dart

+  ///
+  /// When nothing is selected, subclasses should return a [SelectedContentRange.empty],
+  /// with a [SelectedContentRange.contentLength] that represents the length
+  /// of the content under it.


Nit: "under it" => "under it in the widget tree"

Or something more specific like that.

justinmc · 2024-09-09T22:21:35Z

packages/flutter/lib/src/rendering/selection.dart

+    required this.startOffset,
+    required this.endOffset,


Ah ok, offset sounds good to me.

justinmc · 2024-09-09T22:23:49Z

packages/flutter/lib/src/rendering/selection.dart

+    if (identical(this, other)) {
+      return true;
+    }
+    if (other.runtimeType != runtimeType) {


Make sure you didn't miss this ⬆️

justinmc · 2024-09-09T22:24:20Z

packages/flutter/lib/src/rendering/selection.dart

@@ -287,6 +419,11 @@ enum SelectionEventType {
  /// Used by [SelectionEdgeUpdateEvent].
  endEdgeUpdate,

+  /// An event to indicate the selection is finalized.
+  ///
+  /// Used by [SelectionFinalizedSelectionEvent].


+1 to this as well. Point to the description of finalized in SelectionFinalizedSelectionEvent?

justinmc · 2024-09-09T22:29:32Z

packages/flutter/lib/src/widgets/selectable_region.dart

@@ -2573,6 +2657,16 @@ abstract class MultiSelectableSelectionContainerDelegate extends SelectionContai
    return SelectionResult.none;
  }

+  /// Indicates the selection is finalized to all of the [Selectable]s this delegate manages.


Another place to consider explaining "finalized"

justinmc · 2024-09-09T22:30:40Z

packages/flutter/lib/src/widgets/selectable_region.dart

+/// This widget should have an ancestor [SelectionArea] or [SelectableRegion]
+/// to be able to listen to selection changes in this widgets subtree.


Would it be possible/desirable to log warning in that case?

packages/flutter/lib/src/widgets/selectable_region.dart

justinmc · 2024-09-09T22:32:34Z

packages/flutter/lib/src/widgets/text.dart

@@ -1228,6 +1226,70 @@ class _SelectableTextContainerDelegate extends MultiSelectableSelectionContainer
    return a.right > b.right ? 1 : -1;
  }

+  SelectedContentRange _calculateLocalRange(List<SelectedContentRange> ranges) {


Nit: Maybe add a comment here. (I also left a comment saying the same thing on the SelectableRegion version of this.)

packages/flutter/lib/src/rendering/selection.dart

LongCatIsLooong · 2024-09-11T17:13:06Z

packages/flutter/lib/src/rendering/selection.dart

+  /// A selected content range that represents an empty selection, i.e. nothing
+  /// is selected.
+  const SelectedContentRange.empty({int contentLength = 0})
+      : this(


intentation

What should change here?

The indentation should be 2 spaces I think

LongCatIsLooong · 2024-09-11T17:18:41Z

packages/flutter/lib/src/rendering/selection.dart

+          endOffset: -1,
+        );
+
+  /// The length of the content in the [Selectable] or [SelectionHandler] that


nit: The maximum selectable length of ...?

LongCatIsLooong · 2024-09-11T17:24:35Z

packages/flutter/lib/src/rendering/selection.dart

+  /// [TextSpan] tree, with [WidgetSpan] content being flattened. The [startOffset]
+  /// will be 6, and [endOffset] will be 38. This takes into account the
+  /// length of the content in the [WidgetSpan], which is 19, so the overall
+  /// [contentLength] will be 56.


That's not very intuitive IMO. I'd say it's not very obvious to most people that widget span is always length 1. If you know the full content length then how come the end offset is not 38 - 1 + 19?

Oh I think I forgot to update this, the endOffset in this case will be 56 not 38.

LongCatIsLooong · 2024-09-11T17:27:37Z

packages/flutter/lib/src/rendering/selection.dart

+  /// {@endtemplate}
+  final int startOffset;
+
+  /// The end of the selection relative to the end of the content.


relative to the start of the content?

LongCatIsLooong · 2024-09-11T18:01:54Z

packages/flutter/lib/src/widgets/selectable_region.dart

+      // Determining selection direction is innacurate if currentSelectionStartIndex == currentSelectionEndIndex.
+      // Use the range from the selectable within the selection as the source of truth for selection direction.
+      final SelectedContentRange rangeAtSelectableInSelection = selectables[currentSelectionStartIndex].getSelection();
+      forwardSelection = rangeAtSelectableInSelection.endOffset >= rangeAtSelectableInSelection.startOffset;


Are the selectables's selection directions guaranteed consistent?

If they are why do we need to normalize first?

If they are not why can we use that of the selectable at currentSelectionStartIndex?

They are not if we have LTR mixed with RTL. We only use the currentSelectionStartIndex when currentSelectionStartIndex == currentSelectionEndIndex, i.e. there's only one selectable in the selection so that can be thought of as the source of truth for the selection direction. When currentSelectionStartIndex and currentSelectionEndIndex are different (multiple selectables are under the selection), then we can use currentSelectionEndIndex >= currentSelectionStartIndex as the indicator for selection direction.

LongCatIsLooong · 2024-09-11T18:38:08Z

packages/flutter/lib/src/rendering/selection.dart

+///     for the selection under its subtree in its [SelectionListener.onSelectionChanged]
+///     callback.
+@immutable
+final class SelectionDetails with Diagnosticable {


Is this used in the rendering library or it's only used in the widgets library? If it's only used in widgets should this live in widgets for now?

This is only used by SelectionListener, so I think it'd be okay to live in the widgets library alongside it.

LongCatIsLooong · 2024-09-11T18:44:19Z

packages/flutter/lib/src/rendering/selection.dart

+    required this.localEndOffset,
+  });
+
+  /// The status of the selection under the [Selectable] or [SelectionHandler]


nit: the docs from the eum seems to be easier to understand to me, i.e.: whether there is a selection and whether the selection is collapsed

LongCatIsLooong · 2024-09-11T18:45:02Z

packages/flutter/lib/src/rendering/selection.dart

+  /// that created this object.
+  final SelectionStatus status;
+
+  /// Whether the selection is ongoing.


This doesn't seem to be consistent with the field name?

LongCatIsLooong · 2024-09-11T18:47:54Z

packages/flutter/lib/src/rendering/selection.dart

+/// local to the [SelectionListener] that reports this object, and whether
+/// the selection is ongoing.
+///
+/// This object is created by callers of the [SelectionListenerSelectionChangedCallback] callback.


Can the constructor be private if we move this to the widgets library?

Yeah, that works.

LongCatIsLooong · 2024-09-11T19:57:30Z

packages/flutter/lib/src/widgets/selectable_region.dart

+  SelectionResult dispatchSelectionEvent(SelectionEvent event) {
+    final SelectionGeometry lastSelectionGeometry = value;
+    final SelectionResult result = super.dispatchSelectionEvent(event);
+    if (value.status == SelectionStatus.none) {


Why can we short-circuit here? How do we get remove selection events?

Good point, i'll think about this case again.

Removed this so users can receive remove selection events.

chunhtai

overall the selection listener workflow looks good to me. just some question around API surface. The selection finalized event bothers me a little. It feels a bit unecessary if the sole purpose is to expose this state to SelectionDetails.

chunhtai · 2024-09-18T22:21:07Z

examples/api/lib/material/selection_area/selection_area.1.dart

@@ -0,0 +1,377 @@
+// Copyright 2014 The Flutter Authors. All rights reserved.


I don't think an example needs to be this complex, it will be overwhelming for people who may only want to take a quick peak on how to use the api.

How about we just have a maybe one or two level deep of textspan? or is there a specific things you want to show case?

same for selction_area.2.dart

I think for selection_area.1 I was trying to make sure it worked for complex cases, more for my own testing, but I also wanted to the example to show how one can deal with a WidgetSpan in the tree. By two levels deep, did you mean for the TextSpan tree that includes the bulleted list, we should remove everything after the bulleted list?

It seems like this example has been simplified while 2 contains the more complex stuff?

Yeah 1 is a simple example, while 2 is a complex one.

chunhtai · 2024-09-18T22:28:46Z

packages/flutter/lib/src/rendering/paragraph.dart

@@ -463,6 +463,9 @@ class RenderParagraph extends RenderBox with ContainerRenderObjectMixin<RenderBo

  /// Determines whether the given [Selectable] was created by this
  /// [RenderParagraph].
+  ///
+  /// The [RenderParagraph] splits its text into multiple [Selectable]s,
+  /// delimited by [PlaceholderSpan.placeholderCodeUnit].


Suggested change

/// delimited by [PlaceholderSpan.placeholderCodeUnit].

/// delimited by [PlaceholderSpan]s or [WidgetSpan]s.

chunhtai · 2024-09-18T22:40:08Z

packages/flutter/lib/src/rendering/selection.dart

+  const SelectedContentRange.empty({int contentLength = 0})
+    : this(


Suggested change

const SelectedContentRange.empty({int contentLength = 0})

: this(

const SelectedContentRange.empty({

int contentLength = 0

}) : this(

chunhtai · 2024-09-18T22:43:56Z

packages/flutter/lib/src/rendering/selection.dart

+  /// length of the content in the [WidgetSpan], which is 19, so the overall
+  /// [contentLength] will be 56.
+  ///
+  /// If [startOffset] and [endOffset] are both -1, the selected content range is


I will probably expose a isEmpty or isValid getter instead of ask consumer of this class to check for -1

chunhtai · 2024-09-18T22:45:42Z

packages/flutter/lib/src/rendering/selection.dart

+    if (identical(this, other)) {
+      return true;
+    }
+    if (other.runtimeType != runtimeType) {


we could open it when people does ask, but I am fine either way.

chunhtai · 2024-09-19T16:42:11Z

packages/flutter/lib/src/rendering/selection.dart

+  /// When nothing is selected, subclasses should return a [SelectedContentRange.empty],
+  /// with a [SelectedContentRange.contentLength] that represents the length
+  /// of the content under this [SelectionHandler].
+  SelectedContentRange getSelection();


Have you thought about extending the getSelectedContent API instead of creating a new one?

I thought about this, we would have to change the contract of getSelectedContent since it returns null when there is nothing selected, while the new API returns an empty representation of the selection. Also we would be exposing the start and end offsets through SelectionArea.onSelectionChanged. I think exposing those offsets through SelectionArea.onSelectionChanged might make them difficult to consume depending on the complexity of the Selectable tree, SelectionListener solves for this by allowing the user to get offsets for a specific subtree. I do think these APIs are similar/related but not exactly sure how to merge them unless we are okay with exposing the start and end offsets at the SelectionArea level. What do you think about this?

exposing start and end at selectionArea level seems fine to me. They don't have to use it. I would choose have less similar API if possible. The only thing that I would consider not to is that if merging these API will cause performance draw back. e.g. if we have to recalculate a lot of thing to just to have all data to be in one callback.

getSelectedContent is called somewhat often since it used by SelectableRegion.onSelectionChanged, at each selection gesture/keyboard shortcut step. Meaning we would have to tree walk and calculate the ranges each time. We currently guard against excessive onSelectionChanged calls by checking if the last plainText of SelectedContent is different from the current one, but to do that we have to call the getSelectedContent API itself. One thing we might be able to do here is instead of check plainText we can check the _lastSelectionGeometry, and then check plainText? _lastSelectionGeometry guards against excessive tree walks and plainText guards against excessive onSelectionChanged calls. Another simple guard we can do add is against calling the API at all when the callback onSelectionChanged is not provided to SelectableRegion. Despite these guards the calculation will still happen if the selection changes.

getSelection currently is called on demand, it needs to be explicitly requested by the user by calling SelectionListenerNotifier.range. I'm leaning towards this option since there are no performance implications at all here, but what do you think given the context?

chunhtai · 2024-09-19T16:43:41Z

packages/flutter/lib/src/rendering/selection.dart

+/// This event can be sent as the result of a mouse drag end, touch
+/// long press drag end, a single click to collapse the selection, a
+/// double click/tap to select a word, ctrl + A / cmd + A to select all,
+/// or a triple click/tap to select a paragraph.


maybe mention selection can still change through keyboard even if a selectable received this event

chunhtai · 2024-09-19T17:05:28Z

examples/api/lib/material/selection_area/selection_area.2.dart

+              ContextMenuController.removeAny();
+            }
+            if (selectionDetails.status != SelectionStatus.uncollapsed
+               || !selectionDetails.selectionFinalized) {


I believe this is the only reason selectionFinalized is introduce right? Have you thought about exposing the state through selectableResgion instead.

for example you can do selectionAreaKey.selectableRegion.isSelectionOnGoing or something like that.

I will try this out, am also not the biggest fan of the SelectionEvent.

I tried this but it did not work out as well. When we end a drag or a long press we do not dispatch any selection events, so with isSelectionOnGoing, when we receive the last event for a given drag, it is still set to true. Another thought is to make isSelectionOnGoing, a ValueNotifier<bool>, and a user can listen to it and so something like below:

selectionAreaKey.selectableRegion.isSelectionOnGoing.addListener(_handleSelectionOnGoing); void _handleSelectionOnGoing() { if (selectionAreaKey.selectableRegion.isSelectionOnGoing.value) { return; } // Show color me red context menu. This requires the user to save the last `SelectionDetails` they received in // SelectionListener.onSelectionChanged. }

This would also require a post frame callback to add the listener.

chunhtai · 2024-10-10T23:19:42Z

Talked to @Renzo-Olivares offline, the current idea is to use a listen/notify API something like TextEditingController to listen to selection changes, Ideally this can also get rid of the SelectionFinalizedEvent if the controller can listen to the SelectableRegion's state changes above the current widget, and it sounds good to me. Since I will be out for another 2 weeks, this PR will LGTM if it can be implemented without SelectionfinalizedEvent and also LGT @justinmc and @LongCatIsLooong .

vergaraSC · 2024-10-16T15:43:57Z

@Renzo-Olivares This will go into production (we have an estimated date)?, it is a very good feature.

justinmc

Thanks for adding the example tests.

…er#154202)

Renzo-Olivares changed the title ~~Add SelectionListener/SelectedContentRange/SelectionDetails API~~ Add SelectionListener/SelectedContentRange/SelectionDetails Aug 27, 2024

Renzo-Olivares marked this pull request as ready for review August 27, 2024 21:19

Renzo-Olivares mentioned this pull request Aug 27, 2024

Add SelectionListener and SelectedContentRange #148998

Closed

9 tasks

Renzo-Olivares requested review from justinmc, chunhtai and LongCatIsLooong August 27, 2024 23:10

LongCatIsLooong reviewed Aug 28, 2024

View reviewed changes

justinmc reviewed Aug 29, 2024

View reviewed changes

chunhtai reviewed Aug 29, 2024

View reviewed changes

Renzo-Olivares force-pushed the global-selectionlistener branch 3 times, most recently from 7a9e948 to 6da9401 Compare September 9, 2024 17:27

Renzo-Olivares requested review from justinmc, chunhtai and LongCatIsLooong September 9, 2024 17:32

justinmc approved these changes Sep 9, 2024

View reviewed changes

LongCatIsLooong reviewed Sep 11, 2024

View reviewed changes

chunhtai reviewed Sep 19, 2024

View reviewed changes

Renzo-Olivares force-pushed the global-selectionlistener branch from e80922e to 00b43cd Compare October 1, 2024 20:40

Liloupar mentioned this pull request Oct 14, 2024

摘录语句显示数目问题 octopusring/Light-Ink#5

Closed

Renzo-Olivares force-pushed the global-selectionlistener branch from 06c9bd6 to d226dcd Compare October 16, 2024 22:27

github-actions bot removed the f: scrolling Viewports, list views, slivers, etc. label Oct 16, 2024

Renzo Olivares added 4 commits November 25, 2024 13:43

Address reviewer comments

9577c80

suppress initial selectiongeometry notification

6f9d55b

update documentation

c618374

Address reviewer comments

030a219

Renzo-Olivares force-pushed the global-selectionlistener branch from d03e261 to 030a219 Compare November 25, 2024 21:43

Renzo Olivares added 3 commits November 25, 2024 14:06

Update selectableregionselectionstatus when selection handles are moved

7edd8e2

clean up

6866e1a

clean up

9daf534

justinmc approved these changes Nov 25, 2024

View reviewed changes

Renzo-Olivares added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 26, 2024

auto-submit bot added this pull request to the merge queue Nov 26, 2024

Merged via the queue into flutter:master with commit f3f72ed Nov 26, 2024
79 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Nov 26, 2024

engine-flutter-autoroll mentioned this pull request Nov 26, 2024

Roll Flutter from 793569f621f7 to 1e824af6bd21 (43 revisions) flutter/packages#8180

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 26, 2024

f3f72ed Add SelectionListener/SelectedContentRange (flutter/flutt…

5993303

…er#154202)

engine-flutter-autoroll mentioned this pull request Nov 26, 2024

Roll Flutter from 793569f621f7 to 1e824af6bd21 (43 revisions) flutter/packages#8182

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 26, 2024

f3f72ed Add SelectionListener/SelectedContentRange (flutter/flutt…

34d6533

…er#154202)

engine-flutter-autoroll mentioned this pull request Nov 26, 2024

Roll Flutter from 793569f621f7 to e9fe719b31cb (44 revisions) flutter/packages#8183

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 26, 2024

f3f72ed Add SelectionListener/SelectedContentRange (flutter/flutt…

9cdce3a

…er#154202)

engine-flutter-autoroll mentioned this pull request Nov 26, 2024

Roll Flutter from 793569f621f7 to fae695ed6198 (45 revisions) flutter/packages#8185

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 26, 2024

f3f72ed Add SelectionListener/SelectedContentRange (flutter/flutt…

b832380

…er#154202)

Piinks mentioned this pull request Dec 6, 2024

Flutter Framework Team: Notable Commits #121415

Closed

reidbaker mentioned this pull request Dec 13, 2024

Cutout rotation integration test #160255

Closed

11 tasks

espresso3389 mentioned this pull request Jan 15, 2025

_PdfTextRenderBox misses SelectionHandler.getSelection/contentLength introduced on Flutter master espresso3389/pdfrx#295

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 12, 2025

f3f72ed Add SelectionListener/SelectedContentRange (flutter/flutt…

9709921

…er#154202)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

f3f72ed Add SelectionListener/SelectedContentRange (flutter/flutt…

cd6f795

…er#154202)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 13, 2025

f3f72ed Add SelectionListener/SelectedContentRange (flutter/flutt…

1f81780

…er#154202)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 6, 2025

f3f72ed Add SelectionListener/SelectedContentRange (flutter/flutt…

58f1665

…er#154202)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Mar 7, 2025

f3f72ed Add SelectionListener/SelectedContentRange (flutter/flutt…

68dba18

…er#154202)

		/// end of the '?' in the [WidgetSpan], we will receive
		/// two [SelectedContentRange]s from [SelectionHandler.getSelections].

		/// This widget should have an ancestor [SelectionArea] or [SelectableRegion]
		/// to be able to listen to selection changes in this widgets subtree.

		@@ -0,0 +1,377 @@
		// Copyright 2014 The Flutter Authors. All rights reserved.

	/// delimited by [PlaceholderSpan.placeholderCodeUnit].
	/// delimited by [PlaceholderSpan]s or [WidgetSpan]s.

		const SelectedContentRange.empty({int contentLength = 0})
		: this(

Add SelectionListener/SelectedContentRange #154202

Add SelectionListener/SelectedContentRange #154202

Conversation

Renzo-Olivares commented Aug 27, 2024 • edited Loading

Pre-launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Renzo-Olivares Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Renzo-Olivares Sep 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Renzo-Olivares Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Renzo-Olivares Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Renzo-Olivares commented Sep 9, 2024

justinmc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `SelectionListener`/`SelectedContentRange` #154202

Add `SelectionListener`/`SelectedContentRange` #154202

Renzo-Olivares commented Aug 27, 2024 •

edited

Loading

Renzo-Olivares Sep 9, 2024 •

edited

Loading

Renzo-Olivares Sep 10, 2024 •

edited

Loading

Renzo-Olivares Sep 13, 2024 •

edited

Loading

Renzo-Olivares Sep 9, 2024 •

edited

Loading