More efficient build dashboard by Hixie · Pull Request #741 · flutter/cocoon

Hixie · 2020-04-09T04:44:42Z

This is an incremental improvement but there's still plenty of room for making this even faster. I left TODOs where I saw places we could continue the work. I didn't do them yet because this is already a huge patch.

Hixie · 2020-04-09T04:44:47Z

cc @CaseyHillers @yjbanov

CaseyHillers

This is a great improvement! Much faster and feels good to run the app in dev mode :)

Just a note, the lattice render object code is out of scope for my review. Maybe @yjbanov can review it?

Some observations:

Seems like web image is showing the canvaskit version everytime (using fake data). I didn't try this in release mode.
The circle spinning icon for running tasks is nice, however it can be a headache when there are multiple running on the screen (team feedback). Do you have any ideas to still show some sort of motion on running tasks? Such as a dot going from side to side.
Dart docs and documenting some of the more complex logic in lattice.
Looks like StatusGrid needs a golden image update

CaseyHillers · 2020-04-09T16:19:03Z

app_flutter/lib/logic/task_helper.dart

Seems like these helper functions should be moved into QualifiedTask

CaseyHillers · 2020-04-09T16:20:00Z

app_flutter/lib/service/dev_cocoon.dart

I can already see the 2D gradient in my head :)

CaseyHillers · 2020-04-09T16:20:38Z

app_flutter/lib/service/dev_cocoon.dart

Is 0 transparent (no task for that cell)?

the numbers here are the probability that it'll pick this item, so 0 means no chance

added comments to this method

CaseyHillers · 2020-04-09T16:22:07Z

app_flutter/lib/state/build.dart

I would make a note saying that this is safe to call multiple times.

CaseyHillers · 2020-04-09T16:23:01Z

app_flutter/lib/state/build.dart

nit: Seems weird to have _ and Internal in the same function 🤷‍♂️

yeah, i never know how to name private internal versions of public methods like this :-(

CaseyHillers · 2020-04-09T17:17:37Z

app_flutter/lib/widgets/task_overlay.dart

nit: SHOW might be more appropriate

CaseyHillers · 2020-04-09T17:23:24Z

app_flutter/lib/service/dev_cocoon.dart

Observation: The fake data is showing attempts at 0 for a lot of the task statuses.

Updated this logic to have more plausible attempt counts.

CaseyHillers · 2020-04-09T17:24:48Z

app_flutter/lib/widgets/lattice.dart

I think this file should be split into smaller files, and just import those into this one. Such as lattice_scrollview.dart and lattice_body.dart

I considered that but really there's only two files here, LatticeScrollView (about 120 lines) and everything else (about 750 lines), most of which is currently marked private and would have to be made public if we split it. How strongly do you feel about this?

Reviewing it again, I agree with you. I wish it was smaller from a maintenance perspective, but this is fine. I just hope it can be moved into the framework :)

CaseyHillers · 2020-04-09T17:28:42Z

app_flutter/lib/widgets/status_grid.dart

Can you add the steps for converting List to the 2D matrix? It was in TaskMatrix and helps outline what this is doing.

Added docs.

CaseyHillers · 2020-04-09T17:39:27Z

app_flutter/lib/widgets/status_grid.dart

With TaskMatrix gone, it might make more sense to call this TaskGrid. That would make the files a bit more cohesive for the build dashboard widgets

Hixie · 2020-04-09T20:41:09Z

Just a note, the lattice render object code is out of scope for my review. Maybe @yjbanov can review it?

@goderbauer may also be interested in it.

Seems like web image is showing the canvaskit version everytime (using fake data). I didn't try this in release mode.

I made it use the avatar instead of the grey box when it fails to load the image, is that what you're seeing? I never see the images for some reason.

The circle spinning icon for running tasks is nice, however it can be a headache when there are multiple running on the screen (team feedback). Do you have any ideas to still show some sort of motion on running tasks? Such as a dot going from side to side.

Ah, interesting, I didn't realize we had turned it off due to feedback, I thought it was because of performance. In the old dashboard we had a spinning square, would that work? Or maybe a pulsating circle or something?

Dart docs and documenting some of the more complex logic in lattice.

Please leave comments on any lines you'd like docs for, I'm happy to add docs places but I have no idea what isn't obvious and what is. :-)

Looks like StatusGrid needs a golden image update

Roger.

Hixie · 2020-04-09T22:50:56Z

Ah, interesting, I didn't realize we had turned it off due to feedback, I thought it was because of performance. In the old dashboard we had a spinning square, would that work? Or maybe a pulsating circle or something?

I went with a slowly pulsating circle, seems pretty good.

Hixie · 2020-04-09T22:51:31Z

Not sure what's up with the golden image on Luci not matching, it works on my machine, and I'm pretty sure I've removed all the sources of randomness... I'll keep poking.

goderbauer

The lattice stuff LGTM

goderbauer · 2020-04-09T21:05:19Z

app_flutter/lib/widgets/commit_box.dart

nit: this class stands out in this file as the only public one without docs. (not sure if we require docs in this repository)

This class should be moved to its own file so it can be reused in the commit overlay too

This is the commit overlay. Did you mean another overlay?

In any case, factored this out into its own file.

goderbauer · 2020-04-09T21:06:50Z

app_flutter/lib/widgets/commit_box.dart

style nit: => only if everything fits in one line: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#consider-using--for-short-functions-and-methods

but https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#use--for-inline-callbacks-that-just-return-list-or-map-literals

goderbauer · 2020-04-09T21:14:55Z

app_flutter/lib/widgets/task_overlay.dart

nit: /// -> //

goderbauer · 2020-04-09T21:44:19Z

app_flutter/lib/widgets/lattice.dart

Not having this will break a11y scrolling on some platforms :(

oh, interesting. The API docs weren't clear about that.

Actually looks like what really matters is that the render object doesn't implement showOnScreen. I've added a dummy implementation for now. It's not actually clear to me how this object should work with accessibility. I mean, it probably needs a semantic label on each LatticeCell, and the accessibility logic should probably build a ton of accessibility nodes and the performance will suck... right now it's pretty much inaccessible since it's all just a big canvas with a few random nodes in it.

Oh, right. The RenderViewportBase and the RenderSingleChildViewport are using getOffsetToReveal to implement showOnScreen, but you're not subclassing from those. So, yes, as you said, you'll need to implement showOnScreen instead (which I imagine would probably use getOffsetToReveal behind the scene, though).

CaseyHillers

LGTM on the Cocoon side once the goldens are figured out. Thank you for making this change!

CaseyHillers · 2020-04-10T00:00:21Z

app_flutter/lib/widgets/pulse.dart

Can you add a quick dart doc for explaining that is used to pulsate tasks that are currently running in the TaskGrid?

yup! Sorry I keep forgetting to add dartdocs to the top-level classes!

CaseyHillers · 2020-04-10T00:04:35Z

app_flutter/lib/widgets/task_icon.dart

I was thinking an icon getter so we can just called qualifiedTask.icon here. However, i'll leave it up to you :)

CaseyHillers · 2020-04-10T00:05:07Z

app_flutter/lib/widgets/task_icon.dart

My reasoning is it might be nice to include the icon in the task overlay

app_flutter/lib/widgets/task_overlay.dart

This removes the _FakeViewport from the build dashboard implementation. It was added in #741, and we think it's sole purpose was the preserve the 1D expectations of widgets in the framework like scrollbars and overscroll indicators. Since adding support for 2D in the framework, the framework has been updated so that the fake viewport should no longer be necessary. All of the tests pass, so I think this is safe to remove now that the framework is 2D compatible. This came up as a test failure in flutter/flutter#128812, where the function signature of getOffsetToReveal is being changed.

Hixie force-pushed the lattice branch 5 times, most recently from 49ffa8f to 42ac997 Compare April 9, 2020 06:23

CaseyHillers reviewed Apr 9, 2020

View reviewed changes

Hixie force-pushed the lattice branch 3 times, most recently from b6ff5aa to f96bc86 Compare April 9, 2020 22:47

Hixie force-pushed the lattice branch from f96bc86 to 97feae2 Compare April 9, 2020 23:07

goderbauer reviewed Apr 9, 2020

View reviewed changes

Hixie force-pushed the lattice branch from 97feae2 to 5660ef0 Compare April 9, 2020 23:16

CaseyHillers approved these changes Apr 10, 2020

View reviewed changes

Hixie force-pushed the lattice branch 3 times, most recently from f93d175 to dc02700 Compare April 10, 2020 05:04

More efficient build dashboard

ac23820

Hixie force-pushed the lattice branch from dc02700 to ac23820 Compare April 10, 2020 21:21

Hixie merged commit 05e3d65 into flutter:master Apr 10, 2020

Hixie deleted the lattice branch April 10, 2020 23:43

Piinks mentioned this pull request Jun 13, 2023

Remove fake viewport from dashboard #2838

Merged

8 tasks

Conversation

Hixie commented Apr 9, 2020

Uh oh!

Hixie commented Apr 9, 2020

Uh oh!

CaseyHillers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Apr 9, 2020

Uh oh!

Hixie commented Apr 9, 2020

Uh oh!

Hixie commented Apr 9, 2020

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CaseyHillers left a comment