Design issues with QcrModifiableComboBox
QcrModifiableComboBox
inherits QComboBox class but break its design philosophy without taking care of the consequences
The constructor of QcrModifiableComboBox
executes the method ::init
...
int i = cell_->val();
if (newTags.empty()) {
newTags.append("");
i = 0;
} else if (i<0 || i>=newTags.count()) {
newTags += "";
i = newTags.count()-1;
}
setTags(newTags);
doSetValue(i);
which changes the the index value
to 0 when an empty taglist
is provided, when negative values
are detected or when selected index is outside of valid range
. This breaks the design philosophy of QComboBox:
By default, for an empty combo box or a combo box in which no current item is set, this property has a value of -1.
Source: https://doc.qt.io/qt-6/qcombobox.html
This is especially problematic since the value is only set by using doSetValue
which will change the value in QcrCell
(libqcr inheritance) but not in QCombBox
(Qt inheritance). The result is that QcrModifiableComboBox
can enter an undefined state (in libqcr side: 0 and in qt side: -1)
This is the actual cause for Steca Issue 37. There are different solutions, but the proper one (in my opinion) is to follow the spirit of Qt which uses -1 as index for an empty Combobox or one in which no entry is selected.
At the same time also adding an empty entry should be removed since this also break the design philosophy of QtComboBox.
Further more QcrModifiableComboBox::remake()
void QcrModifiableComboBox::remake()
{
if (!isVisible())
return;
...
will prevent the update of taglist
of QcrModifiableComboBox
when it is not visible. I can see the idea behind, but this is problematic as well. If a remake is triggered and the QcrModifiableComboBox
is not visible then the ComboBox is not updated and will keep its old entries.
This defeats the purpose of this gui element since the key feature of it is its ability to update the taglist which is executed by remake method. Also Steca trusts libqcr to execute remake and update the gui element. If this property is not changed in libqcr we would have to recheck the whole Steca source code (and other projects) that each gui element execute an setVisible
before a remake.
In addition: If one looks deeper:
void QcrModifiableComboBox::remake()
{
if (!isVisible())
return;
QStringList newTags = makeTags_();
int i = newTags.indexOf(currentText());
if (newTags.empty()) {
newTags.append("");
i = 0;
} else if (i<0 || i>=newTags.count()) {
newTags += "";
i = newTags.count()-1;
}
if (newTags == tags_ && i==currentIndex())
return;
spuriousCall_ = true;
setTags(newTags);
QComboBox::setCurrentIndex(i);
spuriousCall_ = false;
QcrRemakable::remake();
}
```
void QcrRemakable::remake()
{
const QWidget* w = dynamic_cast<const QWidget*>(this);
if ((w && w->isVisible()) || dynamic_cast<const QAction*>(this)) {
remake_();
}
}
The last line of QcrModifiableComboBox::remake()
will call QcrRemakable::remake();
which will take the visibility of the gui element into consideration anyway. And no: The code of QcrRemakable::remake()
does not need to be changed.
This causes at least two issues in Steca