[Problem] Undo/Redo for sketcher constraints doesn't work correctly

[wwmayer]

Is there an existing issue for this?

• I have searched the existing issues

Problem description

1. Create a sketch
2. Create e.g. a rectangle
3. Select a line and set a length constraint
4. Select the length constraint and move
5. Click on undo
   Result: the length constraint disappears and the redo button is greyed out

Expected result: the constraint should be moved to the old position and the redo button should become active

Full version info

OS: Ubuntu 22.04.3 LTS (XFCE/xubuntu)
Word size of FreeCAD: 64-bit
Version: 0.22.0dev.35859 +2 (Git)
Build type: debug
Branch: fix_port_occ_780
Hash: b129029745cff523bcee87b2abe85a550e0a0da8
Python 3.10.12, Qt 5.15.3, Coin 4.0.0, Vtk 7.1.1, OCC 7.5.1
Locale: German/Germany (de_DE)

Subproject(s) affected?

Sketcher

Anything else?

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[chennes]

@PaddleStroke and/or @AjinkyaDahale can one of you guys take a look?

[PaddleStroke]

So if I understand correctly you suggest that the move constraint is done inside a transaction correct? Was this already the case and got broken or is it a feature request? Just to know what I'm looking for.

[AjinkyaDahale]

@PaddleStroke the redo button going away is more concerning. Apart from that, ideally since we are providing the ability to move the dimensions around they should stay there and be undoable.

[wwmayer]

So if I understand correctly you suggest that the move constraint is done inside a transaction correct?

I don't mind. It's probably good to add a transaction because when moving a geometry also a transaction is opened.

But this is not my actual point. The bug is that if you press the undo button after moving a constraint it will be removed and it's not possible any more to restore it because the redo button is disabled.

If you create and delete a constraint without having moved it the undo/redo works as expected.

[PaddleStroke]

Ok I see, the 'Drag constraint' is not on the undo stack like it should. So doing undo undoes the previous action. At that point the 'Drag constraint' appears which breaks the redo.

• This behavior is already like this in 0.20 so it's not new.
• Drag curve works as expected
• Pressing recompute makes the 'drag constraint' appears in the undo stack. But undoing it doesn't work.

[PaddleStroke]

Ah so comparing drag curve and drag constraint I see that drag curve is doing a tryAutoRecomputeIfNotSolve(getSketchObject());. Adding that to drag curve does solve the problem as the drag constraint appears in the stack.

Now undoing this transaction still does nothing. It seems that the initial position doesn't seem to be registered anywhere.

[PaddleStroke]

I do not know well the transaction system. Can you please advise : Is that suppose to work?

                        getDocument()->openCommand(QT_TRANSLATE_NOOP("Command", "Drag Constraint"));
                        moveConstraint(id, Base::Vector2d(x, y)); //VPSketch function changing the constraint position directly
                        getDocument()->commitCommand();

Or does the command 'actions' has to be 'registered' by wrapping them in 'cmdAppObjectArgs' ? Like drag curve :

        getDocument()->openCommand(QT_TRANSLATE_NOOP("Command", "Drag Curve"));
        Gui::cmdAppObjectArgs(getObject(),
                     "movePoint(%d,%d,App.Vector(%f,%f,0),%d)",
                       drag.DragCurve, 0, vec.x, vec.y, 1);
       getDocument()->commitCommand();

[PaddleStroke]

If we need to use cmdAppObjectArgs, then I guess we need to add a python binding to the moveConstraint function right? And VPSketch does not have a ViewProviderSketchPy.cpp so I guess this needs to be implemented too?
Let me know thanks

[wwmayer]

This behavior is already like this in 0.20 so it's not new.

I don't have a v0.20 but I can confirm the bug in v0.21.

Pressing recompute makes the 'drag constraint' appears in the undo stack. But undoing it doesn't work.

Very strange behaviour. The command shows up in the undo stack but undoing it has no effect. And if a press the Recompute button before dragging the constraint then the command doesn't show up in the undo stack.

Can you please advise : Is that suppose to work?

Yes, it should work like that.

Or does the command 'actions' has to be 'registered' by wrapping them in 'cmdAppObjectArgs' ? Like drag curve :

No, that's not needed. The relevant point is that properties of objects are modified. The document then stores the old values in the transaction objects.

If we need to use cmdAppObjectArgs, then I guess we need to add a python binding to the moveConstraint function right? And VPSketch does not have a ViewProviderSketchPy.cpp so I guess this needs to be implemented too?
Let me know thanks

For a correctly working undo/redo all this is not needed.

[PaddleStroke]

Well this is strange then. Because moveConstraint is called at 2 places :

• In mouseMove
• In mouseButtonPressed when button is released.

The first thing is that the 'DragConstraint' command is created and commited only in mouseButtonPressed. So the first thing is that undoing it would undo the last call to moveConstraint. So it would virtually be invisible as the constraint moved in mouseMove just before the button release.

However I have just commented out the moveConstraint in mouseMove to test. So now the constraint does not move until you release the button. So now the constraint movement is all done during the transaction.
But undo is still not working.

[PaddleStroke]

No, that's not needed. The relevant point is that properties of objects are modified. The document then stores the old values in the transaction objects.

So what this means is that the function 'moveConstraint' is not changing the properties of the object.
Looking at moveConstraint function, it is basically doing :

    constr->LabelDistance = whatever;

So my guess is that this is not getting saved in the property at this time. So the command is not catching anything. Thoughts?
Any ideas how sketcher 'saves' the Constraint vector to properties ? Which we may need to trigger manually here

[wwmayer]

I have added #12548 to access the LabelDistance and LabelPosition members of the Constraint class via Python.
The code snippet below clearly shows that iundo/redo basically works:

obj=App.ActiveDocument.ActiveObject
obj.getLabelDistance(0)
doc=App.ActiveDocument
doc.openTransaction("Move label")
obj.setLabelDistance(0, 10)
doc.commitTransaction()

To test the code create a sketch with a single line. Add a length constraint and make sure that here isn't another constraint defined. Don't leave the edit mode and execute the code. Now you can press undo & redo and see that the label switches.

[wwmayer]

I have discovered the actual problem:
The function moveConstraint() is called for every mouse move event and internally it directly changes the LabelDistance of the constraint.
When releasing the mouse mouseButtonPressed() is called that opens the transaction and modifies the constraint. But the LabelDistance is already set to the latest value and thus the undo/redo doesn't show any effect.

[PaddleStroke]

Yes I noticed that (see above posts). But even commenting out the movement during mouse Move, the undo was not working correctly.

…

On Thu, Feb 22, 2024, 17:31 wwmayer ***@***.***> wrote: I have discovered the actual problem: The function moveConstraint() is called for every mouse move event and internally it directly changes the LabelDistance of the constraint. When releasing the mouse mouseButtonPressed() is called that opens the transaction and modifies the constraint. But the LabelDistance is already set to the latest value and thus the undo/redo doesn't show any effect. — Reply to this email directly, view it on GitHub <#12343 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEYO6MIO6XD7SRYM7PQ5CDTYU5XFFAVCNFSM6AAAAABDDSRBN2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJZHAYTQNBTGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[wwmayer]

Well, I am working on a fix. It's done in several steps:

• when the mode STATUS_SKETCH_DragConstraint is set save the current x,y position
• when the dragging is finished call moveConstraint() with the saved x,y position to restore the old values
• open the transaction and recall moveConstraint() with the current cursor position, commit the transaction
• modify moveConstraint() to not directly modify the constraint but use the setValues() method

This already works quite well but is a bit inefficient because using setValues() only needs to be done when the dragging is finished and not for every move event. Thus, I am refactoring moveConstraint() to pass a pointer to the constraint instead of its id.

[wwmayer]

#12554