-
Notifications
You must be signed in to change notification settings - Fork 29.1k
Fix Slider value indicator shape is broken if label is multiple-line text in Material 3 #173324
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
base: master
Are you sure you want to change the base?
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.
Code Review
An excellent pull request that addresses a visual bug with multiline labels on sliders and introduces a well-designed configuration API for it. The code is generally clean and the tests are quite thorough.
I've found a couple of issues that need attention: one critical bug that can lead to a crash, and another high-severity bug that causes incorrect painting.
Once these are addressed, this PR will be in great shape. Thank you for your contribution!
4d44364
to
290af04
Compare
290af04
to
f196f9b
Compare
3209ab5
to
3ed217e
Compare
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 a lot of hardcore canvas work, thanks for taking this on @huycozy. I appreciate all of the nitty gritty details in the tests too. I think the approach looks good, but some hight level considerations here:
I see a lot of TextPainters that are created, laid out, and then quickly disposed. For performance reasons, I wonder if there are any opportunities to cache and reuse any of these TextPainters? They can cheaply paint multiple times if their parameters haven't changed and a relayout isn't needed. If this significantly increases code complexity though, then it's probably not worth it until we have a reason to think performance is a bottleneck.
Also another general comment, it looks like there is a lot of drawing code that is almost the same repeated in a couple of different places for various types/parts of slider. I wonder if there's any opportunity to reuse some generic code? This is not the fault of this PR, it was like that beforehand, but this is just a heads up in case you see any opportunities where it makes sense to DRY things up.
Thank you for reviewing this, Justin! I’ll look into these suggestions next week and consider any optimizations. |
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Hi @justinmc, after looking at this fix again, I also think implementing cache for TextPainters may increase code complexity with minimal benefit and may result in higher maintenance costs. However, if you think this is necessary, please let me know. Instead, I did improve the fix a bit with a cleanup, what I did was to do code deduplication text processing(
TextPainter finalLabelPainter = labelPainter;
if (multilineConfig.enabled && multilineConfig.maxLines != null) {
finalLabelPainter = TextPainter(
text: labelPainter.text,
textDirection: labelPainter.textDirection,
textScaleFactor: textScaleFactor,
maxLines: multilineConfig.maxLines,
)..layout();
// If lines were truncated, add ellipsis manually.
if (finalLabelPainter.didExceedMaxLines) {
finalLabelPainter.dispose(); // Dispose the intermediate painter
finalLabelPainter = _createTextPainterWithEllipsis(
originalPainter: labelPainter,
originalText: labelText,
maxLines: multilineConfig.maxLines!,
textScaleFactor: textScaleFactor,
);
}
}
...
if (finalLabelPainter != labelPainter) {
finalLabelPainter.dispose();
}
_ValueIndicatorTextProcessor/// Utility class for processing multiline text in value indicators.
class _ValueIndicatorTextProcessor {
const _ValueIndicatorTextProcessor._();
/// Processes multiline text for value indicators, handling maxLines
/// truncation and ellipsis.
///
/// Returns the appropriate TextPainter based on the multiline configuration.
/// The caller is responsible for disposing the returned TextPainter
/// if it differs from the original.
static TextPainter processMultilineText({
required TextPainter originalPainter,
required ValueIndicatorMultilineConfig multilineConfig,
required double textScaleFactor,
}) {
final String labelText = originalPainter.text?.toPlainText() ?? '';
if (!multilineConfig.enabled || multilineConfig.maxLines == null) {
return originalPainter;
}
final TextPainter truncatedPainter = TextPainter(
text: originalPainter.text,
textDirection: originalPainter.textDirection,
textScaleFactor: textScaleFactor,
maxLines: multilineConfig.maxLines,
)..layout();
if (!truncatedPainter.didExceedMaxLines) {
return truncatedPainter;
}
truncatedPainter.dispose();
return _createTextPainterWithEllipsis(
originalPainter: originalPainter,
originalText: labelText,
maxLines: multilineConfig.maxLines!,
textScaleFactor: textScaleFactor,
);
}
/// Safely disposes a TextPainter if it was created by processMultilineText.
/// Does nothing if the painter is the same as the original.
static void disposeIfCreated(TextPainter painter, TextPainter original) {
if (painter != original) {
painter.dispose();
}
}
} final TextPainter finalLabelPainter = _ValueIndicatorTextProcessor.processMultilineText(
originalPainter: labelPainter,
multilineConfig: multilineConfig,
textScaleFactor: textScaleFactor,
);
...
_ValueIndicatorTextProcessor.disposeIfCreated(finalLabelPainter, labelPainter); Benefits we gained:
|
Signed-off-by: huycozy <huy@nevercode.io>
Signed-off-by: huycozy <huy@nevercode.io>
Signed-off-by: huycozy <huy@nevercode.io>
Signed-off-by: huycozy <huy@nevercode.io>
Signed-off-by: huycozy <huy@nevercode.io>
Signed-off-by: huycozy <huy@nevercode.io>
3633bb7
to
d859c61
Compare
Demo (after the fix):
Screenshots
video
multiline.mov
Sample code
Description
In this PR:
- Address the issue reported where the value indicator shape does not fully surround the label when multiline text is set. The fix is applied for both cases when
year2023
flag is true or false (corresponding toDropSliderValueIndicatorShape
orRoundedRectSliderValueIndicatorShape
).- Add some tests.
- Enable support for an optional configuration of multiline label for the slider value indicator: introduce
ValueIndicatorMultilineConfig
inSliderThemeData
with 3 properties:enabled
,maxLines
, andcornerPadding
.Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assist
bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.