Skip to content
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

Densify DevTools UI #7030

Merged
merged 10 commits into from Jan 9, 2024
Merged

Densify DevTools UI #7030

merged 10 commits into from Jan 9, 2024

Conversation

kenzieschmoll
Copy link
Member

@kenzieschmoll kenzieschmoll commented Jan 8, 2024

Many styling issues were resolved by changing constants in theme.dart and applying styling to the default textTheme.

In this PR we set the default font size for textTheme.bodyMedium, which is the default font style applied to all Text widgets.

Fixes #6761

Some before (left) and after (right) examples:
Screenshot 2024-01-08 at 3 09 26 PM
Screenshot 2024-01-08 at 3 06 05 PM
Screenshot 2024-01-08 at 3 08 27 PM
Screenshot 2024-01-08 at 3 02 41 PM

@kenzieschmoll
Copy link
Member Author

This PR does have release notes, but it appears the release notes check may not be working.

@@ -318,7 +318,7 @@ class _NotificationMessage extends StatelessWidget {
@override
Widget build(BuildContext context) {
final theme = Theme.of(context);
final textStyle = theme.textTheme.bodyMedium;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you document in our style guide not to use theme.textTheme.bodyMedium

@jacob314
Copy link
Contributor

jacob314 commented Jan 9, 2024

Looks like a big improvement!
Only nit is the close button on the warning message looks a bit close to the RHS of the window.
You might want to also verify that the tree table horizontal scrolling is still robust when using a variable width font. Perhaps we used a fixed width font to make it easier to exactly compute the maximum width of all rows visible in the table. If that was the case, you should see excessive horizontal width of the table when expanding out a large tree table.

Copy link
Contributor

@polina-c polina-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -738,9 +737,9 @@ class VmServiceObjectLink extends StatelessWidget {

final TextStyle style;
if (isServiceObject) {
style = theme.fixedFontLinkStyle;
style = theme.linkTextStyle;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these still using monospace fonts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Chrome DevTools does not use a monospace font for links either:
Screenshot 2024-01-04 at 11 21 42 AM

@kenzieschmoll
Copy link
Member Author

kenzieschmoll commented Jan 9, 2024

You might want to also verify that the tree table horizontal scrolling is still robust when using a variable width font. Perhaps we used a fixed width font to make it easier to exactly compute the maximum width of all rows visible in the table. If that was the case, you should see excessive horizontal width of the table when expanding out a large tree table.

@jacob314

You are right. We assumed a monospace font for calculating the width of the variable width columns. Other options:

  1. I can use a TextPainter instead to measure the size of the text with a variable width font, but that may hurt performance when we have to do that for every row when the table is resized or nodes are expanded.
  2. Continue to use a fixed value as the assumed width of each character. Error on the high side so that we do not overflow, and ellipsize the text in case we do. This will mean there will be some empty space to the right if the user scrolls all the way over.
  3. Use a fixed width for the column and ellipsize any content that does not fit in the fixed width.
  4. Continue using a monospace font for tables.

@Piinks does the new Flutter table you built handle variable width columns well? TL;DR is we used a monospace font so that we could accurately guess the size of a text display (column.getDisplayValue(node).length * characterWidth), and now that we are switching to a variable width font, calculating the width that the column needs to be to fix the content is trickier.

@Piinks
Copy link
Contributor

Piinks commented Jan 9, 2024

does the new Flutter table you built handle variable width columns well?

If you mean does the column automatically size itself based on the content in a given cell? No. That would mean we would need to lay out every cell for a given column to determine how wide it should be, which would make the table not lazy and have poor performance.

However, there are many ways to define the size of a column or row, all of the subclasses of TableSpanExtent are listed here: https://pub.dev/documentation/two_dimensional_scrollables/latest/table_view/TableSpanExtent-class.html

@jacob314
Copy link
Contributor

jacob314 commented Jan 9, 2024

I think option 3 makes sense if we add back in tooltips or another affordance so there is a way to view the full content without truncation.
Clarified offline: option 3 wouldn't be a truly fixed width but instead fixed width + max indent of any row currently visible.

@kenzieschmoll
Copy link
Member Author

This discussed table changes are implemented as a separate PR: #7033

@kenzieschmoll kenzieschmoll merged commit f4264a3 into flutter:master Jan 9, 2024
23 checks passed
@kenzieschmoll kenzieschmoll deleted the dense branch January 9, 2024 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the DevTools UI more dense
6 participants