-
Notifications
You must be signed in to change notification settings - Fork 22
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.
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
Refactor Comments as Antd <Tree/> #7802
Conversation
frontend/javascripts/oxalis/view/right-border-tabs/comment_tab/comment.tsx
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/right-border-tabs/comment_tab/comment_tab_view.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
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]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still an open todo, or isn't it? The current version of the code still creates a keyboard on every render cycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should hopefully have tackled it with this commit: 2c55d62
frontend/javascripts/oxalis/view/right-border-tabs/comment_tab/comment_tab_view.tsx
Show resolved
Hide resolved
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:
|
That's very awesome 🎉
Sorry, but I still cannot see any highlighting in the first screenshot 🙈 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 🎉
@hotzenklotz margy merge? |
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):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)