Skip to content

Conversation

@chinmoy12c
Copy link
Member

This PR adds onSelectionChange callback to SelectionRegion and SelectionArea so its possible to get the selected text.

Fixes: #108231

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.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Aug 4, 2022
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is called when the selection geometry changes, this include scrolling a scrollable with an existing selection. The selected content can be the same in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey, thanks for the review :). Could you please elaborate on this case with a small example code? I tried putting a text in scrollable but could not replicate this behavior that is, the method was not called while scrolling.

Copy link
Contributor

Choose a reason for hiding this comment

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

try this

import 'package:flutter/material.dart';

void main() => runApp(const MyApp());

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  static const String _title = 'Flutter Code Sample';

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: _title,
      home: SelectionArea(
        child: Scaffold(
          appBar: AppBar(title: const Text(_title)),
          body: Center(
            child: ListView.builder(
              itemBuilder: (BuildContext context, int index) {
                return Text('text $index');
              },
              itemCount: 1000,
            ),
          ),
        ),
      ),
    );
  }
}

select something and scroll

Copy link
Contributor

Choose a reason for hiding this comment

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

There is also some concern about calling getSelectedContent in this method since it can be called every frame when scrolling and the getSelectedContent does a tree walk.

Copy link
Member Author

Choose a reason for hiding this comment

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

Screencast.from.08-05-2022.01.53.01.AM.webm

I tried your code on chrome but got the same results. I also tried running it on my android phone and got the same result there. Am I missing something? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Screenshot from 2022-08-05 02-13-16

I've just changed this and I get the "update" output only when I select text or change the selected text 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I see, just scroll back and forth without scroll the selection off screen. because once it is offscreen the method wouldnt be call

Copy link
Member Author

Choose a reason for hiding this comment

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

Same result :( Tried scrolling back and forth a few times, keeping it on the edge and scrolling with different speeds as well. Couldn't replicate it neither on chrome nor on my phone.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is getting more and more mysterious, I can reproduce it. Do you have the latest master branch? when you test it on mobile, does the toolbar move around when you scrolling? because it relies on this method to update its position

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'm running on the latest master just pulled it a while ago. Here is how it looks on my phone, it just gives the update message only when a selection changes.

WhatsApp.Video.2022-08-05.at.2.44.42.AM.mp4

@chinmoy12c
Copy link
Member Author

Hey @chunhtai, could you please review this, I've changed the implementation.

@chinmoy12c chinmoy12c force-pushed the issue_108231 branch 2 times, most recently from 778c614 to 9d16a5f Compare August 26, 2022 09:29
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit

@chunhtai chunhtai requested a review from justinmc August 26, 2022 18:08
@chunhtai
Copy link
Contributor

cc @justinmc for secondary review

Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM with nits, though I do think we should probably change the name of the parameter to onSelectionChanged with a "d".

Copy link
Contributor

Choose a reason for hiding this comment

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

Will this be called if the user just taps various locations in the text? I wonder if it's changing an invisible collapsed selection in those cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

This will only be called when an existing selection changes in value. So, if the user just taps on various locations, it would just call this only once when the selection becomes empty, and then it would remain silent until an actual selection is made.

@chinmoy12c
Copy link
Member Author

@chunhtai could you please merge this?

@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 29, 2022
@auto-submit auto-submit bot merged commit c6870ee into flutter:master Aug 29, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 30, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[SelectionArea] add the ability to get the currently selected text

3 participants