Neatest way to avoid re-entrant selectionChanged signal

JonB

Code I have inherited has a multi-selection list widget which has to handle receiving QListWidget::selectionChanged() (https://doc.qt.io/qt-5/qlistview.html#selectionChanged) signal, where if clicked on a certain item will cause other items to be deselected (via QListWidgetItem::setSelected(bool select) (https://doc.qt.io/qt-5/qlistwidgetitem.html#setSelected)). This will cause re-entrant call of slot, which must be dealt with. The way current code does that is IMHO horrible, so I'd like to ask how others would handle it.

My code is Python/PyQt. However, don't be frightened, the issue would be just the same in C++, and I'm sure you can figure how the Python corresponds to C++. I am happy to accept proposed solutions in C++ (provided they can be turned into Python). With that in mind, here is what the code reads at present, with an "English" explanation of what's going on below.

The list has a special first item, allItem, which when clicked on indicates that "all items are wanted":

All Items
Item #1
Item #2
...

It does not select all the other items; instead it deselects all other items, leaving the "allItem" as the sole selection, which code will act on appropriately.

Slot definition:

@staticmethod
def allItemSelection(lw: QListWidget, selected: QItemSelection, allItem: QListWidgetItem, connectedLambda):
    if selected.contains(lw.indexFromItem(allItem)):
        # If 'all' item just selected, deselect everything else
        lw.selectionModel().selectionChanged.disconnect(connectedLambda)
        lw.clearSelection()
        allItem.setSelected(True)
        lw.selectionModel().selectionChanged.connect(connectedLambda)
    else:
        # else deselect 'all' item
        allItem.setSelected(False)

Caller's signal connection:

connectedLambda = lambda selection: self.allItemSelection(self.listWidget, selection, self.allItem, connectedLambda)
self.listWidget.selectionModel().selectionChanged.connect(connectedLambda)

The essential of the slot definition is that if the changed selection contains the allItem we clear all existing selected items and re-select just the allItem (otoh if any other item is selected the allItem is automatically deselected). In itself, fair enough, the problem is that the lw.clearSelection(), and more specifically the following allItem.setSelected(True), will cause the slot to be re-entered, because (unfortunately for me) calling setSelected() from code will raise the signal anew.

The existing code works by:

The slot is itself a lambda (because of the parameters needing to be passed to it).
The caller connects the lambda, passing it the lambda itself as one of the arguments (the slot is used shared by other lists with the same behaviour), so that...
The slot prevents re-entrancy by QObject::disconnect(lambdaArgument) before setSelected() and re-attach via QObject::connect(lambdaArgument) afterwards.

To me this is horrible! I don't like storing the lambda in a variable and passing it around, and I don't really like the whole idea of disconnecting the slot and reconnecting it anyway as the way to deal with this.

So... is there another/neater way to accomplish what is needed here?

VRonin

I think you are kinda of overcomplicating the problem... selectionChanged already has all the information it needs not to continuously recurse. see the example below:

#include <QApplication>
#include <QListWidget>
#include <QDebug>
#include <QItemSelection>
#include <QItemSelectionModel>

int main(int argc, char ** argv)
{
    QApplication app(argc,argv);
    QListWidget mainWid;
    mainWid.addItem("All");
    mainWid.setSelectionMode(QAbstractItemView::MultiSelection);
    for(int i=1;i<6;++i)
        mainWid.addItem("Item "+ QString::number(i));
    const QModelIndex allIdx = mainWid.model()->index(0,0);
    QObject::connect(mainWid.selectionModel(),&QItemSelectionModel::selectionChanged,&mainWid,[&mainWid,&allIdx](const QItemSelection &selected){
        if(selected.isEmpty())
            return;
        if(selected.contains(allIdx)){
// if the changed selection contains the allItem we clear all existing selected items and re-select just the allItem
            mainWid.selectionModel()->select(allIdx,QItemSelectionModel::ClearAndSelect);
            return;
        }
        if(mainWid.selectionModel()->isSelected(allIdx))
//otoh if any other item is selected the allItem is automatically deselected
            mainWid.selectionModel()->select(allIdx,QItemSelectionModel::Deselect);
    });
    mainWid.show();
    return app.exec();
}

P.S.
Reentrancy has nothing to do with the topic

JonB

@VRonin

P.S.
Reentrancy has nothing to do with the topic

You may have picked a definition of "re-entrancy" which is specifically to do with threads, that's not what I meant! I am talking about: a signal calls my slot, and my slot is in danger of doing something which will cause a new signal calling the same slot to be re-raised. That may then cause unending recursion. I don't really mind what terminology we use!

I will now look at your code and try implementing. I didn't know about QItemSelectionModel::ClearAndSelect. But doesn't doing that cause a new "selection changed" signal to be raised from my slot, which will call my slot again? I'll have to see, I guess, using the debugger....

VRonin

@JonB said in Neatest way to avoid re-entrant selectionChanged signal:

But doesn't doing that cause a new "selection changed" signal to be raised from my slot, which will call my slot again?

You have 5 cases:

Nothing is selected, you select "All":
- The new selection coming from ClearAndSelect is identical to the current one so no selectionChanged signal is emitted
Nothing is selected, you select an item:
An item is selected and you select another item:
- All the conditions in the ifs are false and nothing happens in the lambda
An item is selected and you select "All":
- ClearAndSelect causes selectionChanged to fire again but since you are not selecting any new index ("All" was already selected) selected will be empty and the first if takes care of stopping
"All" is selected and you select another item:
- Deselect will cause selectionChanged to fire again but, as above, selected will be empty and the first if takes care of stopping

JonB

@VRonin
Only 5? ;-)

Yep, thanks. Now changed over. Each click now still causes the slot to be called, and from within the slot the new selection calls it to be called recursively a second time. But as you say, the logic means that the recursive call will do nothing further. This still "concerns" me, in case someone in future changes what the code does without realising the recursive implications and hangs/crashes the app, but that's programming for you!

At any rate, it seems much nicer to me than passing lambdas around and disconnecting signals, I trust you feel the same, so thanks.