Making edits by context menu undoable

Here's the place for discussion related to coding in FreeCAD, C++ or Python. Design, interfaces and structures.
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Making edits by context menu undoable

Post by jnxd »

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.
My latest (or last) project: B-spline Construction Project.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Making edits by context menu undoable

Post by openBrain »

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.
User avatar
jnxd
Posts: 951
Joined: Mon Mar 30, 2015 2:30 pm
Contact:

Re: Making edits by context menu undoable

Post by jnxd »

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.
My latest (or last) project: B-spline Construction Project.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Making edits by context menu undoable

Post by openBrain »

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
wmayer
Founder
Posts: 20241
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Making edits by context menu undoable

Post by wmayer »

openBrain wrote: Fri Sep 17, 2021 7:47 am I'd be more than happy to have @wmayer's opinion on this.
I can confirm the observed behaviour.

Opening a transaction in startEditing(), setEdit(), open(), ... has become a bit cumbersome in v0.19 with the auto-transaction mechanism.
But if you have a look at Part features you will see that it works there and the solution is quite simple and robust.

Example PartGui::ViewProviderPrimitive::setupContextMenu:

Code: Select all


void ViewProviderPrimitive::setupContextMenu(QMenu* menu, QObject* receiver, const char* member)
{
    Gui::ActionFunction* func = new Gui::ActionFunction(menu);
    QAction* act = menu->addAction(QObject::tr("Edit %1").arg(QString::fromUtf8(getObject()->Label.getValue())));
    act->setData(QVariant((int)ViewProvider::Default));
    func->trigger(act, boost::bind(&ViewProviderPrimitive::startDefaultEditMode, this));

    ViewProviderPart::setupContextMenu(menu, receiver, member);
}


The idea is to have a private function startDefaultEditMode() in the view provider class that is connected to an added QAction. This way we can be 100% sure that function is called by clicking at the menu entry.

The implementation of the function is:

Code: Select all

void ViewProviderPrimitive::startDefaultEditMode()
{
    QString text = QObject::tr("Edit %1").arg(QString::fromUtf8(getObject()->Label.getValue()));
    Gui::Command::openCommand(text.toUtf8());

    Gui::Document* document = this->getDocument();
    if (document) {
        document->setEdit(this, ViewProvider::Default);
    }
}
We can adopt this mechanism to PD features and only need to change a couple of classes (basically those that re-implement setupContextMenu()).
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Making edits by context menu undoable

Post by openBrain »

wmayer wrote: Mon Sep 20, 2021 12:22 pm Opening a transaction in startEditing(), setEdit(), open(), ... has become a bit cumbersome in v0.19 with the auto-transaction mechanism.
I don't know exactly what "auto-transaction" is. If you have a link, I'm interested in reading more info.
But if you have a look at Part features you will see that it works there and the solution is quite simple and robust.

Example PartGui::ViewProviderPrimitive::setupContextMenu:

The idea is to have a private function startDefaultEditMode() in the view provider class that is connected to an added QAction. This way we can be 100% sure that function is called by clicking at the menu entry.

We can adopt this mechanism to PD features and only need to change a couple of classes (basically those that re-implement setupContextMenu()).
OK, I see the point. Looks good and appropriate from the concept point of view, but still makes me wonder about implementation (mainly code duplication).
Wouldn't that be better to assume that a vast majority of features that will represents "shapes" will need this 'Edit NAME' context menu that will lead to an undoable action ? Hence implement the mechanism in a common class (ViewProviderGeometryObject ?) and so only classes that eventually want to differ from this behavior will have to override 'setupContextMenu' ? Rather than duplicating mechanism & code in a lot of subclasses ?
wmayer
Founder
Posts: 20241
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Making edits by context menu undoable

Post by wmayer »

Wouldn't that be better to assume that a vast majority of features that will represents "shapes" will need this 'Edit NAME' context menu that will lead to an undoable action ? Hence implement the mechanism in a common class (ViewProviderGeometryObject ?) and so only classes that eventually want to differ from this behavior will have to override 'setupContextMenu' ? Rather than duplicating mechanism & code in a lot of subclasses ?
Yes, it should work to move the code to ViewProviderGeometryObject but needs to be verified first.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Making edits by context menu undoable

Post by openBrain »

wmayer wrote: Mon Sep 20, 2021 2:13 pm Yes, it should work to move the code to ViewProviderGeometryObject but needs to be verified first.
That's my proposal. :) If you think it's a good way, I can move the code and test if it works correctly without regression. Also I have to understand why ViewProviderSketchBased doesn't subclass ViewProviderGeometryObject and look if it can be done harmlessly. :) Will probably take me some time though.
wmayer
Founder
Posts: 20241
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Making edits by context menu undoable

Post by wmayer »

The inheritance for ViewProviderSketchBased is: PartDesignGui::ViewProvider -> PartGui::ViewProviderPart -> PartGui::ViewProviderPartExt -> Gui::ViewProviderGeometryObject
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Making edits by context menu undoable

Post by openBrain »

wmayer wrote: Mon Sep 20, 2021 2:33 pm The inheritance for ViewProviderSketchBased is: PartDesignGui::ViewProvider -> PartGui::ViewProviderPart -> PartGui::ViewProviderPartExt -> Gui::ViewProviderGeometryObject
Fortunately I have a super nice IDE to help me with the classes inheritance. :mrgreen:
So everything is fine, I'll try & test in the coming days and submit a PR when ready.
Post Reply