From 8d2f13d2399dd4c0bc0c330f28475661a9dbafbe Mon Sep 17 00:00:00 2001 From: hlomzik Date: Mon, 11 Nov 2024 15:16:14 +0000 Subject: [PATCH 1/7] fix: LEAP-1615: Fix tool unselect Pan was blocking the app upon unselection, because there were no tools selected after this. When drawing tools are not displayed (like in usual RectangleLabels case) then it's hard to start drawing again. --- web/libs/editor/src/tools/Manager.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/web/libs/editor/src/tools/Manager.js b/web/libs/editor/src/tools/Manager.js index e863983d9c87..6ee654ef7dfd 100644 --- a/web/libs/editor/src/tools/Manager.js +++ b/web/libs/editor/src/tools/Manager.js @@ -123,12 +123,12 @@ class ToolsManager { if (selected) { this.unselectAll(); - if (tool.setSelected) tool.setSelected(true); + tool.setSelected?.(true); } else { const drawingTool = this.findDrawingTool(); if (drawingTool) return this.selectTool(drawingTool, true); - if (tool.setSelected) tool.setSelected(false); + this.selectTool(this._default_tool, true); } } From f7c1fcbbddc01b0ad5f833ee39d51b67ab9e988e Mon Sep 17 00:00:00 2001 From: hlomzik Date: Mon, 11 Nov 2024 15:20:05 +0000 Subject: [PATCH 2/7] Also make Move tool unselectable The same way as Pan tool: default tool will be selected. --- web/libs/editor/src/tools/Selection.js | 36 +++++++++++++++++--------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/web/libs/editor/src/tools/Selection.js b/web/libs/editor/src/tools/Selection.js index d15dbdf2f390..f3eeb6e96c43 100644 --- a/web/libs/editor/src/tools/Selection.js +++ b/web/libs/editor/src/tools/Selection.js @@ -1,26 +1,38 @@ +import { observer } from "mobx-react"; import { types } from "mobx-state-tree"; -import BaseTool from "./Base"; -import ToolMixin from "../mixins/Tool"; -import { AnnotationMixin } from "../mixins/AnnotationMixin"; import { IconMoveTool } from "../assets/icons"; +import { Tool } from "../components/Toolbar/Tool"; +import { AnnotationMixin } from "../mixins/AnnotationMixin"; +import ToolMixin from "../mixins/Tool"; import { FF_LSDV_4930, isFF } from "../utils/feature-flags"; +import BaseTool from "./Base"; + +const ToolView = observer(({ item }) => { + return ( + } + label="Move" + shortcut={item.shortcut} + extraShortcuts={item.extraShortcuts} + onClick={() => { + item.manager.selectTool(item, !item.selected); + }} + /> + ); +}); const _Tool = types .model("SelectionTool", { shortcut: "V", group: "control", }) - .views(() => { + .views((self) => { return { - get isSeparated() { - return true; - }, - get viewTooltip() { - return "Move"; - }, - get iconComponent() { - return IconMoveTool; + get viewClass() { + return () => ; }, get useTransformer() { return true; From 07068c325a0e5c66628aeebb4adce26f98bc92b4 Mon Sep 17 00:00:00 2001 From: hlomzik Date: Mon, 11 Nov 2024 15:20:54 +0000 Subject: [PATCH 3/7] Also fix smart tools selection If user clicks on exact icon in popup panel the next tool is actually selected, because there are two handlers active at this moment. Fix this by blocking the more general one, used to rotate through all smart tools. --- .../editor/src/components/Toolbar/Toolbar.jsx | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/web/libs/editor/src/components/Toolbar/Toolbar.jsx b/web/libs/editor/src/components/Toolbar/Toolbar.jsx index 1fdcc5563e31..c2eecf503b9a 100644 --- a/web/libs/editor/src/components/Toolbar/Toolbar.jsx +++ b/web/libs/editor/src/components/Toolbar/Toolbar.jsx @@ -1,13 +1,15 @@ import { useMemo, useState } from "react"; -import { Block, Elem } from "../../utils/bem"; -import "./Toolbar.scss"; -import "./Tool.scss"; -import "./FlyoutMenu.scss"; +import { inject, observer } from "mobx-react"; + import { useWindowSize } from "../../common/Utils/useWindowSize"; +import { Block, cn, Elem } from "../../utils/bem"; import { isDefined } from "../../utils/utilities"; -import { inject, observer } from "mobx-react"; -import { ToolbarProvider } from "./ToolbarContext"; import { Tool } from "./Tool"; +import { ToolbarProvider } from "./ToolbarContext"; + +import "./FlyoutMenu.scss"; +import "./Tool.scss"; +import "./Toolbar.scss"; export const Toolbar = inject("store")( observer(({ store, tools, expanded }) => { @@ -110,9 +112,12 @@ const SmartTools = observer(({ tools }) => { ) : null } controls={selected.controls} - onClick={() => { + onClick={(e) => { let nextIndex = selectedIndex + 1; + // if that's a smart button in extra block, it's already selected + if (e.target.closest(`.${cn("tool").elem("extra")}`)) return; + if (!hasSelected) nextIndex = 0; else if (nextIndex >= tools.length) nextIndex = 0; From 1a8af690c9631bfa984f98a2421a7263dc58c2a0 Mon Sep 17 00:00:00 2001 From: hlomzik Date: Thu, 14 Nov 2024 12:31:29 +0000 Subject: [PATCH 4/7] Fix aria-label to match the auto generated one It's used in tests and they are failing now, because naming was changed --- web/libs/editor/src/tools/Selection.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/libs/editor/src/tools/Selection.js b/web/libs/editor/src/tools/Selection.js index f3eeb6e96c43..adf62f0ae227 100644 --- a/web/libs/editor/src/tools/Selection.js +++ b/web/libs/editor/src/tools/Selection.js @@ -11,7 +11,7 @@ import BaseTool from "./Base"; const ToolView = observer(({ item }) => { return ( } label="Move" From a1b8d2d2a43286a77bae77249e336980198e8a73 Mon Sep 17 00:00:00 2001 From: hlomzik Date: Thu, 14 Nov 2024 12:55:16 +0000 Subject: [PATCH 5/7] Make `selectTool()` more readable --- web/libs/editor/src/tools/Manager.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/web/libs/editor/src/tools/Manager.js b/web/libs/editor/src/tools/Manager.js index 6ee654ef7dfd..3f183741b96b 100644 --- a/web/libs/editor/src/tools/Manager.js +++ b/web/libs/editor/src/tools/Manager.js @@ -127,8 +127,7 @@ class ToolsManager { } else { const drawingTool = this.findDrawingTool(); - if (drawingTool) return this.selectTool(drawingTool, true); - this.selectTool(this._default_tool, true); + this.selectTool(drawingTool ?? this._default_tool, true); } } From 3917bb8636e333687d89f642adce2fa398db8b6a Mon Sep 17 00:00:00 2001 From: hlomzik Date: Mon, 18 Nov 2024 15:53:01 +0000 Subject: [PATCH 6/7] Fix tests --- .../tests/e2e/tests/regression-tests/brush-relations.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/web/libs/editor/tests/e2e/tests/regression-tests/brush-relations.test.js b/web/libs/editor/tests/e2e/tests/regression-tests/brush-relations.test.js index b074bffcd6b9..87157f05c0aa 100644 --- a/web/libs/editor/tests/e2e/tests/regression-tests/brush-relations.test.js +++ b/web/libs/editor/tests/e2e/tests/regression-tests/brush-relations.test.js @@ -82,8 +82,8 @@ Scenario("Brush relations shouldn't crash everything", async ({ I, LabelStudio, AtImageView.waitForImage(); // Check that relation still exist AtSidebar.seeRelations(1); - // switch to the move tool for easy region selecting - I.pressKey("v"); + // move tool is already selected and stored so we don't need to select it again + // I.pressKey("v"); // select the third region AtImageView.clickAt(regionsCentralPoints[2].x, regionsCentralPoints[2].y); // create relation to the fourth region From e04178ed82d312a1c62ccd27df778238836b4f79 Mon Sep 17 00:00:00 2001 From: hlomzik Date: Tue, 19 Nov 2024 02:18:33 +0000 Subject: [PATCH 7/7] Fix hotkey handler --- web/libs/editor/src/components/Toolbar/Toolbar.jsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/web/libs/editor/src/components/Toolbar/Toolbar.jsx b/web/libs/editor/src/components/Toolbar/Toolbar.jsx index c2eecf503b9a..7e848e3bed94 100644 --- a/web/libs/editor/src/components/Toolbar/Toolbar.jsx +++ b/web/libs/editor/src/components/Toolbar/Toolbar.jsx @@ -116,7 +116,8 @@ const SmartTools = observer(({ tools }) => { let nextIndex = selectedIndex + 1; // if that's a smart button in extra block, it's already selected - if (e.target.closest(`.${cn("tool").elem("extra")}`)) return; + // if it's a hotkey handler, there are no `e` event + if (e?.target?.closest(`.${cn("tool").elem("extra")}`)) return; if (!hasSelected) nextIndex = 0; else if (nextIndex >= tools.length) nextIndex = 0;