LayoutTree: use different hideCaret name in table

[ethan-james]

Fixes #2707

The calculated indent value for a thought increases when the thought isTableCol2, but its integer indentDepth stays the same. Since moving the cursor into a table hides all thoughts beneath the table, it is straightforward to add 1 to column 2's indentDepth in order to correctly hide the cursor during the transform animation.

[raineorshine]

Tested and works!

Just a recommendation on the implementation.

[raineorshine]

I would avoid treeThoughts here and just determine tableDepth directly from the Redux state in a useSelector. You can use attributeEquals(state, head(rootedParentOf(state, state.cursor)), '=view', 'Table') to determine if the cursor in table col2. This makes all the logic a single selector and avoids the O(n) traversal of treeThoughts.

Separately, there is a performance issue to be aware of. const cursor = useSelector(state => state.cursor) causes the render function get called every time the cursor ref changes, which is not necessary when we are only using it to determine tableDepth. In general, any logic that reduces the range of the value should go in the useSelector. More simply: Aim for "thick" Redux selectors.

(It won't do any harm in this case, because LayoutTree always re-renders when the cursor changes, but it's better to avoid the habit. The LayoutTree is also the heaviest render function in the entire app, so it is highly sensitive to performance issues.)

[ethan-james]

OK, thanks! I'll try to get my head around the rootedParentOf bit.

[raineorshine]

It's basically just path.slice(0, -1) if that helps :)

It handles a few edge cases in the root.

[ethan-james]

I set it up, and it actually does the opposite of what I wanted, returning 1 for column 1 & 0 for column 2, resulting in "C" getting hideCaret2 and "D" getting hideCaret1. But it still works because both values have a greater indent level than "B", which has hideCaret0.

The linearizeTree function, where attributeEquals(state, head(rootedParentOf(state, state.cursor)), '=view', 'Table') appears, is getting childPath passed into it to use as its basePath. childPath is calculated in a fairly complex way, but long story short, if I do this in the selector:

rootedParentOf(state, rootedParentOf(state, state.cursor))

then it appears to work the way I would expect, where column 2 return 1 and column 1 returns 0. Do you think it would be better for me to change it?

[raineorshine]

Ah, yes, that makes sense. A path is in col2 when its grandparent has =view/Table.

You should be able to come up with a selector that gives you 1 under the same circumstances as your previous logic.

[ethan-james]

Is using rootedParentOf twice a good solution, or is there a cleverer utility?

[raineorshine]

It's relatively safe to use parentOf in this case, since root children cannot be in col2 (as they are directly in the root) and root grandchildren are unlikely to be in col2 (as the root itself would have to be a table). However, double rootedParentOf is also fine.

You can always make a grandparentOf helper, or separate out the parent-of-the-parent in a grandparent variable.

[ethan-james]

Thanks for the context. I opted to add rootedGrandparentOf to clean up the selector a bit.

[raineorshine]

As a convention on this project, we generally only allow a single, default export that matches the file name. This makes it easier to know where functions live in the project, and is facilitated by the quick file picker that most editors provide. Reducers/Action Creators are an exception, and a few others, but we try to avoid this when possible.

Could move this into its own file in the src/selectors directory?

[raineorshine]

It might also be worth mentioning in this comment the reason that this is needed. i.e. Why is col2 a special case for the hideCaret depth?