bloguetronica / cp2130-conf

CP2130 Configurator (cp2130-conf) is an application that can be used to configure CP2130 devices, including VID, PID, as well as other descriptors. Most importantly, the application allows you to configure pin functions and states.
GNU General Public License v3.0
1 stars 1 forks source link

MainWindow::validateInput() needs refactoring #19

Closed samuelfmlourenco closed 2 years ago

samuelfmlourenco commented 2 years ago

Currently, validateInput() calls refresh() every time the content of one of the input boxes (regarding the vendor ID or the product ID) is edited. This function is implemented between lines 107 and 121 of "mainwindow.cpp":

// Checks for valid user input, enabling or disabling the combo box and the "Refresh" button, accordingly
void MainWindow::validateInput()
{
    QString vidstr = ui->lineEditVID->text();
    QString pidstr = ui->lineEditPID->text();
    if (vidstr.size() == 4 && pidstr.size() == 4) {
        vid_ = static_cast<quint16>(vidstr.toInt(nullptr, 16));
        pid_ = static_cast<quint16>(pidstr.toInt(nullptr, 16));
        ui->comboBoxDevices->setEnabled(true);
        ui->pushButtonRefresh->setEnabled(true);
    } else {
        ui->comboBoxDevices->setEnabled(false);
        ui->pushButtonRefresh->setEnabled(false);
    }
    refresh();  // This also disables the "Open" button - Note that this is the intended behavior!
}

However, refresh() is expensive, and should be only called if the input is valid. Also, note that when the input is not valid, no new values are passed to the variables "vid" or "pid", so these variables will either assume the last valid values, or zero it the input was never valid. This means that refresh() may be working with nonsensical values. Thus, validateInput() should be rewritten:

// Checks for valid user input, enabling or disabling the combo box and the "Refresh" button, accordingly
void MainWindow::validateInput()
{
    QString vidstr = ui->lineEditVID->text();
    QString pidstr = ui->lineEditPID->text();
    if (vidstr.size() == 4 && pidstr.size() == 4) {
        vid_ = static_cast<quint16>(vidstr.toInt(nullptr, 16));
        pid_ = static_cast<quint16>(pidstr.toInt(nullptr, 16));
        refresh();  // This has the "side effect" of disabling the "Open" button - Note that this is the intended behavior!
        ui->comboBoxDevices->setEnabled(true);
        ui->pushButtonRefresh->setEnabled(true);
    } else {
        ui->comboBoxDevices->setCurrentIndex(0);  // This also disables the "Open" button
        ui->comboBoxDevices->setEnabled(false);
        ui->pushButtonRefresh->setEnabled(false);
    }
}

This new form effectively disables the "Open" button when the input values are not valid, without calling refresh(). It does so by setting the combo box index to zero, just as refresh() would, although the latter does that by clearing the combo box list and rebuilding it again.

samuelfmlourenco commented 2 years ago

Code refactored in version 1.4.