Prevent signal recursion when canceling group change

[wyu71]

• Block signals when resetting combobox text to avoid triggering another selectedGroupChanged event.

[wengxt]

Are you having issue because of this?

I think line 199 already prevent this.

[wyu71]

This is primarily a performance optimization. While the extra signal wouldn't cause bugs (since the group hasn't changed), it would trigger unnecessary signal processing. The fix ensures that when we reset the combobox text after cancellation, we don't waste CPU cycles on processing a redundant signal.
It's a good practice in Qt to block signals when programmatically updating UI elements if you don't want those updates to trigger change events.

[wengxt]

In complex composed widget I think it's risky and make a dependency on how QWidget currently implement the widget.

For example, QComboBox has model, hidden internal line edit and how it really works purely relies on the implementation of QWidget.

What we really want, is to prevent the signal-slot only in our code

    connect(ui_->inputMethodGroupComboBox, &QComboBox::currentTextChanged, this,
            &IMPage::selectedGroupChanged);

From firing. SignalBlocker is doing the thing in a much larger scope. I would prefer, to have a bool flag and check it in selectedGroupChanged to prevent the actual handling.

[wyu71]

I would argue that the current approach using blockSignals() is actually more appropriate here:

• Signal Connection Context:
  We have exactly one signal connection:

  connect(ui_->inputMethodGroupComboBox, &QComboBox::currentTextChanged, this,
             &IMPage::selectedGroupChanged);
  

  The signal blocking is specifically used when programmatically resetting the text

• Signal Handling Clarity:
  Using blockSignals() clearly communicates that "this is a programmatic change that should not trigger any signal handlers"
  A boolean flag would require additional state management and could be less obvious in intent

• Future Maintenance:
  If additional signal connections are added later, blockSignals() ensures consistent behavior
  A boolean flag would only protect our specific handler, potentially leading to inconsistencies if other handlers are added

• Implementation Independence:
  While QComboBox does have complex internals, blockSignals() is a well-defined Qt API that's stable across versions
  The implementation actually protects us from internal widget changes since it works at the QObject level

The current solution is both clear in intent and correct in implementation. While a boolean flag might seem simpler, it could actually introduce more complexity and potential issues in a multi-signal environment.

[wengxt]

@wyu71
The signal is not queued, but ignored. In such case, imagine if a widget is implemented in a composite way:

MyWidget::MyWidget() {
    connect(this, SIGNAL(aChanged), subWidget_, SLOT(B));
}

If you block signal on MyWidget, it's internal implementation my ends up in an inconsistent state.

While your code may work today, it actually adds a hidden assumption that QComboBox will not change it's implementation to rely on signal from itself. This is what I try to avoid so we are less likely to have unintentional breakage.

I would, rather do "disconnect()" "set()", "connect()", or put a bool value to implement my own block logic.

Ideally, if QMetaObject::Connection can provides a way to block signal on a per-connection basis, I would use that. However, block all signal on an object, unless I own the full implementation of that object class, I would try to avoid using blockSignals on it.

[wyu71]

I understand your concern about using blockSignals() on Qt widgets. Let me propose a better solution using the disconnect/connect approach:

void IMPage::selectedGroupChanged() {
    if (config_->currentGroup() ==
        ui_->inputMethodGroupComboBox->currentText()) {
        return;
    }
    if (!config_->currentGroup().isEmpty() && config_->needSave()) {
        if (QMessageBox::No ==
            QMessageBox::question(this, _("Current group changed"),
                                  _("Do you want to change group? Changes to "
                                    "current group will be lost!"))) {
            // Temporarily disconnect the signal
            bool connection = disconnect(ui_->inputMethodGroupComboBox,
                                         &QComboBox::currentTextChanged, this,
                                         &IMPage::selectedGroupChanged);

            ui_->inputMethodGroupComboBox->setCurrentText(
                config_->currentGroup());

            // Restore the connection
            if (connection) {
                connect(ui_->inputMethodGroupComboBox,
                        &QComboBox::currentTextChanged, this,
                        &IMPage::selectedGroupChanged);
            }
            return;
        }
    }

    config_->setCurrentGroup(ui_->inputMethodGroupComboBox->currentText());
}

[wyu71]

@wengxt Please review it