[patch] missing shortcut to delete constraint items

[uwestoehr]

I wanted to add a sortcut to delete constraint items from the dialog list. In other WBs like e.g. PartDesign this is simply done by adding a shortcut to the context menu action.

Why does this patch has no effect then?:

Code: Select all

diff --git "a/src/Mod/Fem/Gui/TaskFemConstraintDisplacement.cpp" "b/src/Mod/Fem/Gui/TaskFemConstraintDisplacement.cpp"
index cfb16d9d8..6dec7a4a7 100644
--- "a/src/Mod/Fem/Gui/TaskFemConstraintDisplacement.cpp"
+++ "b/src/Mod/Fem/Gui/TaskFemConstraintDisplacement.cpp"
@@ -68,6 +68,7 @@ TaskFemConstraintDisplacement::TaskFemConstraintDisplacement(ViewProviderFemCons
     QMetaObject::connectSlotsByName(this);
 
     QAction* action = new QAction(tr("Delete"), ui->lw_references);
+    action->setShortcut(QKeySequence::Delete);
     action->connect(action, SIGNAL(triggered()), this, SLOT(onReferenceDeleted()));
     ui->lw_references->addAction(action);
     ui->lw_references->setContextMenuPolicy(Qt::ActionsContextMenu);

It seems the keypress is filtered/taken by another dialog or menu but I cannot find out if and how. Has anybody an idea?

[uwestoehr]

Why does this patch has no effect then?:

I found it but don't know how to fix it: When there is an existing selection and thus

Code: Select all

Gui::Selection().addSelection(...)

was executed, all keypresses are ignored/filtered. But of course one wants to highlight the selected item in the dialog.

[uwestoehr]

I found it but don't know how to fix it:

Thanks to Werner there is a solution.
Here is the PR that add the shortcut "Del" to the existing context menu to delete items from the listview in the dialogs: https://github.com/FreeCAD/FreeCAD/pull/3074

[wmayer]

There is quite a lot of code duplication (10x) which can actually easily be avoided. The implementation of the event() function can go to TaskFemConstraint because it is not specific to its sub-classes. The class member deleteAction can also go to TaskFemConstraint and all what this class needs is a function to create the QAction with this signature: void createDeleteAction(QWidget* parent, const char* slotFunc) or if you prefer modern C++ void createDeleteAction(QWidget* parent, std::function<void()> slotFunc)

The sub-classes then only have to call createDeleteAction() with the list widget and the slot function as arguments.

[uwestoehr]

There is quite a lot of code duplication (10x) which can actually easily be avoided. The implementation of the event() function can go to TaskFemConstraint...

Thanks for the remark. I'll do this accordingly.

[uwestoehr]

There is quite a lot of code duplication (10x) which can actually easily be avoided. The implementation of the event() function can go to TaskFemConstraint because it is not specific to its sub-classes. The class member deleteAction can also go to TaskFemConstraint and all what this class needs is a function to create the QAction with this signature: void createDeleteAction(QWidget* parent, const char* slotFunc) or if you prefer modern C++ void createDeleteAction(QWidget* parent, std::function<void()> slotFunc)

The sub-classes then only have to call createDeleteAction() with the list widget and the slot function as arguments.

I did so for the PD dialogs:
git commit ee8228e4

I could not implement your proposal to also have the slotFunc as function parameter. I got strange compiler messages and no C++ forum thread I found could help me.

I had this function declaration:

Code: Select all

createDeleteAction(QWidget* parent, std::function<void()> slotFunc)

and called it this way:

Code: Select all

createDeleteAction(ui->listWidgetReferences, onRefDeleted())

also using a pointer did not help.
So how is this done?

[wmayer]

OK. As said the traditional way is to use Qt's SIGNAL() and SLOT() macros which returns a const char*. This const char* can be used as a function argument.
An example where this is used is here:
https://github.com/FreeCAD/FreeCAD/blob ... m.cpp#L696

Code: Select all

QWidget* PropertyStringItem::createEditor(QWidget* parent, const QObject* receiver, const char* method) const
{
    ExpLineEdit *le = new ExpLineEdit(parent);
    le->setFrame(false);
    le->setReadOnly(isReadOnly());
    QObject::connect(le, SIGNAL(textChanged(const QString&)), receiver, method);
    if(isBound()) {
        le->bind(getPath());
        le->setAutoApply(autoApply());
    }
        
    return le;
}

See that "method" is directly passed to the connect() function. The function createEditor() then is called within:
https://github.com/FreeCAD/FreeCAD/blob ... e.cpp#L142

Code: Select all

editor = childItem->createEditor(parent, this, SLOT(valueChanged()));

This approach is fine but has one big drawback: you can pass any (invalid) string and the compiler cannot tell you if it's wrong. You have to test it at run-time and then Qt will print a warning if such a slot function doesn't exist.

The more modern C++ approach is to pass a function pointer. Then the compiler will tell you if the method exists.

createDeleteAction(ui->listWidgetReferences, onRefDeleted())

What you do is to call onRefDeleted() inside createDeleteAction() and its return value "void" cannot be bound to the std::function.
std::function expects a function pointer and if the passed function is a static class function or global function it will look like this:

Code: Select all

createDeleteAction(ui->listWidgetReferences, &globalRefDeleted) // see the & and the lack of ()
createDeleteAction(ui->listWidgetReferences, &MyClass::staticRefDeleted)

Now if you have a non-static function you cannot pass it the same way because it's bound to an instance of the class. So, you do it this way:

Code: Select all

createDeleteAction(ui->listWidgetReferences, std::bind(&MyClass::onRefDeleted, this))

This is a rather new feature and was added in C++11 I think. The boost library has this for many, many years and the syntax is quite similar:

Code: Select all

boost::bind(&MyClass::section, this, _1)

The boost::bind also works very well with Qt's concurrent framework (it's about multi-threading) as you can see here:
https://github.com/FreeCAD/FreeCAD/blob ... s.cpp#L276
But I am not sure if it works with Qt's connect function (I haven't tested it).

Now back to std::function. The implementation of createDeleteAction will look like this:

Code: Select all

void TaskDressUpParameters::createDeleteAction(QWidget* w, std::function<void()> delFunc)
{
    // See https://doc.qt.io/qt-5/qobject.html#connect-3 
    deleteAction = new QAction(tr("Remove"), this);
    deleteAction->setShortcut(QKeySequence::Delete);
#if QT_VERSION >= QT_VERSION_CHECK(5, 10, 0)
    // display shortcut behind the context menu entry
    deleteAction->setShortcutVisibleInContextMenu(true);
#endif
    w->addAction(deleteAction);
    w->setContextMenuPolicy(Qt::ActionsContextMenu);
    connect(deleteAction, &QAction::triggered, this, delFunc);
}

And you call it like this:

Code: Select all

createDeleteAction(ui->listWidgetReferences, std::bind(&TaskFemConstraintBearing::onReferenceDeleted, this));

[wmayer]

I did so for the PD dialogs:
git commit ee8228e4

Code: Select all

void TaskDressUpParameters::createDeleteAction(QListWidget* parentList, QWidget* parentButton)
{
    // creates a context menu, a shortcutt for it and connects it to e slot function

    deleteAction = new QAction(tr("Remove"), this);
    deleteAction->setShortcut(QKeySequence::Delete);
#if QT_VERSION >= QT_VERSION_CHECK(5, 10, 0)
    // display shortcut behind the context menu entry
    deleteAction->setShortcutVisibleInContextMenu(true);
#endif
    parentList->addAction(deleteAction);
    // if there is only one item, it cannot be deleted
    if (parentList->count() == 1) {
        deleteAction->setEnabled(false);
        deleteAction->setStatusTip(tr("There must be at least one item"));
        parentButton->setEnabled(false);
        parentButton->setToolTip(tr("There must be at least one item"));
    }
    parentList->setContextMenuPolicy(Qt::ActionsContextMenu);
}

Do not put too much logic inside this function. This method is only supposed to create a QAction and connect it with a slot. All the extras like disabling a widget should go to a separate function. This is a bit more work but it pays off in the long-term because the split functions are much more flexible and improves re-usability a lot.

[uwestoehr]

Do not put too much logic inside this function. This method is only supposed to create a QAction and connect it with a slot.

My approach was to put the action setup into one function. Since all dialogs using this function (and also possible future ones I could imagine) will have the action logic that the last item cannot be deleted, I purposely put it there. If in future it is possible to delete also the last item, then this will affect all dialogs and thus one only needs to change it there.

I do not perform the connection in this function, see my next post.

[uwestoehr]

Code: Select all

createDeleteAction(ui->listWidgetReferences, &globalRefDeleted) // see the & and the lack of ()
createDeleteAction(ui->listWidgetReferences, &MyClass::staticRefDeleted)

Now if you have a non-static function you cannot pass it the same way because it's bound to an instance of the class. So, you do it this way:

Code: Select all

createDeleteAction(ui->listWidgetReferences, std::bind(&MyClass::onRefDeleted, this))

Many thanks for this background info!
Meanwhile I found the solution with using std::bind. I wanted to implement this when I realized that in the task***.cpp files we neatly list all connection. Therefore I thought it is OK to keep the connect(xxx) command there and only outsource the action setup. Thus I would keep it as it is.