Add support for separators in text copied from `SelectableRegion` #126835

Rexios80 · 2023-05-15T15:28:49Z

Replaces #121112 since that one turned into a mess of changes we don't want

Right now, copying text in a SelectionArea copies it with no separation. Since SelectionArea is very useful with vertical lists of Text widgets this is not ideal. This PR adds a separator field to SelectionArea so that developers can add separators such as newlines between selected Text widgets.

#104548

I added a new test for this

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.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

justinmc

Thanks for reopening this. The approach looks good, but I'll defer to @chunhtai.

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

packages/flutter/test/widgets/selectable_region_test.dart

justinmc · 2023-05-22T18:11:26Z

packages/flutter/lib/src/material/selection_area.dart

  /// Called when the selected content changes.
  final ValueChanged<SelectedContent?>? onSelectionChanged;

+  /// The separator to use when concatenating the selected text.


I would explain the default value of empty string here, too, and then say that you might want to use '\n' instead.

Actually, maybe we should default to newline?

@justinmc I don't think we should default to newline as that could introduce a breaking change to apps already using it and expecting no separator.

Are there any existing apps that are expecting no separator, or would they consider that a bug? I would guess that most expect as newline separator, but it's true I'm just guessing. If you @dsyrstad have an app that expects no separator then that's pretty strong evidence we should keep that default.

@justinmc I don't have an app which expects no separator, but I was working on an e2e test that was going to check what was copied from the clipboard (which is how I ran into this issue). If there are existing e2e tests out there expecting it as it is, I'd guess they'd break. Also, it may be that users of apps historically expect it in a certain format, so you might break the users. I'm of the opinion to do the least harm possible. If an app wants a separator, it can configure it.

True, thanks for sharing your context here! I'm happy either way.

I updated the doc comment. Let me know if you have any thoughts.

…-2' into feature/selection-area-separator-2

justinmc

Thanks for the fixes! LGTM.

CC @chunhtai for review.

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

justinmc · 2023-05-26T18:16:42Z

@Rexios80 Can you push a merge commit to fix the Google tests?

…rea-separator-2

Rexios80 · 2023-06-05T14:59:44Z

@justinmc Is there anything else you need from me on this?

justinmc · 2023-06-05T16:33:20Z

No, looks like the tests are green. I'll find a secondary reviewer.

packages/flutter/test/widgets/selectable_region_test.dart

chunhtai · 2023-06-05T18:18:45Z

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

  @override
  Widget build(BuildContext context) {
    assert(debugCheckHasOverlay(context));
+    final SelectionRegistrar? registrar = SelectionContainer.maybeOf(context);


I don't think we should be stacking selectable regions. Is this for setting different separate for the subtree? if so, can you create a new named SelectionContainer constructor that takes in a separator and create a _SelectableRegionContainerDelegate with that separate in its state?

for example something like SelectionContainer.options(separator: '\n');

How does making a different constructor change anything?

Because SelectableRegion doesn't mean to nest, and the SelectionContainer does.

I'm not sure what to do here. If the SelectableRegions don't share a registrar how will they get separated when copied?

packages/flutter/test/widgets/selectable_region_test.dart

…rea-separator-2

justinmc · 2023-07-21T20:43:10Z

@Rexios80 What do you think of @YeungKC's idea to add an onCopy function instead of separators?

Rexios80 · 2023-07-22T01:24:44Z

I like that idea, but someone else will probably have to implement it if we want it done any time soon. I got busy with other work.

flutter-dashboard · 2023-08-11T01:25:22Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Hixie · 2023-10-31T22:57:49Z

@Rexios80 thanks so much for your contributions here, I've added a link to this PR to the issue so hopefully your work will not be in vain! I'm going to close this for now since you are no longer working on it.

Rexios80 added 8 commits February 20, 2023 21:13

Add separator field to SelectionArea

fff1cae

Made super parameter not required

8dcf1fd

Remove trailing spaces in doc comment

11ac902

Use existing selection registrar from context if it exists

c182d7e

Added test for composing SelectableRegions

faa11cd

Updated documentation

b5242bb

Removed extra space

94f4472

Fix selection starting from nested SelectableRegion

bece955

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 15, 2023

Rexios80 mentioned this pull request May 15, 2023

Add support for separators in text copied from SelectableRegion #121112

Closed

8 tasks

Merge branch 'master' into feature/selection-area-separator-2

10ebca8

HansMuller added a: text input Entering text in a text field or keyboard related problems f: selection SelectableRegion, SelectionArea, SelectionContainer, Selectable, and related APIs labels May 19, 2023

HansMuller requested review from chunhtai and justinmc May 19, 2023 17:37

justinmc reviewed May 22, 2023

View reviewed changes

Rexios80 added 3 commits May 24, 2023 13:40

Merge branch 'main' into feature/selection-area-separator-2

5e38b45

Fixes

6c9a43b

Merge remote-tracking branch 'origin/feature/selection-area-separator…

e0c5f4a

…-2' into feature/selection-area-separator-2

github-actions bot removed a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. labels May 24, 2023

justinmc approved these changes May 25, 2023

View reviewed changes

packages/flutter/lib/src/widgets/selectable_region.dart Outdated Show resolved Hide resolved

packages/flutter/lib/src/widgets/selectable_region.dart Outdated Show resolved Hide resolved

Rexios80 added 3 commits May 28, 2023 13:40

Merge remote-tracking branch 'upstream/main' into feature/selection-a…

81b990a

…rea-separator-2

Fixing issues

853de58

Merge branch 'master' into feature/selection-area-separator-2

34bd7d6

chunhtai reviewed Jun 5, 2023

View reviewed changes

Rexios80 added 2 commits July 7, 2023 14:21

Merge remote-tracking branch 'upstream/main' into feature/selection-a…

78d2329

…rea-separator-2

Fixes

caf1cbf

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Jul 7, 2023

Hixie mentioned this pull request Oct 31, 2023

Copy text in SelectionArea is not like copying text from HTML #104548

Open

Hixie closed this Oct 31, 2023

Rexios80 mentioned this pull request Apr 11, 2024

Add CopyInterceptor support to SelectionArea #146625

Closed

8 tasks

TheMaverickProgrammer mentioned this pull request Sep 20, 2024

SelectionContainer.getSelectionContent() add newline char iff none exists in Selectable's result. #155481

Closed

9 tasks

Add support for separators in text copied from SelectableRegion #126835

Add support for separators in text copied from SelectableRegion #126835

Uh oh!

Conversation

Rexios80 commented May 15, 2023

Pre-launch Checklist

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

justinmc commented May 26, 2023

Uh oh!

Rexios80 commented Jun 5, 2023

Uh oh!

justinmc commented Jun 5, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justinmc commented Jul 21, 2023

Uh oh!

Rexios80 commented Jul 22, 2023

Uh oh!

flutter-dashboard bot commented Aug 11, 2023

Uh oh!

Hixie commented Oct 31, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Add support for separators in text copied from `SelectableRegion` #126835

Add support for separators in text copied from `SelectableRegion` #126835