Refactor Comments as Antd <Tree/>

[hotzenklotz]

We have three different tree hierarchy implementations in WK for skeletons, comments and segments. In this PR I refactor the comments tab to use the antd <Tree /> component, similar to the Segments tab.

I also updated the Comments Tab to be a React functional component.

URL of deployed dev instance (used for testing):

• https://commenttree.webknossos.xyz/

Steps to test:

• Create a few skeletons and create comments for them
  • Add a few markdown comments
• Create empty tree with no nodes (and hence no comments)
• Import a large NML with many comments and check performance

TODOs:

• Test rendering performance

Issues:

• Contributes to WIP Update React to v17 #7765

---

(Please delete unneeded items, merge only when none are left open)

• Updated changelog
• Updated migration guide if applicable
• Updated documentation if applicable
• Adapted wk-libs python client if relevant API parts change
• Removed dev-only changes like prints and application.conf edits
• Considered common edge cases
• Needs datastore update after deployment

[MichaelBuessemeyer]

Thanks for the refactoring 🙏

I noticed one potential improvement (not creating a keyboard input instance in every rendering cycle) and found a major visual change during testing. In the image the active node id is 2, but its comment is not highlighted.

This is how it looks on the master: The comment of the active node is highlighted.

But it is not this branch. The changes seemed like you removed the highlighting intentionally. Is that so? And if yes, why?

Besides this, everything lgtm :)

[MichaelBuessemeyer]

I'd say that this code instanciates a new keyboard on every rerender 🤔. But we only want to create one on the first rendering cycle.

I'd say putting the instanciation into the useEffect might be better:

Suggested change

	const keyboard = new InputKeyboard(
	{
	n: () => this.nextComment(),
	p: () => this.previousComment(),
	n: () => nextComment(),
	p: () => previousComment(),
	},
	{
	delay: Store.getState().userConfiguration.keyboardDelay,
	delay: keyboardDelay,
	},
	);
	state: CommentTabState = {
	isSortedAscending: true,
	sortBy: SortByEnum.NAME,
	collapsedTreeIds: {},
	isMarkdownModalOpen: false,
	};
	componentDidMount() {
	this.storePropertyUnsubscribers.push(
	listenToStoreProperty(
	(state) => state.userConfiguration.keyboardDelay,
	(keyboardDelay) => {
	if (this.keyboard != null) {
	this.keyboard.delay = keyboardDelay;
	}
	},
	),
	);
	}
	shouldComponentUpdate(nextProps: PropsWithSkeleton, nextState: CommentTabState) {
	if (nextState !== this.state) {
	return true;
	}
	if (this.props.skeletonTracing.activeNodeId !== nextProps.skeletonTracing.activeNodeId) {
	return true;
	}
	if (this.props.skeletonTracing.activeTreeId !== nextProps.skeletonTracing.activeTreeId) {
	return true;
	}
	const updateActions = Array.from(
	cachedDiffTrees(this.props.skeletonTracing.trees, nextProps.skeletonTracing.trees),
	);
	const relevantUpdateActions = updateActions.filter(
	(ua) =>
	RELEVANT_ACTIONS_FOR_COMMENTS.includes(ua.name) ||
	(ua.name === "createTree" && ua.value.comments.length > 0),
	);
	return relevantUpdateActions.length > 0;
	}
	componentDidUpdate(prevProps: PropsWithSkeleton) {
	if (
	this.listRef != null &&
	prevProps.skeletonTracing.trees !== this.props.skeletonTracing.trees
	) {
	// Force the virtualized list to update if a comment was changed
	// as it only rerenders if the rowCount changed otherwise
	this.listRef.forceUpdateGrid();
	}
	}
	componentWillUnmount() {
	this.keyboard.destroy();
	this.unsubscribeStoreListeners();
	useEffect(() => {
	keyboard.delay = keyboardDelay;
	return () => {
	keyboard.destroy();
	};
	}, [keyboard, keyboardDelay]);
	useEffect(() => {
	const keyboard = new InputKeyboard(
	{
	n: () => nextComment(),
	p: () => previousComment(),
	},
	{ delay: keyboardDelay },
	);
	return () => {
	keyboard.destroy();
	};
	}, [keyboard, keyboardDelay]);

[hotzenklotz]

Putting the keyboard in an useEffect will create a new Keyboard every time the keyboardDelay changes. The keyboard delay can be adjusted in the UI by the user. So maybe putting it in useEffectOnlyOnce is the right way..?

[MichaelBuessemeyer]

Yeah you are totally right. Sorry, I missed that.

So I would suggest creating a convenience hook for this as this looks like a reoccurring pattern to me, which could be encapsulated into an own hook:

function useInstantiateAndUnmountOnly<T>(createInstance: () => T, destroyInstance: (T) => void){
  const [instance, setInstance] = useState<T>(null);
  useEffectOnlyOnce(() => {
    setInstance(createInstance());
    return () => destroyInstance(instance);
  });
  return instance;
}

And then use it kinda like this:

useInstantiateAndUnmountOnly<Keyboard>(() => new Keyboard(....), (keyboard) => keyboard.destory());
useEffect(() => {
    keyboard.delay = keyboardDelay;
  }, [keyboardDelay]);

The code like this should ensure to only instantiate one keyboard, destroy it on unmount and update its keyboard delay once changed by the user.

What do you think of my idea?

[MichaelBuessemeyer]

This is still an open todo, or isn't it? The current version of the code still creates a keyboard on every render cycle.

[hotzenklotz]

I should hopefully have tackled it with this commit: 2c55d62

[hotzenklotz]

I noticed one potential improvement (not creating a keyboard input instance in every rendering cycle) and found a major visual change during testing. In the image the active node id is 2, but its comment is not highlighted.

I think you are confusing things here. In first screenshot, the comment belonging to node 2 is indeed highlighted in "blue". In you second screenshot ("old version"), the tree with id #2 is highlighted in a addition to the node with id 6 is highlighted.

The main visual changes in the PR are:

• I removed highlighting of the active tree, so that only one element (a comment) is always highlighted.
• I removed the tree id in front of the tree names. They are not helpful and are not present in the skeleton tab either. Instead I replaced them with the colored dot, similar to the skeleton tab.
• I removed the ">" indicator in front of the active comment. Instead the comment is simply highlighted ("background is blue")

[MichaelBuessemeyer]

I removed the tree id in front of the tree names. They are not helpful and are not present in the skeleton tab either. Instead, I replaced them with the colored dot, similar to the skeleton tab.

That's very awesome 🎉

In first screenshot, the comment belonging to node 2 is indeed highlighted in "blue".

Sorry, but I still cannot see any highlighting in the first screenshot 🙈
Edit: Ok turns out, the color space settings of my display did not show the highlighting ... 🤣 => The brightness and contrast settings were at fault. (too high contrast)

[MichaelBuessemeyer]

Awesome 🎉

[MichaelBuessemeyer]

@hotzenklotz margy merge?