feat(sr): Material and Cupertino Slider recorder added#1035
Conversation
fuzzybinary
left a comment
There was a problem hiding this comment.
A few minor suggestions, nothing too major.
Fix up dartfmt and I'll take a second look and approve.
| // With maskAllInputs every thumb should be anchored at the track midpoint | ||
| // regardless of the supplied value (0.0, 0.5, 1.0 should all look the same). | ||
| final fixture = MaterialApp( |
There was a problem hiding this comment.
I'm sure you verified this, but I just want to be sure this is what iOS / Android do for masked sliders. They don't do anything else special to indicate they've been masked?
There was a problem hiding this comment.
That’s actually my suggestion. In a native app, there’s no reason to mask a slider. Masking is a Session Replay feature, which means we’re intentionally not representing what is actually shown on the screen.
There was a problem hiding this comment.
It looks like iOS and Android show the track, but not the thumb. I'm okay having the thumb just be at the middle, but we should verify we're okay with that being different from the other platforms.
There was a problem hiding this comment.
What I noticed is that when the onChanged property is set to null, the thumb is still displayed, the only difference is the color used to represent the widget (which I already handled). Or do you mean that the other SDKs only display the track in that state?
There was a problem hiding this comment.
This is specifically when a user asks to mask inputs by specifying .maskAllInputs either at the root or with a masking override. It looks like in that case iOS and Android only display the track and not the thumb, but confirm with @gonzalezreal.
|
|
||
| return SpecificElement( | ||
| subtreeStrategy: CaptureNodeSubtreeStrategy | ||
| .ignore, // Ignore subtree to prevent CustomPaintRecorder from capturing the inner CustomPaint |
There was a problem hiding this comment.
nit: We don't need this comment every time. For most of these recorders we know we're using them to attempt to capture the full widget, which is why the subtree strategy is always .ignore
There was a problem hiding this comment.
Okay, sure. It’s just because I added that comment in the first recorder I implemented, and I’ve been using the same structure ever since, so the comment ended up being carried over.
| typedef _SliderGeometry = ({ | ||
| _SliderThumbGeometry thumb, | ||
| _SliderTrackSegmentGeometry inactiveTrack, | ||
| _SliderTrackSegmentGeometry activeTrack, | ||
| _SliderTrackSegmentGeometry? secondaryActiveTrack, | ||
| Rect? gap, | ||
| Rect? stopIndicator, | ||
| List<Rect> activeTickMarks, | ||
| List<Rect> inactiveTickMarks, | ||
| }); |
There was a problem hiding this comment.
This seems like a lot of stuff to put in a Record. Are we concerned about any downsides? I guess since it's mostly just being used as a return type that's probably okay, but it does seem like we're pushing it with this struct.
There was a problem hiding this comment.
The slider widget is the most complex widget I’ve worked with so far. It includes several shapes and variants within the same component, so I introduced that structure to better organize the code.
That said, it’s true that I could implement the recorder without defining that structure. However, I would still need to return all the objects defined there anyway. I think the current solution preserves consistency, makes the intent clearer, and helps keep the codebase more readable and maintainable.
There was a problem hiding this comment.
Yes, the question is whether to use a Record or a class with @immutable on it.
I looked it up, and Records saves some boilerplate and can have some advantages over a class in terms of memory management when they're small. But, they aren't gc allocated, so every time they're passed around you get an alloc / copy, so when they get big and are passed around frequently, they can cause performance problems.
This is likely fine because it's returned once then its individual properties are transferred, so the heap allocations should be kept to a minimum. But generally large Records like this should be avoided.
There was a problem hiding this comment.
Ohhh okay, I understand now. Do you think it would still be better to change it to an immutable class, or since the allocations should remain minimal in this case, are you okay with keeping it as it is now?
There was a problem hiding this comment.
I'm okay with keeping as is, since it's only used essentially as a return value. Just be aware of it in the future. If we're passing large Records around a lot it can be an easily avoidable performance drain.
| final widget = element.widget; | ||
| if (widget is! Slider) return null; |
There was a problem hiding this comment.
This is a faster rejection and should be put above the CupertinoSlider check.
| ); | ||
| } else { | ||
| thumbStyle = _SliderThumbStyle.round; | ||
| thumbSize = Size(20.0 * scale, 20.0 * scale); |
There was a problem hiding this comment.
Some of these naked numbers should be pulled out as constants.
| final double valueRatio = isMasked | ||
| ? 0.5 | ||
| : (range == 0 | ||
| ? 0.0 | ||
| : ((widget.value - widget.min) / range).clamp(0.0, 1.0).toDouble()); |
There was a problem hiding this comment.
I am still of the opinion this is easier to read.
final double valueRatio;
if (isMasked) {
valueRatio = 0.5
} else {
valueRatio = range == 0 ? 0.0 : (widget.value - widget.min / range).clamp(0.0, 1.0).toDouble();
}| final double clampedSec = | ||
| secValue.clamp(widget.min, widget.max).toDouble(); | ||
| final double secRatio = | ||
| range == 0 ? 0.0 : (clampedSec - widget.min) / range; | ||
| final double secX = trackLeft + trackWidth * secRatio; |
There was a problem hiding this comment.
There's no reason to specify these as doubles. Removing the type annotation and letting Dart treat them as num removes the unneeded toDouble after clamp, and everything internally should work just fine. You then only have to do the num to double conversion at the final set when it's used to create the Rect.
Additionally the extra clampedSec variable can be avoided with just:
final secValue = widget.secondaryTrackValue?.clamp(widget.min, widget.max);
| defaultColor = theme.colorScheme.onPrimary.withValues(alpha: 0.12); | ||
| } else if (year2023) { | ||
| defaultColor = theme.colorScheme.onSurface.withValues(alpha: 0.38); |
There was a problem hiding this comment.
If we're pulling these alpha values from somewhere, it might be good to say where.
| Color _findBackgroundColor(Element element, ThemeData theme) { | ||
| Color? result; | ||
| element.visitAncestorElements((ancestor) { |
There was a problem hiding this comment.
Instead of performing this backward tree walk and adding an extra render element, these containers should all render in SR just fine, and if we shorten the tracks, the gap should display as intended. Did we try that instead?
There was a problem hiding this comment.
Yes, I tried that. I initially thought it was the cleaner and better solution, but setting year2023 to false is a bit tricky.
It works well most of the time, but near the edges, both on the left and right sides, the representation is not accurate. When the gap cuts off before the track can reach its full height, the visual output is incorrect. In fact, in that case it ends up inverting the track orientation.
Also, the wireframes don’t allow me to assign a custom border to each box individually, so the inner border representation is inaccurate as well.
There was a problem hiding this comment.
🤔 I'm curious about how inaccurate it is. This backward tree walk could end up going up quite a bit for minimal gain. We don't need to be 100% accurate in our drawing here, and if the end result is a fairly small gain, it may not be worth the extra processing time. I'll defer to @gonzalezreal on this one.
This comment has been minimized.
This comment has been minimized.
| // With maskAllInputs every thumb should be anchored at the track midpoint | ||
| // regardless of the supplied value (0.0, 0.5, 1.0 should all look the same). | ||
| final fixture = MaterialApp( |
There was a problem hiding this comment.
This is specifically when a user asks to mask inputs by specifying .maskAllInputs either at the root or with a masking override. It looks like in that case iOS and Android only display the track and not the thumb, but confirm with @gonzalezreal.
| typedef _SliderGeometry = ({ | ||
| _SliderThumbGeometry thumb, | ||
| _SliderTrackSegmentGeometry inactiveTrack, | ||
| _SliderTrackSegmentGeometry activeTrack, | ||
| _SliderTrackSegmentGeometry? secondaryActiveTrack, | ||
| Rect? gap, | ||
| Rect? stopIndicator, | ||
| List<Rect> activeTickMarks, | ||
| List<Rect> inactiveTickMarks, | ||
| }); |
There was a problem hiding this comment.
I'm okay with keeping as is, since it's only used essentially as a return value. Just be aware of it in the future. If we're passing large Records around a lot it can be an easily avoidable performance drain.
| Color _findBackgroundColor(Element element, ThemeData theme) { | ||
| Color? result; | ||
| element.visitAncestorElements((ancestor) { |
There was a problem hiding this comment.
🤔 I'm curious about how inaccurate it is. This backward tree walk could end up going up quite a bit for minimal gain. We don't need to be 100% accurate in our drawing here, and if the end result is a fairly small gain, it may not be worth the extra processing time. I'll defer to @gonzalezreal on this one.
What and why?
Adds Session Replay support for Flutter's
SliderandCupertinoSliderwidgets so they render in replays instead of falling through to the generic CustomPaint recorder. Previously these widgets either weren't represented or appeared as blank/incorrect shapes, hiding user interaction with sliders from the replay UI.How?
Two new element recorders (SliderRecorder and CupertinoSliderRecorder) capture each slider as a small stack of SRShapeWireframes — inactive track, optional secondary, active, optional ticks/gap/stop indicator, and thumb on top — built from the widget's resolved geometry and colors. Colors mirror Flutter's own M2 / M3-2023 / M3-2024 defaults and honor a local SliderTheme override (via SliderTheme.of(element)), while the Cupertino side resolves through CupertinoDynamicColor. The M3-2024 gap finds its background color by walking the slider's ancestors (Material / ColoredBox / Scaffold / etc.), and masked sliders anchor the thumb at the track midpoint so the value isn't leaked. Both recorders share a ShapeWireframeBuilder.shape(...) helper to avoid duplicated wireframe construction.
Review checklist