Sketcher: contextual right click context menu based on selection by maxwxyz · Pull Request #11884 · FreeCAD/FreeCAD

maxwxyz · 2024-01-02T19:55:06Z

fixes #11818
This PR introduces a contextual right click context menu in the Sketcher WB which shows different commands based on the selection of the user (none, one or multiple geometry elements, or constraints).
The current context menu is static and is overloaded with commands which are either greyed out or not working in Sketcher.

New:

sketcher-context-menu.mp4

Old:

maxwxyz · 2024-01-02T20:00:20Z

No selection:

Selection of a geometry (in this example one line):

Selection of constraints:

PaddleStroke · 2024-01-02T20:46:50Z

A first comment on the functionality :

'Fit all' : Wouldn't it make more sense to bring it up under 'view section'
'Draw style' : I would remove it it seems unecessary here.
'Selected associated geo' : For consistency with the other menu maybe it would be best to move it to after the toggles.

PaddleStroke

I have not finished reviewing. Tomorrow I will check the core of the feature

src/Mod/Sketcher/Gui/CommandConstraints.cpp

src/Mod/Sketcher/Gui/ViewProviderSketch.cpp

PaddleStroke · 2024-01-03T08:24:05Z

Overall very good, it seems you are learning fast and your contribution is smart. Thanks for contributing !

By the way, to ease your life, here is the code block with the modifications I offered. I have not tested so please do.


                case STATUS_NONE: {
                    int selectedEdges = 0;
                    int selectedLines = 0;
                    int selectedConics = 0;
                    int selectedPoints = 0;
                    int selectedConstraints = 0;

                    Gui::MenuItem geom;
                    geom.setCommand("Sketcher context");

                    std::vector<Gui::SelectionObject> selection = Gui::Selection().getSelectionEx(0, Sketcher::SketchObject::getClassTypeId());

                    if (selection.size() > 0) {
                        const std::vector<std::string> SubNames = selection[0].getSubNames();

                        for (auto name : SubNames) {
                            if (name.substr(0, 4) == "Edge") {
                                ++selectedEdges;

                                int geoId = std::atoi(name.substr(4, 4000).c_str()) - 1;
                                if (geoId >= 0) {
                                    const Part::Geometry* geo = getSketchObject()->getGeometry(geoId);
                                    if (isLineSegment(*geo)) {
                                        ++selectedLines;
                                    }
                                    else {
                                        ++selectedConics;
                                    }
                                }
                            }
                            else if (name.substr(0, 4) == "Vert") {
                                ++selectedPoints;
                            }
                            else if (name.substr(0, 4) == "Cons") {
                                ++selectedConstraints;
                            }
                        }

                        if (selectedEdges >= 1 && selectedPoints == 0) {
                            // Here you want to make special cases for line vs arcs vs circle and so on.
                            geom << "Sketcher_Dimension"
                                << "Sketcher_ConstrainHorVer"
                                << "Sketcher_ConstrainVertical"
                                << "Sketcher_ConstrainHorizontal";

                            if (selectedEdges > 1) {
                                geom << "Sketcher_ConstrainParallel"
                                    << "Sketcher_ConstrainPerpendicular"
                                    << "Sketcher_ConstrainTangent"
                                    << "Sketcher_ConstrainEqual";
                            }
                        }
                        else if (selectedEdges == 1 && selectedPoints >= 1) {
                            // Here you want to make special cases for line vs arcs vs circle and so on.
                            geom << "Sketcher_Dimension"
                                << "Sketcher_ConstrainCoincidentUnified"
                                << "Sketcher_ConstrainHorVer"
                                << "Sketcher_ConstrainVertical"
                                << "Sketcher_ConstrainHorizontal";
                        }
                        else if (selectedEdges == 0 && selectedPoints >= 1) {
                             geom << "Sketcher_Dimension";

                             if (selectedPoints > 1) {
                                geom << "Sketcher_ConstrainCoincidentUnified"
                                    << "Sketcher_ConstrainHorVer"
                                    << "Sketcher_ConstrainVertical"
                                    << "Sketcher_ConstrainHorizontal";
                             }
                        }
                        else if (selectedConstraints >= 1) {
                            geom << "Sketcher_SelectElementsAssociatedWithConstraints"
                                << "Sketcher_ToggleDrivingConstraint"
                                << "Sketcher_ToggleActiveConstraint"
                                << "Separator"
                                << "Std_Delete";
                        }

                        geom << "Separator"
                            << "Sketcher_ToggleConstruction"
                            << "Separator"
                            << "Sketcher_CreatePointFillet"
                            << "Sketcher_Trimming"
                            << "Sketcher_Extend"
                            << "Separator"
                            << "Sketcher_CompDimensionTools"
                            << "Sketcher_CompConstrainTools"
                            << "Sketcher_SelectConstraints"
                            << "Separator"
                            << "Std_Delete";
                    }
                    else {  //without selection
                        geom << "Sketcher_ViewSketch"
                            << "Sketcher_ViewSection"
                            << "Std_ViewFitAll"
                            << "Separator"
                            << "Sketcher_CreatePoint"
                            << "Sketcher_CreatePolyline"
                            << "Sketcher_CreateArc"
                            << "Sketcher_CreateCircle"
                            << "Sketcher_CreateRectangle"
                            << "Sketcher_CreateHexagon"
                            << "Sketcher_CreateBSpline"
                            << "Separator"
                            << "Sketcher_ToggleConstruction"
                            << "Separator"
                            << "Sketcher_CreatePointFillet"
                            << "Sketcher_Trimming"
                            << "Sketcher_Extend"
                            << "Separator"
                            << "Sketcher_External"
                            << "Separator"
                            << "Sketcher_CompDimensionTools"
                            << "Sketcher_CompConstrainTools"
                            << "Separator"
                            << "Sketcher_DeleteAllGeometry"
                            << "Sketcher_DeleteAllConstraints"
                            << "Separator"
                            << "Sketcher_LeaveSketch";
                    }

To make special cases of line vs circle, have a look at how to get the geometry from the name in sketcher_dimension tool (see line 1510 of CommandConstraints.cpp, function handleInitialSelection)

src/Mod/Sketcher/Gui/ViewProviderSketch.cpp

PaddleStroke · 2024-01-03T08:37:41Z

@maxwxyz I have just edited my previous block of code to integrate counting edges and conics. I let you adapt the menu based on these values.

PaddleStroke · 2024-01-03T08:40:57Z

If you want finer control than line vs conics, you can add more variables :

                                    if (isLineSegment(*geo)) {
                                        ++selectedLines;
                                    }
                                    else if (isCircle(*geo)) {
                                        ++selectedCircles;
                                    }
                                    ...

PaddleStroke · 2024-01-03T08:47:27Z

Also the CI macOS build is failing :

You can get more information looking into the details (try ctrl-F for error in the logs).

Having read the code, I would venture a guess that this is caused by the 'goto' as it is the only unusual thing I have seen. So if you pickup my code block push it to see if it solves the CI failure before trying to debug it.

PaddleStroke · 2024-01-03T08:54:07Z

Ok maybe the macos build failure is unrelated to your PR then.

Btw one last thing, this feature is adding a lot of lines of code to the mouseButtonPressed function. So please make a new function to put all this new code in, for example : ViewProviderSketch::generateMenuBasedOnSelection()

maxwxyz · 2024-01-03T10:30:27Z

@PaddleStroke Thanks for reviewing.

I've created a separate function and called this function from the previous spot and also replaced the weird cases where currently a different menu was called (left click on edge, hold and then right click) with the new function.
Context menu was adjusted to present valid constraints for cases if conics are in the selection and refined for other cases
Your new tools for offset and rotate were added as well when geometry is selected (translate/array has to be included when merged).

off-topic: Fillet/Trim/Extend edge does currently not work with a pre-selection. Is this already in progress or should I create an issue?

Syres916 · 2024-01-03T12:10:57Z

@maxwxyz do you actually need those additional lines in ViewProviderSketch.h ?

Syres916 · 2024-01-03T13:09:51Z

@maxwxyz it's going to be a few hours before I have more than one CPU core free as I'm doing a Git Bisect from 2021 to trace a bug. If it's still not solved then I'll compile your PR just to make sure I get the same error as being reported in the CI builds.

src/Mod/Sketcher/Gui/ViewProviderSketch.cpp

Syres916 · 2024-01-03T13:56:36Z

@Syres916 I've built successfully with conda on Windows and get no warnings or errors concerning the changed files.

There's always some 'specifics' with Linux that Windows compiling won't highlight, that's why the CI's are across platforms though I don't have a lot of confidence in the Mac one at present.

src/Mod/Sketcher/Gui/ViewProviderSketch.cpp

PaddleStroke · 2024-01-03T15:24:14Z

Please fix my last comment (and squash the change), then compile and make sure everything works as expected. Then I think it's ready to merge @chennes.

maxwxyz · 2024-01-03T16:24:35Z

@PaddleStroke @chennes done and thanks!

PaddleStroke · 2024-01-03T16:43:47Z

Ah sorry one thing I missed :

switch (Mode) {
                case STATUS_SKETCH_UseHandler:
                    // delegate to handler whether to quit or do otherwise
                    sketchHandler->pressRightButton(Base::Vector2d(x, y));
                    return true;
                case STATUS_NONE: {
                    generateContextMenu();
                    return true;
                }
                case STATUS_SELECT_Point:{
                    generateContextMenu();
                    return true;
                }
                case STATUS_SELECT_Edge: {
                    generateContextMenu();
                    return true;
                }
                case STATUS_SELECT_Cross:
                case STATUS_SELECT_Constraint: {
                    generateContextMenu();
                    return true;
                }
                case STATUS_SKETCH_DragPoint:
                case STATUS_SKETCH_DragCurve:
                case STATUS_SKETCH_DragConstraint:
                case STATUS_SKETCH_StartRubberBand:
                case STATUS_SKETCH_UseRubberBand:
                    break;
            }

Can be simplified to :

switch (Mode) {
                case STATUS_SKETCH_UseHandler:
                    // delegate to handler whether to quit or do otherwise
                    sketchHandler->pressRightButton(Base::Vector2d(x, y));
                    return true;
                case STATUS_NONE: 
                case STATUS_SELECT_Point:
                case STATUS_SELECT_Edge:
                case STATUS_SELECT_Cross:
                case STATUS_SELECT_Constraint: {
                    generateContextMenu();
                    return true;
                }
                case STATUS_SKETCH_DragPoint:
                case STATUS_SKETCH_DragCurve:
                case STATUS_SKETCH_DragConstraint:
                case STATUS_SKETCH_StartRubberBand:
                case STATUS_SKETCH_UseRubberBand:
                    break;
            }

src/Mod/Sketcher/Gui/ViewProviderSketch.cpp

maxwxyz · 2024-01-03T18:25:18Z

all done

src/Mod/Sketcher/Gui/CommandConstraints.cpp

src/Mod/Sketcher/Gui/ViewProviderSketch.cpp

chennes · 2024-01-08T03:12:35Z

src/Mod/Sketcher/Gui/ViewProviderSketch.cpp

 {
    return editCoinManager != nullptr;
 }
+void ViewProviderSketch::generateContextMenu()


I'm OK with merging this code as-is, and I know there is a LONG history in FreeCAD of methods that look like this, but IMO it should really be something like eight individual methods, so would look something like:

void ViewProviderSketch::generateContextMenu() { // I'm using structured bindings here, but you could write countSelection in a number of different ways // to achieve the same basic idea... const auto [selectedEdges, selectedLines, selectedConics, selectedPoints, selectedConstraints] = countSelection(); if (selectedEdges >= 1 && selectedPoints == 0) { setContextMenuForEdges(menu, selectedConics, selectedLines); } else if (selectedEdges == 1 && selectedPoints >= 1) { setContextMenuForOneEdgeAndPoints(menu, selectedConics, selectedPoints); } // and so forth, for each of your top-level conditionals -- the functions can be local // to the CPP file, in an anonymous namespace, since they don't need access to `this` }

@chennes shall I redo it or do it like this for future PRs?

But then you are creating a lot of functions which are cluttering VPSketch. Even if only the cpp file.
So the solution might be to create a class in another file to handle this. But it seems a bit overkill. And also Max is a new c++ contributor asking him that wouldn't be easy.
So perhaps we can keep like this for now? This function is long but only because of the menu names taking many lines not because of complexity.

I'd also say that this is fine. Every branch of the if consists basically of one statement - which is pretty long but it still is one statement. Introducing more functions into that case would be harder to maintain as every added or removed scenario will require adding or removing a function which won't be reusable anyways as it is so specific.

PaddleStroke · 2024-01-08T17:55:45Z

Congrats on a nice feature @maxwxyz

github-actions bot added the Mod: Sketcher Related to the Sketcher Workbench label Jan 2, 2024

PaddleStroke reviewed Jan 2, 2024

View reviewed changes

src/Mod/Sketcher/Gui/CommandConstraints.cpp Outdated Show resolved Hide resolved

src/Mod/Sketcher/Gui/CommandConstraints.cpp Outdated Show resolved Hide resolved

PaddleStroke reviewed Jan 3, 2024

View reviewed changes