Cancel handling issue with FEM constraint dialogs

About the development of the FEM module/workbench.

Moderator: bernd

User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Cancel handling issue with FEM constraint dialogs

Post by uwestoehr »

As noticed by Bernd, when deleting otems from the list in a constraint dialog but instead of pressing OK, you press cacnel, the deletions are not cancelled.

The reason is that the rejection routine used by all dialogs is this:

Code: Select all

bool TaskDlgFemConstraint::reject()
{
    // roll back the changes
    Gui::Command::abortCommand();
    Gui::Command::doCommand(Gui::Command::Gui,"Gui.activeDocument().resetEdit()");

    return true;
}
So if you make a deletion, it cannot be restored, only edits.

In PartDesign they use therefore this routine:

Code: Select all

bool TaskDlgFeatureParameters::reject()
{
    PartDesign::Feature* feature = static_cast<PartDesign::Feature*>(vp->getObject());
    PartDesign::Body* body = PartDesign::Body::findBodyOf(feature);

    // Find out previous feature we won't be able to do it after abort
    // (at least in the body case)
    App::DocumentObject* previous = feature->getBaseObject(/* silent = */ true );

    // detach the task panel from the selection to avoid to invoke
    // eventually onAddSelection when the selection changes
    std::vector<QWidget*> subwidgets = getDialogContent();
    for (auto it : subwidgets) {
        TaskSketchBasedParameters* param = qobject_cast<TaskSketchBasedParameters*>(it);
        if (param)
            param->detachSelection();
    }
    // roll back the done things
    Gui::Command::abortCommand();
    Gui::Command::doCommand(Gui::Command::Gui,"Gui.activeDocument().resetEdit()");

    // if abort command deleted the object make the previous feature visible again
    if (!Gui::Application::Instance->getViewProvider(feature)) {
        // Make the tip or the previous feature visible again with preference to the previous one
        // TODO: ViewProvider::onDelete has the same code. May be this one is excess?
        if (previous && Gui::Application::Instance->getViewProvider(previous)) {
            Gui::Application::Instance->getViewProvider(previous)->show();
        } else if (body != NULL) {
            App::DocumentObject* tip = body->Tip.getValue();
            if (tip && Gui::Application::Instance->getViewProvider(tip)) {
                Gui::Application::Instance->getViewProvider(tip)->show();
            }
        }
    }
    return true;
}
So they restore the object as it was as before the dialog actions. This must be done for FEM too.
wmayer
Founder
Posts: 20245
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Cancel handling issue with FEM constraint dialogs

Post by wmayer »

The reason is that the rejection routine used by all dialogs is this:
I haven't checked the code but if things are not restored then the transaction mechanism probably is not used correctly.

The transaction works this way:
doc->openTransaction("Operation")
doc->addObject(...)
doc->removeObject(...)
obj->Property.setValue(...)

and at the end either you press OK and do a
doc->commitTransaction() or you press cancel and do a
doc->abortTransaction()

Now if things don't behave as expected then there are likely two reasons:
* when starting the operation no transaction was opened
* during the operation the pending transaction was already committed

For the latter there was unfortunately a major regression with realthunder's Link branch (and/or some later PRs). Many functions in PartDesign suffer from this strange behaviour that when e.g. creating a fillet, adding and removing edges and you press Cancel the fillet object won't disappear. If you click on the little arrow next to the Undo button you can see why. Because an additional Edit operation was performed you have to actively click the Undo once or twice to get rid of the feature.

Now also the transaction mechanism itself seems to have a problem issue #0004265 but I am not sure if this is the reason for the above behaviour.
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: Cancel handling issue with FEM constraint dialogs

Post by uwestoehr »

wmayer wrote: Sat Feb 22, 2020 2:15 pm Now if things don't behave as expected then there are likely two reasons:
But why is there all the additional code in TaskDlgFeatureParameters::reject()?
(I haven't touched rejection code, I only had a look why pressing Cancel after deleting items work for the PD dialogs and not for the FEM constraint dialogs.)

e.g. creating a fillet, adding and removing edges and you press Cancel the fillet object won't disappear. If you click on the little arrow next to the Undo button you can see why. Because an additional Edit operation was performed you have to actively click the Undo once or twice to get rid of the feature.
I see. However, I am not sure I am to blame or if my changes just unveiled the underlying issues. I am for example now sitting on this issue:
https://forum.freecadweb.org/viewtopic. ... 79#p371179
wmayer
Founder
Posts: 20245
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Cancel handling issue with FEM constraint dialogs

Post by wmayer »

But why is there all the additional code in TaskDlgFeatureParameters::reject()?
It restores visibility of how it was before starting the operation. So, it's something specific to PartDesign here. However, the question is whether this is still needed. In the past the transaction mechanism only considered objects from App document but later it has been extended to also handle view providers from the Gui document.
wmayer
Founder
Posts: 20245
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Cancel handling issue with FEM constraint dialogs

Post by wmayer »

This is actually another regression in v0.19. It worked fine with v0.18.

When double-clicking the feature in the tree view the function TreeWidget::mouseDoubleClickEvent() is called and inside there it instantiates an App::AutoTransaction. Now because of this the check for a pending transaction e.g. inside TaskDlgFemConstraintFixed::open() (and in all other open() functions of all FEM task panels) says that there is a pending transaction so it leaves without doing anything.

But then afterwards Document::setEdit() closes this transaction. And that's why no transaction is open when the dialog is shown.
Now, if instead of double-clicking you start the editing via context-menu then everything is fine and canceling your changes works as expected.

In the PartDesign dialogs you don't have this problem because all functions where a feature is about to be edited call the function setupTransaction() to create a transaction if needed. But IMO, this is a bad design to force module authors to always check for an open transaction and it only cures the symptoms but doesn't fix the actual problem that the logic in the core system is broken.
User avatar
uwestoehr
Veteran
Posts: 4961
Joined: Sun Jan 27, 2019 3:21 am
Location: Germany
Contact:

Re: Cancel handling issue with FEM constraint dialogs

Post by uwestoehr »

wmayer wrote: Sun Feb 23, 2020 3:20 pm In the PartDesign dialogs you don't have this problem because all functions where a feature is about to be edited call the function setupTransaction() to create a transaction if needed. But IMO, this is a bad design to force module authors to always check for an open transaction and it only cures the symptoms but doesn't fix the actual problem that the logic in the core system is broken.
Hmm, I feel a bit confused and see that my capabilities are not sufficient to resolve this. Maybe I opened Pandora's box but at least I uncovered some problems. :|

It seems that FEM lacks something PartDesign has but as it is implemented in PartDesign is not optimal. I therefore think that I cannot improve the situation but you can. So maybe the best is you implement it the right way and then we use this solution for both, PartDesign and FEM.
Or maybe Lei can help out in doing so.
realthunder wrote: .
wmayer
Founder
Posts: 20245
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Cancel handling issue with FEM constraint dialogs

Post by wmayer »

So maybe the best is you implement it the right way
The right way is a noble goal but isn't easy to achieve because there are quite different uses cases. But at least things should be consistent (i.e. double-click is supposed to do the same as the bold action from the context-menu) and as easy as possible for module authors.
realthunder
Veteran
Posts: 2190
Joined: Tue Jan 03, 2017 10:55 am

Re: Cancel handling issue with FEM constraint dialogs

Post by realthunder »

PR submitted here.
wmayer wrote: Sun Feb 23, 2020 3:20 pm When double-clicking the feature in the tree view the function TreeWidget::mouseDoubleClickEvent() is called and inside there it instantiates an App::AutoTransaction. Now because of this the check for a pending transaction e.g. inside TaskDlgFemConstraintFixed::open() (and in all other open() functions of all FEM task panels) says that there is a pending transaction so it leaves without doing anything.
Defining an AutoTransaction without name does not create any transaction. But if a name is given, like TreeWidget double click, it does sets up an application wide active transaction. The purpose of AutoTransaction is to make sure that existing (or future) transaction will be kept open until no AutoTransaction instance lives in the current call stack. User code can abort the transaction at any time, but the commission will be delayed. It has another effect to merge multiple open transaction request into one transaction, which can happen if one command invokes other commands. AutoTransaction is mainly used by Gui::Command to simplify transaction handling.

However, all of this only works with the assumption that all the operations happen below the current call stack. It does not work for modeless dialog, hence the bug. I am actually aware of this, so I have disabled AutoTransaction inside Gui::Document::setEdit(). But I have apparently missed the use case here, where the command shows a task panel for object creation without calling setEdit().
Try Assembly3 with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
realthunder
Veteran
Posts: 2190
Joined: Tue Jan 03, 2017 10:55 am

Re: Cancel handling issue with FEM constraint dialogs

Post by realthunder »

wmayer wrote: Sat Feb 22, 2020 2:15 pm Many functions in PartDesign suffer from this strange behaviour that when e.g. creating a fillet, adding and removing edges and you press Cancel the fillet object won't disappear. If you click on the little arrow next to the Undo button you can see why. Because an additional Edit operation was performed you have to actively click the Undo once or twice to get rid of the feature.
That would be a different issue. Creating multiple transactions inside a task is desirable in many cases, because the editing task may be complex, and the user may want to undo during the task. In my fork, I have this code to remember the transaction ID when entering the task, and roll back till that ID on reject. Do you think this should be the standard behavior and be added to core?
Try Assembly3 with my custom build of FreeCAD at here.
And if you'd like to show your support, you can donate through patreon, liberapay, or paypal
wmayer
Founder
Posts: 20245
Joined: Thu Feb 19, 2009 10:32 am
Contact:

Re: Cancel handling issue with FEM constraint dialogs

Post by wmayer »

realthunder wrote: Mon Feb 24, 2020 12:39 am PR submitted here.
Thanks.
realthunder wrote: Mon Feb 24, 2020 12:56 am That would be a different issue. Creating multiple transactions inside a task is desirable in many cases, because the editing task may be complex, and the user may want to undo during the task.
There might be use cases where this is useful but for the current behaviour of the dress-up dialogs I find it a bit odd that when I e.g. create a fillet, add and remove edges and then cancel the operation that the fillet is still there.
In my fork, I have this code to remember the transaction ID when entering the task, and roll back till that ID on reject. Do you think this should be the standard behavior and be added to core?
I wonder if the core system can really handle this reliably because it doesn't know what happens during the edit operation when a dialog is open. There can be sure use cases where an open transaction is committed on purpose and a new one is opened which might then be canceled. In this case the automatism must not roll back both transaction but only the pending transaction.
Post Reply