Skip to content

Refactor action bar components to React functional components #8754

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

Merged
merged 18 commits into from
Jul 14, 2025
Merged

Refactor action bar components to React functional components #8754

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
4a50f8f
Refactor action bar components to functional components using hooks
hotzenklotz Jul 4, 2025
929fc74
Merge branch 'master' into refactor_tracing_ui
hotzenklotz Jul 7, 2025
b75db00
apply CodeRabbit feedback
hotzenklotz Jul 7, 2025
089b569
Merge branch 'master' of github.com:scalableminds/webknossos into ref…
hotzenklotz Jul 9, 2025
9217e23
pr feedback
hotzenklotz Jul 9, 2025
457b6cf
split tracing_actions_view into subcomponents
hotzenklotz Jul 9, 2025
5977990
split dataset position into sub components
hotzenklotz Jul 10, 2025
0f0651b
split save, unde/redo actions into sub components
hotzenklotz Jul 10, 2025
e8e846a
renamed files for camelcase to snake case
hotzenklotz Jul 10, 2025
753df33
typing
hotzenklotz Jul 10, 2025
73dfccb
apply CodeRabbit feedback
hotzenklotz Jul 10, 2025
606f846
Merge branch 'master' of github.com:scalableminds/webknossos into ref…
hotzenklotz Jul 10, 2025
df64f82
apply CodeRabbit feedback
hotzenklotz Jul 10, 2025
e2f7225
use modal hook for consistent styling
hotzenklotz Jul 10, 2025
eb3710a
remove volume/fallback from "Copy to my account" for sandboxed annota…
hotzenklotz Jul 10, 2025
c927767
prevent unnecessary re-renders for SaveButton
hotzenklotz Jul 10, 2025
52bcc55
Merge branch 'master' of github.com:scalableminds/webknossos into ref…
hotzenklotz Jul 14, 2025
f5f0ec6
apply PR feedback
hotzenklotz Jul 14, 2025
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
174 changes: 78 additions & 96 deletions frontend/javascripts/viewer/view/action-bar/dataset_position_view.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,20 @@ import { PushpinOutlined, ReloadOutlined } from "@ant-design/icons";
import { Space } from "antd";
import FastTooltip from "components/fast_tooltip";
import { V3 } from "libs/mjs";
import { useWkSelector } from "libs/react_hooks";
import Toast from "libs/toast";
import { Vector3Input } from "libs/vector_input";
import message from "messages";
import type React from "react";
import { PureComponent } from "react";
import { connect } from "react-redux";
import type { APIDataset } from "types/api_types";
import type { Vector3, ViewMode } from "viewer/constants";
import type { Vector3 } from "viewer/constants";
import constants from "viewer/constants";
import { getDatasetExtentInVoxel } from "viewer/model/accessors/dataset_accessor";
import { getPosition, getRotation } from "viewer/model/accessors/flycam_accessor";
import { setPositionAction, setRotationAction } from "viewer/model/actions/flycam_actions";
import type { Flycam, Task, WebknossosState } from "viewer/store";
import Store from "viewer/store";
import { ShareButton } from "viewer/view/action-bar/share_modal_view";
import ButtonComponent from "viewer/view/components/button_component";

type Props = {
flycam: Flycam;
viewMode: ViewMode;
dataset: APIDataset;
task: Task | null | undefined;
};
const positionIconStyle: React.CSSProperties = {
transform: "rotate(-45deg)",
marginRight: 0,
Expand All @@ -42,29 +33,33 @@ const positionInputErrorStyle: React.CSSProperties = {
...warningColors,
};

class DatasetPositionView extends PureComponent<Props> {
copyPositionToClipboard = async () => {
const position = V3.floor(getPosition(this.props.flycam)).join(", ");
function DatasetPositionView() {
const flycam = useWkSelector((state) => state.flycam);
const viewMode = useWkSelector((state) => state.temporaryConfiguration.viewMode);
const dataset = useWkSelector((state) => state.dataset);
const task = useWkSelector((state) => state.task);

const copyPositionToClipboard = async () => {
const position = V3.floor(getPosition(flycam)).join(", ");
await navigator.clipboard.writeText(position);
Toast.success("Position copied to clipboard");
};

copyRotationToClipboard = async () => {
const rotation = V3.round(getRotation(this.props.flycam)).join(", ");
const copyRotationToClipboard = async () => {
const rotation = V3.round(getRotation(flycam)).join(", ");
await navigator.clipboard.writeText(rotation);
Toast.success("Rotation copied to clipboard");
};

handleChangePosition = (position: Vector3) => {
const handleChangePosition = (position: Vector3) => {
Store.dispatch(setPositionAction(position));
};

handleChangeRotation = (rotation: Vector3) => {
const handleChangeRotation = (rotation: Vector3) => {
Store.dispatch(setRotationAction(rotation));
};

isPositionOutOfBounds = (position: Vector3) => {
const { dataset, task } = this.props;
const isPositionOutOfBounds = (position: Vector3) => {
Copy link
Member

Choose a reason for hiding this comment

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

this is a general pattern about which I'm a bit hesitant. in class components, (helper) methods don't really have a cost because they are set up once for the lifecycle of the component. however, in functional components, these local functions are defined in every "call" (I don't use "render" here because even when no re-render happens, the function is called to get its jsx) of that component. these redefinition have (at least) two downsides:

  • performance for the definition itself
  • new identities are created in each call (usually causes unnecessary rerenders)

useCallback is a pattern which can mitigate this a bit (the second point is solved fully by it). The downside is that it's annoying to list all dependencies. However, that's the way it is I guess (at least until the react compiler hits the road).

One other solution is possible for functions that merely act as handlers (e.g., onclick handlers). The result of such handlers doesn't influence the actual rendering. This is why it is fine to lift them to the module-level and make them simply call Store.getState(). Then, one doesn't need to use useCallback with its dependencies.

In the code highlighted above copyRotationToClipboard, handleChangePosition and handleChangeRotation are functions that can be lifted to the top level (the flycam can be retrieved from Store.getState()). However, isPositionOutOfBounds directly influences the rendering output which is why it must not be lifted (instead, useCallback would make sense).

For other components, this might not make much of a difference (perf-wise), but the action bar has a high impact on the FPS performance because it's updated very frequently. Therefore, I'd like to get this right.

const { min: datasetMin, max: datasetMax } = getDatasetExtentInVoxel(dataset);

const isPositionOutOfBounds = (min: Vector3, max: Vector3) =>
Expand Down Expand Up @@ -95,100 +90,87 @@ class DatasetPositionView extends PureComponent<Props> {
};
};

render() {
const position = V3.floor(getPosition(this.props.flycam));
const { isOutOfDatasetBounds, isOutOfTaskBounds } = this.isPositionOutOfBounds(position);
const iconColoringStyle = isOutOfDatasetBounds || isOutOfTaskBounds ? iconErrorStyle : {};
const positionInputStyle =
isOutOfDatasetBounds || isOutOfTaskBounds
? positionInputErrorStyle
: positionInputDefaultStyle;
let maybeErrorMessage = null;

if (isOutOfDatasetBounds) {
maybeErrorMessage = message["tracing.out_of_dataset_bounds"];
} else if (!maybeErrorMessage && isOutOfTaskBounds) {
maybeErrorMessage = message["tracing.out_of_task_bounds"];
}
const position = V3.floor(getPosition(flycam));
const { isOutOfDatasetBounds, isOutOfTaskBounds } = isPositionOutOfBounds(position);
const iconColoringStyle = isOutOfDatasetBounds || isOutOfTaskBounds ? iconErrorStyle : {};
const positionInputStyle =
isOutOfDatasetBounds || isOutOfTaskBounds ? positionInputErrorStyle : positionInputDefaultStyle;
let maybeErrorMessage = null;

if (isOutOfDatasetBounds) {
maybeErrorMessage = message["tracing.out_of_dataset_bounds"];
} else if (!maybeErrorMessage && isOutOfTaskBounds) {
maybeErrorMessage = message["tracing.out_of_task_bounds"];
}

const rotation = V3.round(getRotation(this.props.flycam));
const isArbitraryMode = constants.MODES_ARBITRARY.includes(this.props.viewMode);
const positionView = (
<div
const rotation = V3.round(getRotation(flycam));
const isArbitraryMode = constants.MODES_ARBITRARY.includes(viewMode);
const positionView = (
<div
style={{
display: "flex",
}}
>
<Space.Compact
style={{
display: "flex",
whiteSpace: "nowrap",
}}
>
<FastTooltip title={message["tracing.copy_position"]} placement="bottom-start">
<ButtonComponent
onClick={copyPositionToClipboard}
style={{ padding: "0 10px", ...iconColoringStyle }}
className="hide-on-small-screen"
>
<PushpinOutlined style={positionIconStyle} />
</ButtonComponent>
</FastTooltip>
<Vector3Input
value={position}
onChange={handleChangePosition}
autoSize
style={positionInputStyle}
allowDecimals
/>
<ShareButton dataset={dataset} style={iconColoringStyle} />
</Space.Compact>
{isArbitraryMode ? (
<Space.Compact
style={{
whiteSpace: "nowrap",
marginLeft: 10,
}}
>
<FastTooltip title={message["tracing.copy_position"]} placement="bottom-start">
<FastTooltip title={message["tracing.copy_rotation"]} placement="bottom-start">
<ButtonComponent
onClick={this.copyPositionToClipboard}
style={{ padding: "0 10px", ...iconColoringStyle }}
onClick={copyRotationToClipboard}
style={{
padding: "0 10px",
}}
className="hide-on-small-screen"
>
<PushpinOutlined style={positionIconStyle} />
<ReloadOutlined />
</ButtonComponent>
</FastTooltip>
<Vector3Input
value={position}
onChange={this.handleChangePosition}
autoSize
style={positionInputStyle}
value={rotation}
onChange={handleChangeRotation}
style={{
textAlign: "center",
width: 120,
}}
allowDecimals
/>
<ShareButton dataset={this.props.dataset} style={iconColoringStyle} />
</Space.Compact>
{isArbitraryMode ? (
<Space.Compact
style={{
whiteSpace: "nowrap",
marginLeft: 10,
}}
>
<FastTooltip title={message["tracing.copy_rotation"]} placement="bottom-start">
<ButtonComponent
onClick={this.copyRotationToClipboard}
style={{
padding: "0 10px",
}}
className="hide-on-small-screen"
>
<ReloadOutlined />
</ButtonComponent>
</FastTooltip>
<Vector3Input
value={rotation}
onChange={this.handleChangeRotation}
style={{
textAlign: "center",
width: 120,
}}
allowDecimals
/>
</Space.Compact>
) : null}
</div>
);
return (
<FastTooltip title={maybeErrorMessage || null} wrapper="div">
{positionView}
</FastTooltip>
);
}
}
) : null}
</div>
);

function mapStateToProps(state: WebknossosState): Props {
return {
flycam: state.flycam,
viewMode: state.temporaryConfiguration.viewMode,
dataset: state.dataset,
task: state.task,
};
return (
<FastTooltip title={maybeErrorMessage || null} wrapper="div">
{positionView}
</FastTooltip>
);
}

const connector = connect(mapStateToProps);
export default connector(DatasetPositionView);
export default DatasetPositionView;
Loading