Add textSelectionControls to TextField etc. - #66785

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

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

@chunhtai Any reason that we shouldn't add this to SelectableText? See #36985 (comment) for context.

@jamesblasco Could you write a quick test for this?

Great, I will add it tomorrow. What do you think about adding the same param to TextField, CupertinoTextField, TextFormField?

@justinmc I probably missed it when i wrote the selectable text

Ah I forgot that selectionControls is not already added to TextField et al, it's only on EditableText.

I'm going to hold off on merging this until we've planned out how to bring custom text selection menus to the framework. I worry that we may expose this selectionControls param and then decide on a different API and have to remove it.

I'm working on that design doc now and will send it to you @jamesblasco. Then we can decide whether to use selectionControls or not.

@jamesblasco Could you add a test for this? And could you also add this parameter to TextField, CupertinoTextField, and TextFormField and test those?

I think my plan with the text selection menu customization is to keep TextSelectionControls as-is, but clean up and publicly expose some of the classes that are currently private (similar to your project https://github.com/jamesblasco/selection_controls_example). Then users can pass in whatever they want to selectionControls and use the public classes to build their toolbar.

I may deprecate canCopy/handleCopy etc., but I think it's ok to move forward with this PR now anyway.

I'm working on refactoring and cleaning up some of those internal classes now.

I added the param and tests to all of them.

But now I just realized I think it would be better if the param is called selectionControls than textSelectionControls. Should I rename it?

@justinmc can I fix what you have pointed out and request for merge? 😅 it's three days ago... is there any progress on this? Anything I can help...? Thank you...

I think this is just about ready. One comment above about an extra newline @jamesblasco (not a big deal but please remove the extra newline if you have time). The Mac build tests are failing but I think it's an infrastructure problem, I'll try to get it to go green.

Sorry about that @justinmc, I missed the newline. Let's see if the Mac build test works now. Also, the Linux gradle_r8_test has been running for 10h and not finished yet.

@jamesblasco Thanks! Could you merge master into this PR? I think that's what we have to do to get that Linux gradle_r8_test to run again...

@justinmc hey, one quick question here, I have seen so many build fail because of the new line or wrong indentation, this code style kind of thing, while it can be annoying but can be great help when we want the codebase clean and readable, do you know what tool flutter is using here to enforce the code style? Thanks 😁

I've rebased the code to the latest master version. Let's see if this solves it.

@SeasonLeee The code styling for the Flutter repository is defined here https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#formatting. Most of the code style issues are detected by the dart analyzer and will make the build fail. A very few ones, like extra newlines, are not detected so it is important to make sure there is none before merging to master.

While dart format can auto-format your code directly, it is not used in the Flutter repository, so you have to do it manually.

I am not sure this what you were asking but hope it can help 🙂

@jamesblasco oh! definitely!!! You explain how these things works pretty well, thank you so much!

So, you just said that dart format didn't use in the Flutter repo, why is that?

No wonder there's a time I tried to check the code base and when I change something and, you know that Android Studio I set it to auto format, the whole file has been formatted! And there are not just no single thing I changed but a lot of changes because of the original file is not formatted!

It's quite strange, since I think they made the dart formatter thing...

Although we can still see that even after you tried something there but the build is still failing...

I just wonder when will can I use this textSelectionControl thing😅 and how on earth does this keep failing....

By the way, I opened an issue regarding SelectableText, could you have a look on it? Thanks!

#68675

I want to see the Flutter codebase on what cause that problem, but I lost in the pile of codes... god, how helpless am I 😭...

Yess, auto-format is a problem with Flutter repository. Related to why Flutter doesn't use 'dart format' you can read in the link I send you:

We do not yet use dartfmt. Flutter code tends to use patterns that the standard Dart formatter does not handle well. We are working with Dart team to make dartfmt aware of these patterns.

Related to the flutter tests, flutter-build is a special test that indicates if the master branch is open to merges. It is not related to any PR. If it fails, it means you should wait until is green to merge it. (Notice it will auto-update itself).

I will take a look at the issue you shared. Let's try to keep this PR focused on the issue, and feel free to ping me on Discord/Slack/Stackoverflow or email if you have more questions

@jamesblasco oh, man, you are right, now all the checks are green and it's passed!!!!! So now we are waiting for someone who have the permission to merge it into master and I flutter upgrade, then I will be able to use the selectionControl feature, right?! God, it's so exciting! And congratulation on your work!! And thank you for all the explanations!!

Thank you for your time @justinmc!

Thank you!

Thank you both for everything have done for this feature! I'm sure I will tag you when our product publish!!! Thank you!!! @jamesblasco @justinmc

This may be responsible for a regression: #68890