Making edits by context menu undoable

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
jnxd
Posts: 239
Joined: Mon Mar 30, 2015 2:30 pm

Making edits by context menu undoable

Postby jnxd » Wed Sep 08, 2021 8:33 pm

So this problem stemmed from a discussion in PR #4959.

Steps to reproduce:
1) Right click on any PartDesign feature (testing needs to be done on others features). With PR #4959, will also happen with datum objects.
2) Make changes and press OK.
Expected: The undo tree has an entry of this change.
Observed: It doesn't.

The complication here is best described by @openbrain
openbrain wrote: In the past, the 'Edit OBJECT' command from the context menu was opening a command and it has been removed. This is what introduces the regression at the current state of the PR -- notice that other commands such as Pad/Pocket/... suffers this bug --.
The problem is that the 'setEdit()' command is called without possibility to know the context (typically we don't know here if it's creation time, or post-edit).
I see several possibilities :
  • Re-enable the command opening in Tree.cpp that has been disabled in the past. That would eventually create duplicates in undo stack, but it seems a better case to me and we can fix it in the future
  • Add an optional argument to 'setEdit()' that can be used at call time to give a context
  • Add another function in the ViewProvider template that would be called by the context menu item instead of 'setEdit()' directly so it would be possible to process pre-actions -- same principle as 'doubleClicked()' is called when double-clicking an item, and this last calls 'setEdit()' --
I tried to look at how straightforward it would be to implement these methods. Option 1 is clearly the simplest, but will need some duplicate removal. I couldn't find a way to make option 2 or 3 available to the context menu (though more knowledgeable people probably can).

Instead, I went with adding an extra WithCommand enum to Gui::ViewProvider::EditMode that explicitly asks to open a command when running setEdit(). The changes are stored here: https://github.com/AjinkyaDahale/FreeCA ... ntext-edit. Please let me know if anyone has an opinion on these.

Straight off the bat, it does have a possibility to interrupt the Std UserEditMode. Here, I'd suppose the default mode could be replaced with the WithCommand mode. The name of this new mode could also be better.

I'll shortly put in an issue on Mantis to track this as well [EDIT] See issue #4742.
openBrain
Posts: 6325
Joined: Fri Nov 09, 2018 5:38 pm

Re: Making edits by context menu undoable

Postby openBrain » Thu Sep 09, 2021 8:58 am

Actually I have your current PR opened for a pretty long time but not so much spare time. :)

I find the solution you propose is actually pretty risky and look like a "dirty patch", that I don't like much.
I have thought about it, and think about a solution that looks to me more elegant & efficient (but I didn't test it, so it may just be an illusion :lol:).

How about introducing something like that in 'setEdit' of the 'ViewProviderDatum' :

Code: Select all

if (! Gui::Command::hasPendingCommand()) Gui::Command::openCommand("Edit " + this->pcObject->Label.getValue())
It seems natural to me.
If an undo transaction is already in progress, the 'setEdit' will consider that the calling function already managed that.
If no undo transaction is running, it will automatically create one as there is no chance that one want to edit the object and not being to undo that.

What are your thoughts ?

EDIT : we could also envisage to introduce this in 'Gui::ViewProvider::startEditing' so it will eventually fix the bug for all objects, but have to check if there is no regression doing so.
jnxd
Posts: 239
Joined: Mon Mar 30, 2015 2:30 pm

Re: Making edits by context menu undoable

Postby jnxd » Thu Sep 16, 2021 6:03 pm

openBrain wrote: Thu Sep 09, 2021 8:58 am I find the solution you propose is actually pretty risky and look like a "dirty patch", that I don't like much.
Not going to disagree, especially since the enum is also going to be used to set the default double-click behavior.
I have thought about it, and think about a solution that looks to me more elegant & efficient (but I didn't test it, so it may just be an illusion :lol:).

How about introducing something like that in 'setEdit' of the 'ViewProviderDatum' :

Code: Select all

if (! Gui::Command::hasPendingCommand()) Gui::Command::openCommand("Edit " + this->pcObject->Label.getValue())
It seems natural to me.
If an undo transaction is already in progress, the 'setEdit' will consider that the calling function already managed that.
If no undo transaction is running, it will automatically create one as there is no chance that one want to edit the object and not being to undo that.

What are your thoughts ?

EDIT : we could also envisage to introduce this in 'Gui::ViewProvider::startEditing' so it will eventually fix the bug for all objects, but have to check if there is no regression doing so.
It sure is pretty elegant, at least as far as this setup can get.

Problem with doing that in Gui::ViewProvider::startEditing may be that not all ViewProviders bother about opening/closing commands. If that's the case, we might need a total overhaul.
openBrain
Posts: 6325
Joined: Fri Nov 09, 2018 5:38 pm

Re: Making edits by context menu undoable

Postby openBrain » Fri Sep 17, 2021 7:47 am

jnxd wrote: Thu Sep 16, 2021 6:03 pm It sure is pretty elegant, at least as far as this setup can get.

Problem with doing that in Gui::ViewProvider::startEditing may be that not all ViewProviders bother about opening/closing commands. If that's the case, we might need a total overhaul.
Actually I see 3 options (my preferred is 3).

1) Manage things locally at ViewProviderDatum::setEdit ; this would solve the case of datum objects, but same thing as to be ported to all objects that suffer the same issue (Pad, Pocket, ...)
2) Manage things globally at ViewProvider::startEditing ; assuming that all objects that have an editing command should provide an undoable action. Indeed this looks to me a bit too much
3) Manage things at intermediate level ; as ViewProvider::startEditing is a virtual function, we can override it at an appropriate level. The best level to me ATM seems to be ViewProvider::GeometryObject (I assume that editing a geometry object should be undoable). In this case that would be good that ViewProviderSketchBased inherits ViewProviderGeometryObject. I don't know though why it's not the case today, and what could be the impacts.

I'd be more than happy to have @wmayer's opinion on this.
wmayer wrote: Ping -- When you have some spare time