CupertinoSlidingSegmentedControl is able to have proportional layout based on segment content
#153125
Conversation
github-actions
bot
added
framework
QuncCccccc
force-pushed
the
proportional_segment
branch
from
9c201ec
to
24270a6
Compare
QuncCccccc
changed the title
CupertinoSlidingSegmentedControl is able to have proportional segment based on segment contentCupertinoSlidingSegmentedControl is able to have proportional layout based on segment content
| /// up to meet the min width requirement. | ||
| /// | ||
| /// Defaults to false. | ||
| final bool isProportionalSegment; |
There was a problem hiding this comment.
Looking for some suggestions for this property name:)
There was a problem hiding this comment.
use a similar name as UISegmentedControl?
| @@ -395,6 +397,20 @@ class CupertinoSlidingSegmentedControl<T extends Object> extends StatefulWidget | |||
| /// will not be painted if null is specified. | |||
| final Color backgroundColor; | |||
|
|
|||
| /// Determine whether segments have equal widths or proportional widths based | |||
There was a problem hiding this comment.
Keep the first sentence concise.
There was a problem hiding this comment.
Updated! Also I updated the property name! evenWidth is a lot better than the original one but I just feel even width is already the default behavior. To provide more information in the name, I changed evenWidth to proportionalWidth:)Just wanted to give a little more information in name. But please let me know if anything doesn't make sense!
| /// up to meet the min width requirement. | ||
| /// | ||
| /// Defaults to false. | ||
| final bool isProportionalSegment; |
There was a problem hiding this comment.
use a similar name as UISegmentedControl?
| @@ -7,6 +7,7 @@ library; | |||
|
|
|||
| import 'dart:math' as math; | |||
|
|
|||
| import 'package:collection/collection.dart'; | |||
There was a problem hiding this comment.
I think we want to remove the dependency on collection?
There was a problem hiding this comment.
Ah I asked Michael about this, it seems not decided yet, and should be okay to use it if I only use .sum here which is easy to migrate, so I just kept it here. But I can also remove this library if you prefer not to include it:)!
| /// up to meet the min width requirement. | ||
| /// | ||
| /// Defaults to false. | ||
| final bool isProportionalSegment; |
| T segmentForXPosition(double dx) { | ||
| final RenderBox renderBox = context.findRenderObject()! as RenderBox; | ||
| final int numOfChildren = widget.children.length; | ||
| assert(renderBox.hasSize); | ||
| assert(numOfChildren >= 2); | ||
| if (widget.isProportionalSegment) { |
There was a problem hiding this comment.
This is basically hit testing. If you have access to the render object, maybe move the logic to the render object where the children's dimensions are easily accessible, so you don't need the global keys.
There was a problem hiding this comment.
Moved this part in _RenderSegmentedControl!
| return; | ||
| } | ||
| _isProportionalSegment = value; | ||
| markNeedsPaint(); |
There was a problem hiding this comment.
It's used in performLayout so this needs relayout (consider adding a test).
There was a problem hiding this comment.
Added a unit test:)
| final double allowedMinWidth = constraints.minWidth - totalSeparatorWidth; | ||
| if (totalWidth < allowedMinWidth) { | ||
| final double scale = allowedMinWidth / totalWidth; | ||
| childWidths = childWidths.map<double>((double width) => width * scale).toList(); |
There was a problem hiding this comment.
nit: consider mutating in place.
There was a problem hiding this comment.
also consider merging the scaling here with the scaling logic above.
There was a problem hiding this comment.
Sorry what do you mean by merging the logic of scaling?
| } | ||
|
|
||
| List<double> _getChildIntrinsicWidths(BoxConstraints constraints) { | ||
| List<double> childWidths = <double>[]; |
| } | ||
|
|
||
| Size _computeOverallSizeFromChildSize(Size childSize, BoxConstraints constraints) { | ||
| Size _computeOverallSizeFromChildSize(BoxConstraints constraints) { | ||
| final double maxChildHeight = _getMaxChildHeight(constraints, double.infinity); |
There was a problem hiding this comment.
I don't know if it makes sense to assume that you could use infinite here for maxHeight calculation. Could you add a code comment for the assumption?
There was a problem hiding this comment.
Or should I replace this with constraints.maxWidth?
| RenderBox? child = firstChild; | ||
| int index = 0; | ||
| double start = 0; | ||
| while (child != null) { | ||
| final BoxConstraints childConstraints = BoxConstraints.tight(Size(childWidths[index ~/ 2], childHeight)); |
There was a problem hiding this comment.
every child should have the same height I think?
There was a problem hiding this comment.
Moved seperator constraints out of the while loop!
There was a problem hiding this comment.
oh the separators. Nvm.
| @@ -422,10 +438,12 @@ class _SegmentedControlState<T extends Object> extends State<CupertinoSlidingSeg | |||
| final TapGestureRecognizer tap = TapGestureRecognizer(); | |||
| final HorizontalDragGestureRecognizer drag = HorizontalDragGestureRecognizer(); | |||
| final LongPressGestureRecognizer longPress = LongPressGestureRecognizer(); | |||
| GlobalKey segmentedControlRenderWidgetKey = GlobalKey(); | |||
| final BuildContext currentContext = segmentedControlRenderWidgetKey.currentContext!; | ||
| final _RenderSegmentedControl<T> renderBox = currentContext.findRenderObject()! as _RenderSegmentedControl<T>; | ||
|
|
||
| if (widget.proportionalWidth) { |
There was a problem hiding this comment.
I think you should be able to ask the render object regardless of whether proportionalWidth is set?
There was a problem hiding this comment.
It looks like the original implementation always returns an index in [0, leng - 1). getSegmentIndex should probably be renamed to getClosestSegmentIndex and always return a valid index?
There was a problem hiding this comment.
uber nit: the RTL reordering happens at the widget level so consider doing the RTL thing in the state.
There was a problem hiding this comment.
Updated based on the comments:)
| int? getSegmentIndex(double dx, TextDirection textDirection) { | ||
| double subtotalWidth = 0; | ||
| for (int i = 0; i < childWidths.length; i++) { | ||
| final double segmentWidth = childWidths[i]; |
There was a problem hiding this comment.
I think there are separators in between the children?
There was a problem hiding this comment.
Ah childWidths only contains the segment widths. This is a bad name. Renamed it to segmentWidths!
https://github.com/flutter/flutter/pull/153125/files#diff-aba8be2635c1c4a4ee9c15b8c0bdb7ba4cb22a4f795d4e98e5cebda3fc10666eR1025
| } | ||
|
|
||
| Size _computeOverallSizeFromChildSize(Size childSize, BoxConstraints constraints) { | ||
| List<double> _getChildIntrinsicWidths(BoxConstraints constraints) { |
There was a problem hiding this comment.
nit: consider combining the call paths for the !proportionalWidth case into this method, so you only have to handle proportionalWidth once.
| // each segment width should scale down until the overall size can fit in | ||
| // the parent constraints. | ||
| final double allowedMaxWidth = constraints.maxWidth - totalSeparatorWidth; | ||
| if (totalWidth > allowedMaxWidth) { |
There was a problem hiding this comment.
I think you can combine the 2 if cases to compute the scale only once
There was a problem hiding this comment.
final scale = doubleClamp(x, min, max) / x, and check if scale is 1.0?
There was a problem hiding this comment.
Done! It's cleaner. Thanks for the suggestion!
| return childWidths; | ||
| } | ||
|
|
||
| Size _computeOverallSizeFromChildSize(BoxConstraints constraints) { |
There was a problem hiding this comment.
This doesn't seem to take a child size any more?
QuncCccccc
force-pushed
the
proportional_segment
branch
from
f41b29e
to
fff033a
Compare
|
|
||
| double get totalSeparatorWidth => (_kSeparatorInset.horizontal + _kSeparatorWidth) * (childCount ~/ 2); | ||
| List<double> segmentWidths = <double>[]; | ||
| int getClosestSegmentIndex(double dx) { |
There was a problem hiding this comment.
nit: In the render object you have access to each child's paint offset and size. I think that would be a bit easier to do hit-testing with? It's finding the smallest dx.clamp(childOffset.dx, childOffset.dx.width) - dx
There was a problem hiding this comment.
Updated! Could you take another look? :)
There was a problem hiding this comment.
if dx is smaller than the smallest child offset, this method should return 0 instead of length - 1?
| @@ -924,30 +983,73 @@ class _RenderSegmentedControl<T extends Object> extends RenderBox | |||
| } | |||
|
|
|||
| Size _calculateChildSize(BoxConstraints constraints) { | |||
| final double childWidth = _getMaxChildWidth(constraints); | |||
There was a problem hiding this comment.
is the method still needed?
There was a problem hiding this comment.
Removed _calculateChildSize and updated computeDryBaseline method:)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…t based on segment content (flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
…onal layout based on segment content (flutter/flutter#153125)
This PR is to add
isProportionalSegmentproperty toCupertinoSlidingSegmentedControl. Originally, the width of each segment is determined by the widest segment and they have equal width. With this property setting to true, the width of each segment is determined by their own contents.If the max width from parent constraints is smaller than the width that the segmented control needed, each of the segment width will be scaled down; if the min width from parent constraints is larger than the width that the segmented control needed, each of the segment width will be scaled up.
related to: #43826
Pre-launch Checklist
///).