-
Notifications
You must be signed in to change notification settings - Fork 102
[Bug] fix some UI friction from imports #6139
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| import { expect, test } from '@e2e/playwright/zoo-test' | ||
| import * as fsp from 'fs/promises' | ||
| import path from 'path' | ||
|
|
||
| test.describe('Import UI tests', () => { | ||
| test('shows toast when trying to sketch on imported face', async ({ | ||
| context, | ||
| page, | ||
| homePage, | ||
| toolbar, | ||
| scene, | ||
| editor, | ||
| }) => { | ||
| await context.folderSetupFn(async (dir) => { | ||
| const projectDir = path.join(dir, 'import-test') | ||
| await fsp.mkdir(projectDir, { recursive: true }) | ||
|
|
||
| // Create the imported file | ||
| await fsp.writeFile( | ||
| path.join(projectDir, 'toBeImported.kcl'), | ||
| `sketch001 = startSketchOn(XZ) | ||
| profile001 = startProfileAt([281.54, 305.81], sketch001) | ||
| |> angledLine([0, 123.43], %, $rectangleSegmentA001) | ||
| |> angledLine([ | ||
| segAng(rectangleSegmentA001) - 90, | ||
| 85.99 | ||
| ], %) | ||
| |> angledLine([ | ||
| segAng(rectangleSegmentA001), | ||
| -segLen(rectangleSegmentA001) | ||
| ], %) | ||
| |> line(endAbsolute = [profileStartX(%), profileStartY(%)]) | ||
| |> close() | ||
| extrude(profile001, length = 100)` | ||
| ) | ||
|
|
||
| // Create the main file that imports | ||
| await fsp.writeFile( | ||
| path.join(projectDir, 'main.kcl'), | ||
| `import "toBeImported.kcl" as importedCube | ||
|
|
||
| importedCube | ||
|
|
||
| sketch001 = startSketchOn(XZ) | ||
| profile001 = startProfileAt([-134.53, -56.17], sketch001) | ||
| |> angledLine([0, 79.05], %, $rectangleSegmentA001) | ||
| |> angledLine([ | ||
| segAng(rectangleSegmentA001) - 90, | ||
| 76.28 | ||
| ], %) | ||
| |> angledLine([ | ||
| segAng(rectangleSegmentA001), | ||
| -segLen(rectangleSegmentA001) | ||
| ], %, $seg01) | ||
| |> line(endAbsolute = [profileStartX(%), profileStartY(%)], tag = $seg02) | ||
| |> close() | ||
| extrude001 = extrude(profile001, length = 100) | ||
| sketch003 = startSketchOn(extrude001, seg02) | ||
| sketch002 = startSketchOn(extrude001, seg01)` | ||
| ) | ||
| }) | ||
|
|
||
| await homePage.openProject('import-test') | ||
| await scene.waitForExecutionDone() | ||
|
|
||
| await scene.moveCameraTo( | ||
| { | ||
| x: -114, | ||
| y: -897, | ||
| z: 475, | ||
| }, | ||
| { | ||
| x: -114, | ||
| y: -51, | ||
| z: 83, | ||
| } | ||
| ) | ||
| const [_, hoverOverNonImport] = scene.makeMouseHelpers(611, 364) | ||
| const [importedFaceClick, hoverOverImported] = scene.makeMouseHelpers( | ||
| 940, | ||
| 150 | ||
| ) | ||
|
|
||
| await test.step('check code highlight works for code define in the file being edited', async () => { | ||
| await hoverOverNonImport() | ||
| await editor.expectState({ | ||
| highlightedCode: 'startProfileAt([-134.53,-56.17],sketch001)', | ||
| diagnostics: [], | ||
| activeLines: ['import"toBeImported.kcl"asimportedCube'], | ||
| }) | ||
| }) | ||
|
|
||
| await test.step('check code does nothing when geometry is defined in an import', async () => { | ||
| await hoverOverImported() | ||
| await editor.expectState({ | ||
| highlightedCode: '', | ||
| diagnostics: [], | ||
| activeLines: ['import"toBeImported.kcl"asimportedCube'], | ||
| }) | ||
| }) | ||
|
|
||
| await test.step('check the user is warned when sketching on a imported face', async () => { | ||
| // Start sketch mode | ||
| await toolbar.startSketchPlaneSelection() | ||
|
|
||
| // Click on a face from the imported model | ||
| // await new Promise(() => {}) | ||
| await importedFaceClick() | ||
|
|
||
| // Verify toast appears with correct content | ||
| await expect(page.getByText('This face is from an import')).toBeVisible() | ||
| await expect( | ||
| page.locator('.font-mono').getByText('toBeImported.kcl') | ||
| ).toBeVisible() | ||
| await expect( | ||
| page.getByText('Please select this from the files pane to edit') | ||
| ).toBeVisible() | ||
| }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import toast from 'react-hot-toast' | ||
|
|
||
| interface SketchOnImportToastProps { | ||
| fileName: string | ||
| } | ||
|
|
||
| export function SketchOnImportToast({ fileName }: SketchOnImportToastProps) { | ||
| return ( | ||
| <div className="flex flex-col gap-2"> | ||
| <span>This face is from an import</span> | ||
| <span className="font-mono text-sm bg-gray-100 dark:bg-gray-800 px-2 py-1 rounded"> | ||
| {fileName} | ||
| </span> | ||
| <span>Please select this from the files pane to edit</span> | ||
| </div> | ||
| ) | ||
| } | ||
|
|
||
| export function showSketchOnImportToast(fileName: string) { | ||
| toast.error(<SketchOnImportToast fileName={fileName} />) | ||
| } |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||
| import { useEffect, useRef } from 'react' | ||||
|
|
||||
| import { showSketchOnImportToast } from '@src/components/SketchOnImportToast' | ||||
| import { useModelingContext } from '@src/hooks/useModelingContext' | ||||
| import { getNodeFromPath } from '@src/lang/queryAst' | ||||
| import { getNodePathFromSourceRange } from '@src/lang/queryAstNodePathUtils' | ||||
|
|
@@ -24,10 +25,12 @@ import { | |||
| sceneInfra, | ||||
| } from '@src/lib/singletons' | ||||
| import { err, reportRejection } from '@src/lib/trap' | ||||
| import { getModuleId } from '@src/lib/utils' | ||||
| import type { | ||||
| EdgeCutInfo, | ||||
| ExtrudeFacePlane, | ||||
| } from '@src/machines/modelingMachine' | ||||
| import toast from 'react-hot-toast' | ||||
|
|
||||
| export function useEngineConnectionSubscriptions() { | ||||
| const { send, context, state } = useModelingContext() | ||||
|
|
@@ -186,6 +189,29 @@ export function useEngineConnectionSubscriptions() { | |||
| faceId, | ||||
| kclManager.artifactGraph | ||||
| ) | ||||
| if (!err(extrusion)) { | ||||
| const fileIndex = getModuleId(extrusion.codeRef.range) | ||||
| if (fileIndex !== 0) { | ||||
| const importDetails = | ||||
| kclManager.execState.filenames[fileIndex] | ||||
| if (!importDetails) { | ||||
| toast.error("can't sketch on this face") | ||||
| return | ||||
| } | ||||
| if (importDetails?.type === 'Local') { | ||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to check with the kcl oracles if I should checking anything else here, there's there might one day be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's correct. I don't think anything restricts If you'd like the addition of a new case to fail modeling-app/src/lib/operations.ts Line 1126 in d270447
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||||
| const paths = importDetails.value.split('/') | ||||
| const fileName = paths[paths.length - 1] | ||||
| showSketchOnImportToast(fileName) | ||||
| } else if ( | ||||
| importDetails?.type === 'Main' || | ||||
| importDetails?.type === 'Std' | ||||
| ) { | ||||
| toast.error("can't sketch on this face") | ||||
| } else { | ||||
| const _exhaustiveCheck: never = importDetails | ||||
| } | ||||
| } | ||||
| } | ||||
|
|
||||
| if ( | ||||
| artifact?.type !== 'cap' && | ||||
|
|
||||
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.
One nitpick. We can use
isTopLevelModule()here too.