-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Added onSelectionChange callback to SelectionRegion and SelectionArea #108985
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
Conversation
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.
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.
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.
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.
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.
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
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.
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.
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.
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? 🤔
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.
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.
ah I see, just scroll back and forth without scroll the selection off screen. because once it is offscreen the method wouldnt be call
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.
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.
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.
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
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.
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
1bb7574 to
26b7d56
Compare
|
Hey @chunhtai, could you please review this, I've changed the implementation. |
26b7d56 to
5d15c46
Compare
778c614 to
9d16a5f
Compare
chunhtai
left a comment
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, just one nit
|
cc @justinmc for secondary review |
justinmc
left a comment
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 with nits, though I do think we should probably change the name of the parameter to onSelectionChanged with a "d".
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.
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.
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.
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.
9d16a5f to
2017a50
Compare
2017a50 to
640e208
Compare
|
@chunhtai could you please merge this? |

This PR adds onSelectionChange callback to SelectionRegion and SelectionArea so its possible to get the selected text.
Fixes: #108231
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.