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.

Sign up for GitHub

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

Show top level sections in List View #65202

Merged
merged 4 commits into from
Sep 12, 2024
Merged

Show top level sections in List View #65202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
90c7702
Show top level sections in List View
getdave Sep 10, 2024
98ba5ff
Recompute enabled clientIds when editorMode changes
getdave Sep 10, 2024
3fb66f4
Revert initial changes
getdave Sep 11, 2024
45e98a0
Avoid special conditional for Zoom Out mode
getdave Sep 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions packages/block-editor/src/components/list-view/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,20 +119,16 @@ function ListViewComponent(
const blockIndexes = useListViewBlockIndexes( clientIdsTree );

const { getBlock } = useSelect( blockEditorStore );
const { visibleBlockCount, shouldShowInnerBlocks } = useSelect(
const { visibleBlockCount } = useSelect(
( select ) => {
const {
getGlobalBlockCount,
getClientIdsOfDescendants,
__unstableGetEditorMode,
} = select( blockEditorStore );
const { getGlobalBlockCount, getClientIdsOfDescendants } =
select( blockEditorStore );
const draggedBlockCount =
draggedClientIds?.length > 0
? getClientIdsOfDescendants( draggedClientIds ).length + 1
: 0;
return {
visibleBlockCount: getGlobalBlockCount() - draggedBlockCount,
shouldShowInnerBlocks: __unstableGetEditorMode() !== 'zoom-out',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's allow inner blocks to show in Zoom Out mode because we need to see the "sections" which are inner blocks of the sectionRoot.

};
},
[ draggedClientIds ]
Expand Down Expand Up @@ -397,7 +393,6 @@ function ListViewComponent(
fixedListWindow={ fixedListWindow }
selectedClientIds={ selectedClientIds }
isExpanded={ isExpanded }
shouldShowInnerBlocks={ shouldShowInnerBlocks }
showAppender={ showAppender }
/>
</ListViewContext.Provider>
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/store/private-selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ export const getEnabledClientIdsTree = createSelector(
state.blockEditingModes,
state.settings.templateLock,
state.blockListSettings,
state.editorMode,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

List View depends on this selector to determine which clientIds to display. As it's memoized, when we switch editor modes we will need to recompute the value to ensure it's accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd really like a confidence check on this one. I would have thought that having the state.blockEditingModes dependencies would be enough here but apparently not 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@talldan This turns out to still be required otherwise List View doesn't not reflect the changes correctly. Going to have to continue digging to work out why though...

Copy link
Contributor

Choose a reason for hiding this comment

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

To define the dependencies of a selector, we need to just look what selectors it uses and what state it users So we need to look at. getEnabledClientIdsTreeUnmemoized. So basically we need this to be the sum of getBlockEditingMode's dependencies and getBlockOrder's dependencies.

so state.blocks.order + state.editorMode + state.settings?.[ sectionRootClientIdKey ] + state.settings.templateLock + state.blockEditingModes + state.blocks.parents (same thing as order though, just denormalized) + state.blockListSettings + state.blocks.byClientId + state.blocks.attributes (for native only) + registered blocks.

It's a very hard selector to memoize IMO. We have options:

  • Keep as is and risk re-renderings that are not needed that often.
  • Remove the need for this selector (probably involves component refactoring)
  • Denormalize the selector in a dedicated redux state (like we do for blocks.tree...) it's very hard to get right though and I would only do it if we realize this selector is a real bottleneck performance wise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is missing a lot of dependencies, not just state.editorMode. There's a good chance that there's other subtle bugs because of this, but adding this many dependencies pretty much negates the usefulness of the memoization.

For now, I think we should keep things as-is to fix the obvious bug of not updating the list view correctly. The editorMode doesn't get updated that frequently, so I don't think it will have that large of a performance impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To define the dependencies of a selector, we need to just look what selectors it uses

I came to the same conclusion about dependencies here.

#65202 (comment)

It's a very hard selector to memoize IMO

Agreed.

Keep as is and risk re-renderings that are not needed that often.
Remove the need for this selector (probably involves component refactoring)
Denormalize the selector in a dedicated redux state (like we do for blocks.tree...) it's very hard to get right though and I would only do it if we realize this selector is a real bottleneck performance wise.

editorMode doesn't change often and I can't imagine that changing in the future. We're never very unlikely going to change editorMode on an action that runs repeatedly as it would be poor for UX.

On balance I'd say running with the fix in this PR would be an acceptable trade off.

Would it help if I added a comment to the selector to note that it may be missing dependencies and referring to this discussion? That way if it becomes a problem in the future we know where to look.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's definitely acceptable to run with this PR, my worry is that one of the underlying selectors change and we don't update the dependencies here. It's indirect.

Copy link
Contributor Author

@getdave getdave Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree.

The only viable options seems to be:

Remove the need for this selector (probably involves component refactoring)

It would be cool if you could declare a selector as dependency and then it would automatically use that selector's dependencies as dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can kind of do that:

gutenberg/packages/block-editor/src/store/selectors.js

Line 850 in 9bdfebf

...getSelectedBlockClientIds.getDependants( state ),

But I'm not sure it'd work here because of the way it depends on blockEditingMode (unmemoized, and also per block), which then depends on editorMode. 🤔

]
);

Expand Down
Loading