Skip to content
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

Move length and named value constraint flows into command palette #4675

Merged
merged 27 commits into from
Dec 9, 2024

Conversation

franknoirot
Copy link
Collaborator

@franknoirot franknoirot commented Dec 5, 2024

Closes #4362 by moving the constraint that is invoked when the user clicks the sketch segment overlays into the command palette, providing a value input that wasn't present in the previous modal layout.

Also pulls the Length constraint into the command palette as a proof of concept for moving all the rest of them into a similar flow. The Angle constraint proved pretty tricky to migrate so I decided to leave that for a future PR.

Demo comparing old and new user flows

Screenshare.-.2024-12-05.6_16_25.PM-compressed.mp4

Copy link

qa-wolf bot commented Dec 5, 2024

QA Wolf here! As you write new code it's important that your test coverage is keeping up.
Click here to request test coverage for this PR!

Copy link

vercel bot commented Dec 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview Dec 9, 2024 9:40pm

Copy link
Collaborator Author

@franknoirot franknoirot left a comment

Choose a reason for hiding this comment

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

Probably worth reviewing in reverse order, the stuff at the bottom is the core of the change (modelingMachine, commandTypes, and modelingCommandConfig in particular) and everything further up is changed as a consequence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I only updated existing constraints so let me know if you think I should add new ones, I thought my changes broke enough that it feels well-tested

@@ -524,7 +533,7 @@ part002 = startSketchOn('XZ')
})
}
})
test.describe('Test Angle/Length constraint single selection', () => {
test.describe('Test Angle constraint single selection', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I broke out these tests because I didn't change the Angle constraint's behavior, it still opens the old modal. There are some tricky parts of it I want to break out into a smaller PR.

() =>
arg.defaultValue
? arg.defaultValue instanceof Function
? arg.defaultValue(commandBarState.context, argMachineContext)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

arg.defaultValue can now be a function, everything else here kinda spins out from that

@@ -1043,38 +1052,85 @@ export const ModelingMachineProvider = ({
}
}
),
'Get convert to variable info': fromPromise(
'Apply named value constraint': fromPromise(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've had beef with calling them "variables" in our codebase because they don't change, but lmk if I should use that naming and I can make it Apply named variable constraint and the like everywhere

Copy link
Contributor

Choose a reason for hiding this comment

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

and all variables are named by their very being.

I think the churn isn't worth the bikeshed

pathToNodeMap,
}
}

export async function applyConstraintAngleLength({
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the old version of the function that handled both the angle and the length constraints. I think it makes sense to keep these separate as they do considerably different code mods within.

if (err(angleLength)) return KCL_DEFAULT_LENGTH
const { transforms } = angleLength

// QUESTION: is it okay to reference kclManager here? will its state be up to date?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe this will be okay since kclManager.ast is a getter, but wanted to confirm

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

KCL-type arguments can now provide a variable name and ask for it to be included by default, and can set their default value via a function rather than a raw string.

Copy link
Collaborator

@Irev-Dev Irev-Dev left a comment

Choose a reason for hiding this comment

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

lgtm

Comment on lines +1114 to +1118
// This corrects that error, but TODO we should fix this upstream
// to avoid this kind of error in the future.
astAfterReplacement.pathToReplaced[1][0] =
(astAfterReplacement.pathToReplaced[1][0] as number) - 1
result = astAfterReplacement
Copy link
Collaborator

@Irev-Dev Irev-Dev Dec 9, 2024

Choose a reason for hiding this comment

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

A pattern I'v been using on my branch is have the insertion index returned, where -1 means nothing was inserted, and then having a util function that updates the node path
https://github.com/KittyCAD/modeling-app/pull/4705/files#diff-5b218f67cf51a04c3cff87a21c3b0d4988c95b52ad21a7c28b8a3fba4344ec68R12

Not super applicable since this is an upstream problem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's interesting, thanks!

Copy link
Contributor

@lf94 lf94 left a comment

Choose a reason for hiding this comment

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

Great job :) IMO nothing blocking. Approved

@@ -1043,38 +1052,85 @@ export const ModelingMachineProvider = ({
}
}
),
'Get convert to variable info': fromPromise(
'Apply named value constraint': fromPromise(
Copy link
Contributor

Choose a reason for hiding this comment

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

and all variables are named by their very being.

I think the churn isn't worth the bikeshed

src/components/ModelingMachineProvider.tsx Outdated Show resolved Hide resolved
Comment on lines +1116 to +1117
astAfterReplacement.pathToReplaced[1][0] =
(astAfterReplacement.pathToReplaced[1][0] as number) - 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Will [1][0] access guarantee to succeed forever?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not think so, this line of code makes me very nervous, but only as much as the other pathToNode-diving code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

every path to node starts with

[
  ['body', '']
  [5, 'index']
]

Because it's accessing the body of the program, and then an expression in that body, you could put this accessing inside of a util and do a couple other checks, but [1][0] should always be the expression index.

@lf94
Copy link
Contributor

lf94 commented Dec 9, 2024

Minor bug, but the text input stays as a pointer:

out

@lf94
Copy link
Contributor

lf94 commented Dec 9, 2024

Tried my best to break the input - works very well :).

@lf94
Copy link
Contributor

lf94 commented Dec 9, 2024

I spoke too soon lol:

out

@lf94
Copy link
Contributor

lf94 commented Dec 9, 2024

Oh the text input can go right off the dialog:

out

nrc and others added 8 commits December 9, 2024 16:16
* Invalidate nightly bucket files after publish

* Fix conflict resolution
* Add installation instructions for all platforms
Fixes #4511

* Typo

* Typo2

* Improve linux instructions, thanks @TomPridham

Co-authored-by: Tom Pridham <[email protected]>

---------

Co-authored-by: Tom Pridham <[email protected]>
* WIP: experimenting with Loft UI
Relates to #4470

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Add selection guard

* Working loft for two sketches in the right hardcoded order

* First pass at handling more than 2 sketches

* WIP selections

* WIP selections

* More checks

* Appends the loft line after the 'last' sketch in the code

* Clean up

* Enable multiple selections after the button click

* First point-click loft test (not working locally, loft gets inserted at the wrong place)

* Lint

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* Clean up and working pw test

* Add test for doesSceneHaveSweepableSketch with count = 2

* Clean up loftSketches function

* Add pw test for preselected sketches

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Trigger CI

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Move to fromPromise-based Actor

* Move error logic out of loftSketches, fix pw tests

* Remove comments

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Trigger CI

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Trigger CI

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Fix typo

* Revert snapshots

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Trigger CI

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Trigger CI

* WIP: initial shell code addition

* Rollback pw values to pre cam change

* WIP: more additions

* WIP: closer

* WIP: first time working shell mod

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* Add extrude lookup for more generic shell

* Handle walls

* Add pw tests for cap shell

* Add shell wall test

* Fix lint

* Add selection guard and clean up

* Lint fix

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* WIP mutliple faces

* WIP circular dep

* Lint

* Look at this (photo)Graph *in the voice of Nickelback*

* Trigger CI

* Working multi-face shell across types

* Cap and wall pw test

* Apply suggestions from Frank's review

Co-authored-by: Frank Noirot <[email protected]>

* Fix test annotations

* Add unit tests for doesSceneHaveExtrudedSketch

* Manual resolution of snapshot conflicts

* Fix assertParse

* Updated pathToNode construct

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Frank Noirot <[email protected]>
* move around the files for cache to better localtions

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* cleanup

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* udpates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* cleanup

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* ensure we can change the grid setting via the command bar

Signed-off-by: Jess Frazelle <[email protected]>

* pass thru all setttings

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* fix playwright test

Signed-off-by: Jess Frazelle <[email protected]>

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* emoty

---------

Signed-off-by: Jess Frazelle <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@franknoirot
Copy link
Collaborator Author

Oh the text input can go right off the dialog:

out

Haha great find! I'll fix that in another bit of work: #4719

@franknoirot franknoirot enabled auto-merge (squash) December 9, 2024 21:25
Copy link
Collaborator

@pierremtb pierremtb left a comment

Choose a reason for hiding this comment

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

LGTM 🥳

Comment on lines +1077 to +1079
if (!sketchDetails) {
return Promise.reject(new Error('No sketch details'))
const { variableName } = await getVarNameModal({
valueName: data?.variableName || 'var',
})
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

[comment] Very much prefer this curly style that you have here over one or two liners for blocks like we have elsewhere. Find it easier to maintain. Would love to add this type of rule to our linter. But not today :)

)
pResult = parse(recast(_modifiedAst))
return Promise.reject(parseResultAfterInsertion)
Copy link
Collaborator

Choose a reason for hiding this comment

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

[note to self] I need to dive more into what the promise rejections do here

@franknoirot franknoirot merged commit b15aac9 into main Dec 9, 2024
26 checks passed
@franknoirot franknoirot deleted the franknoirot/4362/constraint-through-cmd-bar branch December 9, 2024 21:44
lf94 pushed a commit that referenced this pull request Dec 11, 2024
…#4675)

* Extend KCL argument input

* Migrate length constraint to be a command

* Add ability for `kcl` arguments to provide an initial variable name

* Move named variable flow into command palette

* Fix one e2e test

* Remove unwanted `ZERO` behavior when length constraint has no `variableName`

* Fix issue with `getSelectionCountByType` with sketches not yet in artifactGraph

* Update broken constraint tests

* Look at this (photo)Graph *in the voice of Nickelback*

* Fix segment overlays tests, which had out-of-date selectors

* Return early from `useConvertToVariable` if no selectionRanges

* Fixup for review comment from #4677 (#4696)

Signed-off-by: Nick Cameron <[email protected]>

* Invalidate nightly bucket files after publish (#4627)

* Invalidate nightly bucket files after publish

* Fix conflict resolution

* Add some more warnings (#4697)

* Add installation instructions for all platforms (#4592)

* Add installation instructions for all platforms
Fixes #4511

* Typo

* Typo2

* Improve linux instructions, thanks @TomPridham

Co-authored-by: Tom Pridham <[email protected]>

---------

Co-authored-by: Tom Pridham <[email protected]>

* Bump node to v22.12.0 (LTS) (#4706)

* Point-and-click Shell (#4666)

* WIP: experimenting with Loft UI
Relates to #4470

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Add selection guard

* Working loft for two sketches in the right hardcoded order

* First pass at handling more than 2 sketches

* WIP selections

* WIP selections

* More checks

* Appends the loft line after the 'last' sketch in the code

* Clean up

* Enable multiple selections after the button click

* First point-click loft test (not working locally, loft gets inserted at the wrong place)

* Lint

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* Clean up and working pw test

* Add test for doesSceneHaveSweepableSketch with count = 2

* Clean up loftSketches function

* Add pw test for preselected sketches

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Trigger CI

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Move to fromPromise-based Actor

* Move error logic out of loftSketches, fix pw tests

* Remove comments

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Trigger CI

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Trigger CI

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Fix typo

* Revert snapshots

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Trigger CI

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* Trigger CI

* WIP: initial shell code addition

* Rollback pw values to pre cam change

* WIP: more additions

* WIP: closer

* WIP: first time working shell mod

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* Add extrude lookup for more generic shell

* Handle walls

* Add pw tests for cap shell

* Add shell wall test

* Fix lint

* Add selection guard and clean up

* Lint fix

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* WIP mutliple faces

* WIP circular dep

* Lint

* Look at this (photo)Graph *in the voice of Nickelback*

* Trigger CI

* Working multi-face shell across types

* Cap and wall pw test

* Apply suggestions from Frank's review

Co-authored-by: Frank Noirot <[email protected]>

* Fix test annotations

* Add unit tests for doesSceneHaveExtrudedSketch

* Manual resolution of snapshot conflicts

* Fix assertParse

* Updated pathToNode construct

---------

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Frank Noirot <[email protected]>

* More aggressive using of cache on engine settings changes (#4691)

* move around the files for cache to better localtions

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* cleanup

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* udpates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* cleanup

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* ensure we can change the grid setting via the command bar

Signed-off-by: Jess Frazelle <[email protected]>

* pass thru all setttings

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* updates

Signed-off-by: Jess Frazelle <[email protected]>

* fix playwright test

Signed-off-by: Jess Frazelle <[email protected]>

* A snapshot a day keeps the bugs away! 📷🐛 (OS: ubuntu-latest-8-cores)

* A snapshot a day keeps the bugs away! 📷🐛 (OS: windows-latest-8-cores)

* emoty

---------

Signed-off-by: Jess Frazelle <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* fix use of `as`

---------

Signed-off-by: Nick Cameron <[email protected]>
Signed-off-by: Jess Frazelle <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Nick Cameron <[email protected]>
Co-authored-by: Pierre Jacquier <[email protected]>
Co-authored-by: Tom Pridham <[email protected]>
Co-authored-by: Jess Frazelle <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need to allow users to edit a value while constraining it via tooltip menu items
6 participants