[Canvas V2][1/n] Implement display of measures #31

taneliang · 2020-06-30T03:09:08Z

Depends on #27, #28. Blocked by #29. Part of #30.

To be rebased on top of the #30's PR before merging.

Review notes

This PR is completed but will not compile without #29's PR.

Splitting #30 into a stack of PRs for easier review.

Test Plan

jevakallio · 2020-06-30T16:50:25Z

src/canvas/renderCanvas.js

+          hoveredEvent && hoveredEvent.measure === measure;
+        const showGroupHighlight =
+          hoveredEvent &&
+          hoveredEvent.measure !== null &&


Next line will throw if hoveredEvent.measure === undefined. Would probably just do a falsy check, or if there's a lot of this type of deep property munging ahead, use idx.

idx is interesting. Just too bad JS hasn't gotten the optional chaining operator yet.

Flow should already help to ensure that hoveredEvent.measure is null | ReactMeasure, but yeah a falsy check won't hurt. I'll change it

jevakallio · 2020-06-30T16:53:44Z

src/canvas/renderCanvas.js

-function renderSingleReactMarkOrMeasure({
-  baseY,
-  canvasWidth,
+function renderSingleReactMeasure(


I wonder if these big functions would be better off using an object argument with destructuring to simulate named arguments?

For long args lists, named arguments are more readable than positional at the call site, and since these args are mostly primitive data types, it's easy to pass the wrong argument at wrong position and still pass flow check

Also, IMO these functions would be easier to grok if we explicitly stated the input/output types, but I'm not married to that idea.

Yeah, these suggestions make sense. I'll do these for all the render* functions in a PR on top of 3/n so that we cover all the functions added after this PR and also so that we don't get too many rebase conflicts

Actually, this doesn't seem important enough to warrant the confusion of stacked PR shuffling, so I've added it as a task in #30 to be done in another PR at the top of the stack

jevakallio · 2020-06-30T17:16:08Z

src/canvas/renderCanvas.js

+
+  const y = baseY - offsetY;
+
+  // $FlowFixMe We know these won't be null


This $FlowFixMe doesn't actually reach to the ternary clauses because it only matches literally next code line.

I'd personally solve by throwing instead of console.warning on line 227, allowing flow to refine the types to always strings.

jevakallio

Few nits, but over all looking good.

For this type open source code base, where future contributors may not have the kind of time we have to familiarise themselves in the codebase, I'd like to see more comments. But these can be added at a later iteration if needed.

What's the rationale for data and dataV2? Are both to be supported in the long term, or are we deprecating data?

jevakallio · 2020-06-30T17:30:53Z

src/canvas/renderCanvas.js

@@ -488,9 +568,9 @@ export const renderCanvas = memoize(

    renderBackgroundFills(context, canvasWidth, canvasHeight);

-    renderReactMarksAndMeasures(
+    const schedulerAreaEndY = renderReactMarksAndMeasures(


It's really quite unintuitive that renderReactMarksAndMeasures returns its minY. I understand why that needs to be computed there, but it should be more visible, as I only realised this is what was happening while reviewing 2/n

renderReactMarksAndMeasuresAndReturnComputedLaneHeight? 😜

Update from 3/n, now that both renderReactEventRow and renderReactMeasures return a new Y, the "stacking" logic is much clearer than it was when it was just one function.

Wouldn't got as far as renaming them anymore, but I'd definitely annotate their function signatures with : number to make it obvious from the start they return something, and add a comment explaining what they return.

Yeah, my idea was for all these render functions to report what they rendered so that we can lay everything out based on what was rendered.

I think this isn't super clear because we're rendering in overlapping layers, and not all layers have stacks. We're also stacking exclusively in the Y direction, but there's definitely a possibility of stacking in the X direction as well (e.g. for the old priority labels).

I'll add the : number in this PR.

There's also the unrelated issue of having to recalculate all of these in the getHoveredEvent util function. Ideally getHoveredEvent will be able to reuse the calculations from these render functions so that we can be more confident that the returned hovered events are actually the ones rendered on screen.

taneliang · 2020-07-01T00:41:25Z

Yeah, I had both data and dataV2 around so that we didn't have to migrate all render* functions to V2 data at once, but I noticed too late yesterday that data is no longer used in renderCanvas. I'll remove it in this PR

taneliang · 2020-07-01T01:12:06Z

Rebased on master and resolved most review comments

taneliang · 2020-07-03T02:33:34Z

Rebased on master and update renderCanvas call in CanvasPage.js

taneliang force-pushed the eliang/canvas-v2-data-1 branch from bb82a38 to 74d629c Compare June 30, 2020 03:55

This was referenced Jun 30, 2020

[Canvas V2][2/n] Implement lane dividers and background #32

Merged

Migration to V2 data: migrate canvas functions code to V2 data #30

Closed

Fix preprocessDataV2 to prevent enormous React timestamps #28

Merged

jevakallio reviewed Jun 30, 2020

View reviewed changes

jevakallio approved these changes Jun 30, 2020

View reviewed changes

jevakallio reviewed Jun 30, 2020

View reviewed changes

Base automatically changed from eliang/break-renderCanvas to master June 30, 2020 23:26

taneliang force-pushed the eliang/canvas-v2-data-1 branch from 74d629c to 11f5207 Compare July 1, 2020 01:11

taneliang force-pushed the eliang/canvas-v2-data-1 branch 2 times, most recently from b38d58c to 2adb0e9 Compare July 1, 2020 01:18

[Canvas V2][1/n] Implement display of measures

f754719

taneliang force-pushed the eliang/canvas-v2-data-1 branch from 2adb0e9 to f754719 Compare July 3, 2020 02:32

taneliang marked this pull request as ready for review July 3, 2020 02:34

taneliang merged commit c075bdc into master Jul 3, 2020

taneliang deleted the eliang/canvas-v2-data-1 branch July 3, 2020 02:35

kartikcho pushed a commit that referenced this pull request Jul 3, 2020

[Canvas V2][1/n] Implement display of measures (#31)

e0baed7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Canvas V2][1/n] Implement display of measures #31

[Canvas V2][1/n] Implement display of measures #31

taneliang commented Jun 30, 2020 •

edited

Loading

jevakallio Jun 30, 2020

taneliang Jun 30, 2020

jevakallio Jun 30, 2020

jevakallio Jun 30, 2020

taneliang Jun 30, 2020

taneliang Jul 1, 2020 •

edited

Loading

jevakallio Jun 30, 2020

jevakallio left a comment

jevakallio Jun 30, 2020 •

edited

Loading

jevakallio Jun 30, 2020

taneliang Jul 1, 2020

taneliang commented Jul 1, 2020 •

edited

Loading

taneliang commented Jul 1, 2020

taneliang commented Jul 3, 2020


		const y = baseY - offsetY;

		// $FlowFixMe We know these won't be null

[Canvas V2][1/n] Implement display of measures #31

[Canvas V2][1/n] Implement display of measures #31

Conversation

taneliang commented Jun 30, 2020 • edited Loading

Review notes

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taneliang Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jevakallio left a comment

Choose a reason for hiding this comment

jevakallio Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taneliang commented Jul 1, 2020 • edited Loading

taneliang commented Jul 1, 2020

taneliang commented Jul 3, 2020

taneliang commented Jun 30, 2020 •

edited

Loading

taneliang Jul 1, 2020 •

edited

Loading

jevakallio Jun 30, 2020 •

edited

Loading

taneliang commented Jul 1, 2020 •

edited

Loading