Add default selection style#100719
Conversation
flutter-dashboard
bot
added
a: text input
chunhtai
force-pushed
the
default-selection
branch
from
4686f4f to
66a4845
Compare
HansMuller
left a comment
HansMuller
left a comment
There was a problem hiding this comment.
This looks good, just some trivial feedback to prove that I read the code :-).
There was a problem hiding this comment.
I assume that we have a test that checks this default for the light/dark themes; likewise for selectionColor.
There was a problem hiding this comment.
The existing tests covered it
There was a problem hiding this comment.
What's the 0.2 opacity based on?
There was a problem hiding this comment.
This is the value that the CupertinoTextField uses. Since CupertinoTextField needs to be under CupertinoApp, so I put the value up to the CupertinoApp. This will however change the behavior for TextField that is under CupertinoApp, I am not sure whether that will be a problem
There was a problem hiding this comment.
This function could be a widget with a const constructor
There was a problem hiding this comment.
how would that help with making TextSelectionTheme const? the use the const widget in the initializer, the input also needs to be const value.
so I can't do
super(key: key, child: const MyWidget(data: data, child: child));because the data and child is runtime value.
There was a problem hiding this comment.
Although i can make it const by doing some hack, but that will need to rely on the implementation detail of ProxyWidget
There was a problem hiding this comment.
Even if it's not possible to use a const constructor for a _WithDefaultSelectionStyle() widget, it seems clearer and more in-line with Flutter conventions than using a function that has the same effect.
chunhtai
force-pushed
the
default-selection
branch
from
564abee to
641a282
Compare
| /// The color of the cursor. | ||
| /// | ||
| /// Defaults to the theme's `cursorColor` when null. | ||
| /// The cursor indicates the current location of the text insertion point in |
There was a problem hiding this comment.
The cursor indicates the current location of the text insertion point in the field. =>
The cursor indicates the current text insertion point.
| /// The cursor indicates the current location of the text insertion point in | ||
| /// the field. | ||
| /// | ||
| /// If this is null it will default to the ambient |
There was a problem hiding this comment.
If null then [DefaultSelectionStyle.cursorColor] is used. If that is also null and [ThemeData.platform] is [TargetPlatform.iOS] or [TargetPlatform.macOS], then [CupertinoThemeData.primaryColor] is used. Otherwise [ColorScheme.primary] of [ThemeData.colorScheme] is used.
| final TextSelectionThemeData data; | ||
|
|
||
| // Overriding the getter to insert `DefaultSelectionStyle` into the subtree | ||
| // without a breaking change. This should be avoid in general since this |
There was a problem hiding this comment.
breaking => breaking API
In general, this approach should be avoided because it relies on an implementation detail of ProxyWidget. This workaround is necessary because TextSelectionTheme is const.
| // without a breaking change. This should be avoid in general since this | ||
| // relies on the implementation detail of ProxyWidget. | ||
| // | ||
| // The `TextSelectionTheme` should be non-const if it were to start |
| }) : assert(data != null), super(key: key, child: child); | ||
| }) : assert(data != null), | ||
| _child = child, | ||
| super(key: key, child: const _NullWidget()); |
There was a problem hiding this comment.
Maybe include a warning comment here:
// See `get child` override below
|
Should the documentation (https://api.flutter.dev/flutter/material/TextSelectionThemeData/selectionHandleColor.html) be edited to reflect the suggested approach above? |
|
@Sylith1231 The TextSelectionTheme still works, it will create a defaultSelectionStyle as part of its widget build. |
This reverts commit c8057bc.
This reverts commit 95e52da.
4 participants
based on #99576
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.