Solved Neatest way to avoid re-entrant selectionChanged signal
-
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 (viaQListWidgetItem::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 theallItem
(otoh if any other item is selected theallItem
is automatically deselected). In itself, fair enough, the problem is that thelw.clearSelection()
, and more specifically the followingallItem.setSelected(True)
, will cause the slot to be re-entered, because (unfortunately for me) callingsetSelected()
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)
beforesetSelected()
and re-attach viaQObject::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?
-
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 -
P.S.
Reentrancy has nothing to do with the topicYou 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.... -
@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 noselectionChanged
signal is emitted
- The new selection coming from
- 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
causesselectionChanged
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 causeselectionChanged
to fire again but, as above,selected
will be empty and the first if takes care of stopping
- Nothing is selected, you select "All":
-
@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.