Fix canExtrudeSelectionItem and getSelectionType for multiple selections

[max-mrgrsk]

This pr closes: #3834
It is a part of bigger project: #2606

PR Description:

This PR addresses and fixes two key issues within the selection logic in the canExtrudeSelectionItem and getSelectionType functions. The previous implementation had limitations when handling multiple selections, resulting in incorrect selection typing and behavior for mixed sets of selections:

Example:
it would return: ['extrude-wall', 1] for a selection of single edge, which is correct
but for 2 edges it would return ['other', 2], which is incorrect and expected to be ['extrude-wall', 2]

Data Flow:

1. getSelectionType uses canExtrudeSelectionItem to determine whether a selection is an extrude-wall or not, and passes the whole selection with all currently selected ranges stored in the selection.codeBasedSelections array and the index of the item that shall be checked.
2. canExtrudeSelectionItem performs a triple check of the selected item with: isSketchPipe, nodeHasClose, and nodeHasExtrude.
3. isSketchPipe does not have an index argument, only selection. It checks the selection with isSingleCursorInPipe and always returns false in the case of multiple selections. It has to be fixed.

Key Changes:

1. Fixed canExtrudeSelectionItem:

   • The issue arose due to the function calling isSketchPipe, which is only capable of handling single selections.
   • The solution was to isolate each selection when calling isSketchPipe, ensuring it works correctly for each item in the selection list. This fix allows canExtrudeSelectionItem to return the correct result for each selection, even in cases where multiple selections are made.

2. Improved getSelectionType:

   • Previously, the function only compared subsequent selections to the first one. If the first selection was of type other, any subsequent extrude-wall types would be ignored, and vice versa.
   • The fix ensures that all selections are evaluated before determining the final type. If all selections are extrudable, the result is extrude-wall. If there’s any non-extrudable selection, the type defaults to other, accurately reflecting the mixed nature of the selection set.

These changes ensure the selection logic works as expected for both single and multiple selections.

Other Findings:

1. Generalization: Currently, everything is marked as an extrude-wall, including not-extruded sketches, faces, and edges. Since it does not block me from multiple fillets and will be addressed in the PR mentioned above by @Irev-Dev - I am ignoring it.
2. nodeHasExtrude: Returns true even if an extrude does not exist. Shall I fix it in a separate PR ?

Next steps:

I am 1 step away from the multiple fillets working, only one thing left to address is:

const updatedAst = await kclManager.updateAst(modifiedAst, true, {
    focusPath: pathToFilletNode,
  })

commenting out the focusPath works:

[qa-wolf]

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!

[vercel]

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

Name	Status	Preview	Updated (UTC)
modeling-app	✅ Ready (Inspect)	Visit Preview	Sep 14, 2024 6:21pm

[franknoirot]

This looks like it'll work for the purposes of unblocking you so I support it, but I do think we should rethink the generic approach in future.

Instead of this getSelectionType and canExtrudeSectionItem stuff, I think future selection command argument configurations should pass in their own validate() functions, which inspect the raw selection to see if it is valid for that command.

[franknoirot]

This is likely a nit, but aren't you already set up to isolate the codeBasedSelections up within the canExtrudeSelectionItem function itself?

[max-mrgrsk]

nah, can extrude selection gets as an argument the whooooole list of selections and an index of the one to check. While isSketchPile is expecting only one argument - selection. Not selection+index. So it gets the whole array of selections and just fails to work with it.

[max-mrgrsk]

we should rethink the generic approach in future.
@franknoirot
agree, those are good points, probably will be covered by #3837