From 49b78d726af7b5e9ed43c084cdd063c74e36a9e2 Mon Sep 17 00:00:00 2001 From: Frank Noirot Date: Mon, 19 May 2025 13:21:43 -0400 Subject: [PATCH] Submit selection to command on unmount of selection arg input (#7047) * Submit selection to command on unmount of selection arg input This fixes #7024 by saving the user's selection to the command even if they click another argument in the command palette's header. It does no harm to save the selection to the argument, even if it's being torn down because the user dismissed it has no negative effect. * Refactor to not auto-submit before selection is cleared on mount Thanks E2E test suite * Update failing E2E tests with new behavior, which allows skip with preselection --------- Co-authored-by: Pierre Jacquier --- e2e/playwright/command-bar-tests.spec.ts | 12 ++- e2e/playwright/editor-tests.spec.ts | 7 +- e2e/playwright/point-click.spec.ts | 98 +------------------ e2e/playwright/sketch-tests.spec.ts | 1 - e2e/playwright/various.spec.ts | 1 - .../CommandBar/CommandBarSelectionInput.tsx | 28 +++++- .../CommandBarSelectionMixedInput.tsx | 20 +++- 7 files changed, 59 insertions(+), 108 deletions(-) diff --git a/e2e/playwright/command-bar-tests.spec.ts b/e2e/playwright/command-bar-tests.spec.ts index 7b9f5e1f7..75fcfa5a3 100644 --- a/e2e/playwright/command-bar-tests.spec.ts +++ b/e2e/playwright/command-bar-tests.spec.ts @@ -38,20 +38,22 @@ test.describe('Command bar tests', () => { // Click the line of code for xLine. await page.getByText(`startProfile(at = [-10, -10])`).click() + // Wait for the selection to register (TODO: we need a definitive way to wait for this) + await page.waitForTimeout(200) + await toolbar.extrudeButton.click() await cmdBar.expectState({ stage: 'arguments', commandName: 'Extrude', - currentArgKey: 'sketches', - currentArgValue: '', + currentArgKey: 'length', + currentArgValue: '5', headerArguments: { - Profiles: '', + Profiles: '1 profile', Length: '', }, - highlightedHeaderArg: 'Profiles', + highlightedHeaderArg: 'length', }) await cmdBar.progressCmdBar() - await cmdBar.progressCmdBar() await cmdBar.expectState({ stage: 'review', commandName: 'Extrude', diff --git a/e2e/playwright/editor-tests.spec.ts b/e2e/playwright/editor-tests.spec.ts index a99d16e01..603e7d684 100644 --- a/e2e/playwright/editor-tests.spec.ts +++ b/e2e/playwright/editor-tests.spec.ts @@ -1131,8 +1131,9 @@ sketch001 = startSketchOn(XZ) await page.waitForTimeout(100) await page.getByText('startProfile(at = [4.61, -14.01])').click() + // Wait for the selection to register (TODO: we need a definitive way to wait for this) + await page.waitForTimeout(200) await toolbar.extrudeButton.click() - await cmdBar.progressCmdBar() await cmdBar.expectState({ stage: 'arguments', currentArgKey: 'length', @@ -1354,7 +1355,9 @@ sketch001 = startSketchOn(XZ) const u = await getUtils(page) const projectLink = page.getByRole('link', { name: 'cube' }) const gizmo = page.locator('[aria-label*=gizmo]') - const resetCameraButton = page.getByRole('button', { name: 'Reset view' }) + const resetCameraButton = page.getByRole('button', { + name: 'Reset view', + }) const locationToHaveColor = async ( position: { x: number; y: number }, color: [number, number, number] diff --git a/e2e/playwright/point-click.spec.ts b/e2e/playwright/point-click.spec.ts index e7a60a6be..9bc355828 100644 --- a/e2e/playwright/point-click.spec.ts +++ b/e2e/playwright/point-click.spec.ts @@ -74,15 +74,6 @@ test.describe('Point-and-click tests', () => { await test.step('do extrude flow and check extrude code is added to editor', async () => { await toolbar.extrudeButton.click() - await cmdBar.expectState({ - stage: 'arguments', - currentArgKey: 'sketches', - currentArgValue: '', - headerArguments: { Profiles: '', Length: '' }, - highlightedHeaderArg: 'Profiles', - commandName: 'Extrude', - }) - await cmdBar.progressCmdBar() await cmdBar.expectState({ stage: 'arguments', currentArgKey: 'length', @@ -1654,15 +1645,6 @@ sketch002 = startSketchOn(plane001) await test.step(`Go through the command bar flow with preselected sketches`, async () => { await toolbar.loftButton.click() - await cmdBar.expectState({ - stage: 'arguments', - currentArgKey: 'sketches', - currentArgValue: '', - headerArguments: { Profiles: '' }, - highlightedHeaderArg: 'Profiles', - commandName: 'Loft', - }) - await cmdBar.progressCmdBar() await cmdBar.expectState({ stage: 'review', headerArguments: { Profiles: '2 profiles' }, @@ -2106,18 +2088,6 @@ extrude001 = extrude(sketch001, length = -12) await test.step(`Apply fillet to the preselected edge`, async () => { await page.waitForTimeout(100) await toolbar.filletButton.click() - await cmdBar.expectState({ - commandName: 'Fillet', - highlightedHeaderArg: 'selection', - currentArgKey: 'selection', - currentArgValue: '', - headerArguments: { - Selection: '', - Radius: '', - }, - stage: 'arguments', - }) - await cmdBar.progressCmdBar() await cmdBar.expectState({ commandName: 'Fillet', highlightedHeaderArg: 'radius', @@ -2647,18 +2617,6 @@ extrude001 = extrude(profile001, length = 5) await test.step(`Apply fillet`, async () => { await page.waitForTimeout(100) await toolbar.filletButton.click() - await cmdBar.expectState({ - commandName: 'Fillet', - highlightedHeaderArg: 'selection', - currentArgKey: 'selection', - currentArgValue: '', - headerArguments: { - Selection: '', - Radius: '', - }, - stage: 'arguments', - }) - await cmdBar.progressCmdBar() await cmdBar.expectState({ commandName: 'Fillet', highlightedHeaderArg: 'radius', @@ -2764,19 +2722,6 @@ extrude001 = extrude(sketch001, length = -12) await test.step(`Apply chamfer to the preselected edge`, async () => { await page.waitForTimeout(100) await toolbar.chamferButton.click() - await cmdBar.expectState({ - commandName: 'Chamfer', - highlightedHeaderArg: 'selection', - currentArgKey: 'selection', - currentArgValue: '', - headerArguments: { - Selection: '', - Length: '', - }, - stage: 'arguments', - }) - await cmdBar.progressCmdBar() - await page.waitForTimeout(1000) await cmdBar.expectState({ commandName: 'Chamfer', highlightedHeaderArg: 'length', @@ -3260,8 +3205,6 @@ extrude001 = extrude(sketch001, length = 30) await test.step(`Go through the command bar flow with a preselected face (cap)`, async () => { await toolbar.shellButton.click() await cmdBar.progressCmdBar() - await page.waitForTimeout(500) - await cmdBar.progressCmdBar() await cmdBar.expectState({ stage: 'review', headerArguments: { @@ -3695,12 +3638,13 @@ tag=$rectangleSegmentC002, // revolve await editor.scrollToText(codeToSelection) await page.getByText(codeToSelection).click() + // Wait for the selection to register (TODO: we need a definitive way to wait for this) + await page.waitForTimeout(200) await toolbar.revolveButton.click() await cmdBar.progressCmdBar() await cmdBar.progressCmdBar() await cmdBar.progressCmdBar() await cmdBar.progressCmdBar() - await cmdBar.progressCmdBar() const newCodeToFind = `revolve001 = revolve(sketch002, angle = 360, axis = X)` expect(editor.expectEditor.toContain(newCodeToFind)).toBeTruthy() @@ -4629,18 +4573,6 @@ path001 = startProfile(sketch001, at = [0, 0]) await test.step('Go through command bar flow', async () => { await toolbar.extrudeButton.click() - await cmdBar.expectState({ - stage: 'arguments', - currentArgKey: 'sketches', - currentArgValue: '', - headerArguments: { - Profiles: '', - Length: '', - }, - highlightedHeaderArg: 'Profiles', - commandName: 'Extrude', - }) - await cmdBar.progressCmdBar() await cmdBar.expectState({ stage: 'arguments', currentArgKey: 'length', @@ -4723,19 +4655,6 @@ path001 = startProfile(sketch001, at = [0, 0]) await test.step('Go through command bar flow', async () => { await toolbar.sweepButton.click() - await cmdBar.expectState({ - stage: 'arguments', - currentArgKey: 'sketches', - currentArgValue: '', - headerArguments: { - Profiles: '', - Path: '', - Sectional: '', - }, - highlightedHeaderArg: 'Profiles', - commandName: 'Sweep', - }) - await cmdBar.progressCmdBar() await cmdBar.expectState({ stage: 'arguments', currentArgKey: 'path', @@ -4820,19 +4739,6 @@ path001 = startProfile(sketch001, at = [0, 0]) await test.step('Go through command bar flow', async () => { await toolbar.closePane('code') await toolbar.revolveButton.click() - await cmdBar.expectState({ - stage: 'arguments', - currentArgKey: 'sketches', - currentArgValue: '', - headerArguments: { - Profiles: '', - AxisOrEdge: '', - Angle: '', - }, - highlightedHeaderArg: 'Profiles', - commandName: 'Revolve', - }) - await cmdBar.progressCmdBar() await cmdBar.expectState({ stage: 'arguments', currentArgKey: 'axisOrEdge', diff --git a/e2e/playwright/sketch-tests.spec.ts b/e2e/playwright/sketch-tests.spec.ts index 49e8afd83..e5c021608 100644 --- a/e2e/playwright/sketch-tests.spec.ts +++ b/e2e/playwright/sketch-tests.spec.ts @@ -1016,7 +1016,6 @@ profile001 = startProfile(sketch001, at = [${roundOff(scale * 69.6)}, ${roundOff // sketch selection should already have been made. // otherwise the cmdbar would be waiting for a selection. - await cmdBar.progressCmdBar() await cmdBar.expectState({ stage: 'arguments', currentArgKey: 'length', diff --git a/e2e/playwright/various.spec.ts b/e2e/playwright/various.spec.ts index d3c819d10..2b75774bc 100644 --- a/e2e/playwright/various.spec.ts +++ b/e2e/playwright/various.spec.ts @@ -573,7 +573,6 @@ profile001 = startProfile(sketch002, at = [-12.34, 12.34]) await expect(page.getByTestId('command-bar')).toBeVisible() await page.waitForTimeout(100) - await cmdBar.progressCmdBar() await cmdBar.progressCmdBar() await expect(page.getByText('Confirm Extrude')).toBeVisible() await cmdBar.progressCmdBar() diff --git a/src/components/CommandBar/CommandBarSelectionInput.tsx b/src/components/CommandBar/CommandBarSelectionInput.tsx index 2348cfa47..f0a2ccb1e 100644 --- a/src/components/CommandBar/CommandBarSelectionInput.tsx +++ b/src/components/CommandBar/CommandBarSelectionInput.tsx @@ -54,6 +54,7 @@ function CommandBarSelectionInput({ const inputRef = useRef(null) const commandBarState = useCommandBarState() const [hasSubmitted, setHasSubmitted] = useState(false) + const [hasClearedSelection, setHasClearedSelection] = useState(false) const selection = useSelector(arg.machineActor, selectionSelector) const selectionsByType = useMemo(() => { return getSelectionCountByType(selection) @@ -102,7 +103,7 @@ function CommandBarSelectionInput({ if (canSubmitSelection && arg.skip && argValue === undefined) { handleSubmit() } - }, [canSubmitSelection]) + }, [arg.name, canSubmitSelection]) function handleChange() { inputRef.current?.focus() @@ -139,7 +140,30 @@ function CommandBarSelectionInput({ selectionType: 'singleCodeCursor', }, }) - }, [arg.clearSelectionFirst]) + setHasClearedSelection(true) + }, [arg]) + + // Watch for outside teardowns of this component + // (such as clicking another argument in the command palette header) + // and quickly save the current selection if we can + useEffect(() => { + return () => { + const resolvedSelection: Selections | undefined = isArgRequired + ? selection + : selection || { + graphSelections: [], + otherSelections: [], + } + + if ( + !(arg.clearSelectionFirst && !hasClearedSelection) && + canSubmitSelection && + resolvedSelection + ) { + onSubmit(resolvedSelection) + } + } + }, [hasClearedSelection]) // Set selection filter if needed, and reset it when the component unmounts useEffect(() => { diff --git a/src/components/CommandBar/CommandBarSelectionMixedInput.tsx b/src/components/CommandBar/CommandBarSelectionMixedInput.tsx index 1032846ea..687c258d3 100644 --- a/src/components/CommandBar/CommandBarSelectionMixedInput.tsx +++ b/src/components/CommandBar/CommandBarSelectionMixedInput.tsx @@ -63,7 +63,7 @@ export default function CommandBarSelectionMixedInput({ setHasAutoSkipped(true) } } - }, []) + }, [arg.name]) // Set selection filter if needed, and reset it when the component unmounts useEffect(() => { @@ -71,6 +71,24 @@ export default function CommandBarSelectionMixedInput({ return () => kclManager.defaultSelectionFilter(selection) }, [arg.selectionFilter]) + // Watch for outside teardowns of this component + // (such as clicking another argument in the command palette header) + // and quickly save the current selection if we can + useEffect(() => { + return () => { + const resolvedSelection: Selections | undefined = isArgRequired + ? selection + : selection || { + graphSelections: [], + otherSelections: [], + } + + if (canSubmitSelection && resolvedSelection) { + onSubmit(resolvedSelection) + } + } + }, []) + function handleChange() { inputRef.current?.focus() }