QComboBox currentIndexChanged optimization bug

[RolandHughes]

I understand the why behind how this got introduced, but it's still not correct. The bug is setCurrentIndex( 7) won't cause currentIndexChanged( 7) to be emitted if 7 is the current index. I understand this was probably assumed to be an optimization, but it's really a bug. In the business world it is very common to have dialogs and/or screens like the following.

There is some outer/higher level data driver (usually from a database or indexed file) then a series of tabbed widgets, each of which focus on some small aspect of the data store for that particular driver value. In the case of the above dialog, if one changes from COBALT to another theme, the selection combo boxes on the tabs reset to their default values. If and only if that is different from the current value, the signal gets emitted and the tabs fill up with data store information for the newly selected theme. If the tab was sitting at its default value, nothing happens. The set method was called, but someone has a check in there to only emit if the value "changed" as visible within the object.

I can understand wanting to get rid of spurious signal executions, but . . . if that is going to remain part of the signal philosophy, we need an overloaded slot.

setCurrentIndexChanged( int index, bool force=false);

Business applications that need the signal to be emitted any time setCurrentIndexChanged() is called can pass in a force value of true and bypass the "did it actually change" check blocking signal emitting.

[agserm]

It is true the currentIndexChanged SIGNAL is only emitted when the index actually changes value. This is intentional and not simply an optimization. The behavior in QComboBox matches how all other propertyChanged signals work.

By only emitting the signal when the underlying value changes we prevent a signal loop if the user wants to update a circular set of widgets each time any one of them changes. There is a good example of this in our KitchenSink demo program, in the “Widgets / Sliders” example.

If you need to simulate emitting the currentIndexChanged SIGNAL you can use code like the following when you set the index.

myCombobox->setCurrentIndex(newIndex);

if (newIndex == oldIndex) {
   emit myCombobox->currentIndexChanged(newIndex);
}

[RolandHughes]

It is true the currentIndexChanged SIGNAL is only emitted when the index actually changes value. This is intentional and not simply an optimization. The behavior in QComboBox matches how all other propertyChanged signals work.

By only emitting the signal when the underlying value changes we prevent a signal loop if the user wants to update a circular set of widgets each time any one of them changes. There is a good example of this in our KitchenSink demo program, in the “Widgets / Sliders” example.

If you need to simulate emitting the currentIndexChanged SIGNAL you can use code like the following when you set the index.

myCombobox->setCurrentIndex(newIndex);

if (newIndex == oldIndex) {
   emit myCombobox->currentIndexChanged(newIndex);
}

Which is a massive application development/maintenance burden. Every place you setCurrentIndex() or setCurrentText() you have to put the bandage

if (idx == m_ui->someCB->currentIndex())
{
    m_ui->someCB->currentIndexChanged( idx);
    m_ui->someCB->currentIndexChanged( m_ui->someCB->currentText());
    m_ui->someCB->currentTextChanged( m_ui->someCB->currentText());
}

if (txt == m_ui->someCB->currentText())
{
    m_ui->someCB->currentIndexChanged( m_ui->someCB->currentIndex());
    m_ui->someCB->currentIndexChanged( txt);
    m_ui->someCB->currentTextChanged( txt);
}

Miss even one and your application will have odd errors you can never track down. Do we also have to emit editTextChanged( txt)? Something could have connected to it as well. In a large application it is difficult to always know. You definitely cannot get away with emitting just one. Other developers will have connected to "the one they like to use."

Your justification with the KitchenSink example is an extreme edge case where as my case is an everyday occurrence in business applications and medical devices. Current API implementation is incorrect.

setCurrentIndex( int index, bool force=false);
setCurrentText( const QString &txt, bool force=false);

These are what should be in the API, not the current versions. By default it will behave as KitcheSink wants and every other application on the planet will be using

m_ui->someCB->setCurrentIndex( idx, true);
or
m_ui->someCB->setCurrentText( str, true);

so all of the signals are emitted even if the value does not change.

[jvoosten]

Which is a massive application development/maintenance burden.

I would argue that recursive / circular signal storm is a bigger headache.

Every place you setCurrentIndex() or setCurrentText() you have to put the bandage

[snip]

Then something is wrong with the initialization order of your application. I have read your problem description several times, but I can't quite make out the issue. You say it's a problem if setCurrentIndex() is set with the existing value, yet you talk about selecting a new theme, which would emit a signal. Then this remark: "If the tab was sitting at its default value, nothing happens." How can a tab sit on its default value?

[RolandHughes]

Then something is wrong with the initialization order of your application. I have read your problem description several times, but I can't quite make out the issue. You say it's a problem if setCurrentIndex() is set with the existing value, yet you talk about selecting a new theme, which would emit a signal. Then this remark: "If the tab was sitting at its default value, nothing happens." How can a tab sit on its default value?

You did not look at the pretty picture in the very first message. Thousands, possibly millions of business applications are designed this way. You have an over arching parent database table, in this case Themes. Linked to Themes in the relational (but could be hierarchical) database are supporting/component tables. In this case:

Themes
Styles
Elements
Indicators

Same thing happens with retail/wholesale inventory. You have an Item table which has a record, say "Blue Jeans" which has a Styles table possessing a fixed range of sub-key values (boot cut, low rider, relaxed fit, etc.) which has its own attributes, Colors (because blue jeans aren't exactly blue anymore), sizes, etc.

At any rate, the primary sub-keys for said table are a limited number of values enforced by reference tables. They easily fit into a combo box. The full unique key for each supporting table is the Primary key of the Theme with the primary sub-key.

Many of the records in the support tables will contain foreign keys into each other. In this particular case an Element has a Style and an Indicator has a Style. Continuing with the "Blue Jeans" example the On-Hand table will have the Primary key for Blue Jeans combined with Style, Color, and Size all as the unique key for that table just to get the on-hand and on-order quantities.

Most end-users and system architects want all of this information displayed in a tabbed widget like you see in the initial message.

The combo-box on each tab must be populated from the reference table. If it wasn't you could never add anything. The combo-box cannot know about the current value of the theme because at time of creation there isn't one. Every combo-box on every tab has a "default" or "initial" value controlled either via sort order or a flag on the record in the reference table.

Everything is fine on first display.

When the user changes from say GreyGray

to, say COBALT

The dialog tells each tab to change the master combo-box on it to "the default value."

If the combo-box on the tab is not already sitting at its "default" value, life is good. The signal fires, whatever the "default" values are for the new Theme are pulled from the database and all the fields on the tab are properly updated. This is how life should always work in the business world.

The problem is the "optimization for an edge case" which does not fire said signal the combo-box value does not change. The class does not offer a fire-the-signal-no-matter-what method call. This leads to hacks in code and needless database I/O along with screen flash/flicker as the values change twice for all cases where the value in the combo-box of the tab was not at its default value.

To hack around this every application developed for business use has to:

1. Catch the signal from the parent dialog, check the combo-box to see if it is at its default value, if so, force the signal out of it, if not, change the value naturally
2. Catch the signal from the parent dialog, force the signal out of combo-box, then let combo-box value change to default happen naturally, potentially updating the display multiple times. Critical if any other tabs have values changing based on values in another tab (foreign keys)
3. Set a class bool to false, have the target of the combo-box signal set class bool to true, and after changing the combo-box value see if the bool changed to true. If not, force the signal out of the combo-box.

I'm sure there are other hacks but these are the hacks I came up with while typing now. They are hacks. The QComboBox class should have a method, fire-no-matter-what if the "optimization for an edge case" must exist because that optimization breaks good design and leads to maintenance nightmares. If you take the bool approach to minimize database I/O you have to have a big boiler plate comment about not removing that seemingly useless thing and why it exists in every class/dialog using the combo-box in a hierarchical situation. In the business world, that is lots.

I hope this better explains things.

"Optimizations for an edge case" always paint developers into a corner. Edge cases are edge cases because they rarely happen. If they happened a lot they would be main stream normal cases.

[jvoosten]

I still do not agree that "optimization breaks good design". I think the update logic in your application is quite different from what I am used to:

The dialog tells each tab to change the master combo-box on it to "the default value."

And there, in my opinion, is the fault in your logic. You use the ComboBox as a relay for updating the rest of the GUI. To me it seems changing a theme is a big enough event it warrants its own special signal/slot handler, that tells the tab(s) to re-initialize.

The combo-box on each tab must be populated from the reference table. If it wasn't you could never add anything. The combo-box cannot know about the current value of the theme because at time of creation there isn't one. Every combo-box on every tab has a "default" or "initial" value controlled either via sort order or a flag on the record in the reference table.

What I would do, is upon change of theme, wipe the entries in the GUI and repopulate them with the correct data. Nothing is loaded until it's needed.

"Optimizations for an edge case" always paint developers into a corner.

It's a design philosphy: only signal when something changes vs. signal every time; CopperSpice chooses the former. Always signalling would paint developers in another corner. My suggestion would be to look at the GUI update strategy inside your program, not try to 'fix' the library.

[janwilmans]

I think neither choice here is an obvious winner, it depends on how you design your UI logic, as @jvoosten mentioned.
@RolandHughes if you 'have that code in many places', why not wrap a small class around it? I have encountered similar situations and decided I didn't like the changed-event, so I used the activated-event, (which fires every time, not just when the value changed) but also wrapped a small domain specific class around it, so values are written directly into a ViewModel (MVVM style).

Doesn't the activated event do exactly what you want?

[RolandHughes]

@jvoosten

I still do not agree that "optimization breaks good design". I think the update logic in your application is quite different from what I am used to:

Optimization always breaks good design. It is based on at least one, and possibly a string of, assumptions. That set of assumptions is based on "what they are used to" instead of what exists in the real world.

And there, in my opinion, is the fault in your logic. You use the ComboBox as a relay for updating the rest of the GUI. To me it seems changing a theme is a big enough event it warrants its own special signal/slot handler, that tells the tab(s) to re-initialize

The tab re-initializes by setting the combo-box to its "default" value. This isn't a flaw in the logic. It's the exact same path for when a user changes the value in the combo-box.

I take it you've never written or worked on either Income Tax filing systems or inventory management (especially retail) systems, correct?

The tabs are re-arrangable, and some will not exist for some users. Let's take the retail inventory since that will be more relatable to others. Since the 1990s and Zebra printers barcodes have been the standard. It's a global unique key in the inventory system. As such a customer service rep processing a return from a receipt has the code. They always configure their screen to start with the UPC and that fills everything else in ever other widget in. That same set of widgets is configured differently for every user, especially the customer.

----------------I want a mens white shirt short sleeved button down wrinkle free Stafford Collection

They tunnel down to a UPC based on a specific path. Each widget calls out to the enclosing parent when it gets a something_changed() signal and requests what_came_before() from the parent getting a collection of key (or as Web pages like to call them, filter) values the screen knows about. The content on the other tabs changes based on what came before.

This is how they've have worked since the days of MCBA written in COBOL using green screens and indexed files. There is a hierarchy to the data. You only need one point to start navigation. Your screen could have started with Brand (Stafford Collection) went to material type (Wrinkle free) then to shirt type (Button Down) sleeve length (short) and the next widget, color, would have only the colors available matching all of the previous keys.

What I would do, is upon change of theme, wipe the entries in the GUI and repopulate them with the correct data. Nothing is loaded until it's needed.

For some reason you aren't grasping this. Let me try again:

• Wiping the screen, even just the data fields, causes a lot of screen flicker/repaint especially on under powered systems.
• Screens that are front ends for actual database tables have to have valid values.
• Valid "wiped" values are determined by the database I/O logic not hard coded in an application. They exist in different "defaults" tables in the database and are returned when a keyed hit misses.

It's a design philosphy: only signal when something changes vs. signal every time; CopperSpice chooses the former. Always signalling would paint developers in another corner. My suggestion would be to look at the GUI update strategy inside your program, not try to 'fix' the library.

It's not a "design philosophy" it's broken. An assumption was made to optimize for an edge case. This optimization took all control away from developers. Please look at the previously posted fix.

setCurrentIndex( int index, bool force=false);
setCurrentText( const QString &txt, bool force=false);

And this really is a fix because now the API puts the developer in control. By default they can let the library continue on with its one-off optimization for KitchenSink. Those who need it to behave properly can pass the second parameter. It should be this way for every method of every class that has a "set" method and optimizes out signalling when nothing changes.

@janwilmans

I used the activated-event, (which fires every time, not just when the value changed)

According to the doc this is not true.

This signal is sent when the user chooses an item in the combobox. The index of the item is passed to any connected slots. This signal is emitted even when the selected item was already the currently selected item. If you need to invoke a slot only when the selection changes, then use the signal currentIndexChanged().

You may speak the truth and the doc may be incorrect, haven't looked. In either case, the combo-box is not being changed by the user, it is being changed by a slot in the widget which caught a somethingChanged() signal from an outer widget. That slot obtains whatever the "default" value should be from the configuration table and performs a setIndex() for the combo-box.

The issue is when you call setIndex() or setText() with the value that is current, nothing happens.

I have previously listed the three primary hacks I could think of to work around this issue. None of them are acceptable for code one wishes to maintain. Eventually a new hire will decide to "fix" code doing something like that without reading the comments or asking why it exists. We were all new hires at some point. We all made stupid mistakes "fixing" code that was a hack around a library or package bug without asking why it was there.

I do thank you for the suggestion of that signal. The doc says it is not what I want though.

[janwilmans]

The docs are correct, it doesn't fire if the program, not the user, triggers the 'set', but if that's what you want, that can be easily changed by inheriting from the widget publicly and adding a setter that does trigger the event.

Do you consider this a maintenance burden? I don't see why, because its perfectly explained by the class and there will always be 'things that don't do exactly what you need in any framework'

[RolandHughes]

@janwilmans

The docs are correct, it doesn't fire if the program, not the user, triggers the 'set', but if that's what you want, that can be easily changed by inheriting from the widget publicly and adding a setter that does trigger the event.

Which requires, basically a cut & paste of the existing code into the new method so everything else the method does also happens. Many newer developers do not fully understand how to use derived classes within the Designer so it is a ticking time bomb for anyone going that route with any production code.

Do you consider this a maintenance burden? I don't see why, because its perfectly explained by the class and there will always be 'things that don't do exactly what you need in any framework'

This was a short-sighted incredibly poor decision. It removed the ability of developers to properly design applications because they cannot control if a signal will be fired. The optimization for a one-off edge case, instead of the much more common commercial use case was flawed at best.

setCurrentIndex( int index, bool force=false);
setCurrentText( const QString &txt, bool force=false);

The above, when placed in the class, does not require any existing production applications to change and it allows developers to properly design production applications going forward.

Any other place in the library where optimizations for a one-off edge case occurred need to have the force option as well.

[janwilmans]

"incredibly poor design" is a bit harsh I think. I think depending on a UI component to send back data that you put into it is also a questionable design choice.

[RolandHughes]

@janwilmans

"incredibly poor design" is a bit harsh I think.
Not harsh really. Tools designed with a single use case in mind always suffer from incredibly poor design.

I think depending on a UI component to send back data that you put into it is also a questionable design choice.

I have no idea what you are talking about here. Nothing is sending anything back. The tab gets a somethingChanged() signal and sets the combo-box index to the database driven "default" value for that combo-box. When a user selects something in that combo-box all of the fields on the tab are updated with values from the database. Creating a separate path to do the exact same thing as a hack around this bug is ensuring that one of those paths will get out of sync.