Skip to content

Fix adapter device ID not incrementing when adding new devices #32

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jgeudens wants to merge 1 commit into
base: master
Choose a base branch
from
+62 −1
Open

Fix adapter device ID not incrementing when adding new devices #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/customwidgets/deviceconfigtab.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,18 @@ void DeviceConfigTab::onAdapterChanged(int index)
{
defaultValues = defaultDevices.first().toObject();
}

// Preserve the existing device id so that switching adapters does not overwrite
// the unique id assigned when the tab was created.
if (_pSchemaForm)
{
const int currentId = _pSchemaForm->values().value("id").toInt(-1);
if (currentId >= 0)
{
defaultValues["id"] = currentId;
}
}

rebuildSchemaForm(newAdapterId, defaultValues);
}

Expand Down
10 changes: 10 additions & 0 deletions src/dialogs/adapterdevicesettings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,12 @@ AdapterDeviceSettings::AdapterDeviceSettings(SettingsModel* pSettingsModel, QWid
}
}

/*! \brief Add a new device tab with a unique, auto-incremented device ID.
*
* Creates a SettingsModel device via addNewDevice() to obtain a unique ID,
* sets its adapter, then opens a new DeviceConfigTab pre-populated with
* the adapter's default values and the assigned ID.
*/
void AdapterDeviceSettings::handleAddTab()
{
QString defaultAdapterId;
Expand Down Expand Up @@ -90,6 +96,10 @@ void AdapterDeviceSettings::handleAddTab()
defaultValues = defaultDevices.first().toObject();
}

deviceId_t newId = _pSettingsModel->addNewDevice();
_pSettingsModel->deviceSettings(newId)->setAdapterId(defaultAdapterId);
defaultValues["id"] = static_cast<int>(newId);

int tabIndex = _pDeviceTabs->count() + 1;
auto* tab = new DeviceConfigTab(_pSettingsModel, defaultAdapterId, defaultValues, _pDeviceTabs);
_pDeviceTabs->addNewTab(constructTabName(defaultValues, tabIndex), tab);
Expand Down
40 changes: 39 additions & 1 deletion tests/dialogs/tst_adapterdevicesettings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,45 @@ void TestAdapterDeviceSettings::addTabUsesDeviceDefaults()

auto* spin = tab->findChild<QSpinBox*>();
QVERIFY(spin != nullptr);
QCOMPARE(spin->value(), 5);
// The adapter default id (5) must be overridden with a unique SettingsModel id.
const int assignedId = spin->value();
QVERIFY(assignedId != 5);
QVERIFY(model.hasDevice(static_cast<deviceId_t>(assignedId)));
QCOMPARE(model.deviceSettings(static_cast<deviceId_t>(assignedId))->adapterId(), QStringLiteral("adapterA"));
}

void TestAdapterDeviceSettings::addTabIncrementsDeviceId()
{
SettingsModel model;
setupAdapter(model, "adapterA", QJsonArray());

AdapterDeviceSettings w(&model);

auto* tabs = w.findChild<AddableTabWidget*>();
QVERIFY(tabs != nullptr);
QCOMPARE(tabs->count(), 0);

emit tabs->addTabRequested();
emit tabs->addTabRequested();

QCOMPARE(tabs->count(), 2);

auto* tab0 = qobject_cast<DeviceConfigTab*>(tabs->tabContent(0));
auto* tab1 = qobject_cast<DeviceConfigTab*>(tabs->tabContent(1));
QVERIFY(tab0 != nullptr);
QVERIFY(tab1 != nullptr);

const int id0 = tab0->values().value("id").toInt(-1);
const int id1 = tab1->values().value("id").toInt(-1);

QVERIFY(id0 >= 1);
QVERIFY(id1 > id0);

// Both ids must be present in the SettingsModel and linked to the adapter.
QVERIFY(model.hasDevice(static_cast<deviceId_t>(id0)));
QVERIFY(model.hasDevice(static_cast<deviceId_t>(id1)));
QCOMPARE(model.deviceSettings(static_cast<deviceId_t>(id0))->adapterId(), QStringLiteral("adapterA"));
QCOMPARE(model.deviceSettings(static_cast<deviceId_t>(id1))->adapterId(), QStringLiteral("adapterA"));
Comment on lines +239 to +270
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Please add a regression test for adapter-switch ID preservation.

This new test covers ID allocation on add, but it does not exercise the second fix path in DeviceConfigTab::onAdapterChanged (ID must remain unchanged when switching adapter).

Suggested test shape 
+void TestAdapterDeviceSettings::adapterSwitchKeepsExistingDeviceId()
+{
+    SettingsModel model;
+    setupAdapter(model, "adapterA", QJsonArray());
+    setupAdapter(model, "adapterB", QJsonArray());
+
+    AdapterDeviceSettings w(&model);
+    auto* tabs = w.findChild<AddableTabWidget*>();
+    QVERIFY(tabs != nullptr);
+    emit tabs->addTabRequested();
+
+    auto* tab = qobject_cast<DeviceConfigTab*>(tabs->tabContent(0));
+    QVERIFY(tab != nullptr);
+
+    const int before = tab->values().value("id").toInt(-1);
+    QVERIFY(before >= 1);
+
+    auto* combo = tab->findChild<QComboBox*>();
+    QVERIFY(combo != nullptr);
+    const int newIndex = combo->findData(QStringLiteral("adapterB"));
+    QVERIFY(newIndex >= 0);
+    combo->setCurrentIndex(newIndex);
+
+    const int after = tab->values().value("id").toInt(-1);
+    QCOMPARE(after, before);
+}

As per coding guidelines, "tests/**/*.cpp: When fixing a bug, add a test that reproduces the issue before implementing the fix".

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
void TestAdapterDeviceSettings::addTabIncrementsDeviceId()
{
SettingsModel model;
setupAdapter(model, "adapterA", QJsonArray());
AdapterDeviceSettings w(&model);
auto* tabs = w.findChild<AddableTabWidget*>();
QVERIFY(tabs != nullptr);
QCOMPARE(tabs->count(), 0);
emit tabs->addTabRequested();
emit tabs->addTabRequested();
QCOMPARE(tabs->count(), 2);
auto* tab0 = qobject_cast<DeviceConfigTab*>(tabs->tabContent(0));
auto* tab1 = qobject_cast<DeviceConfigTab*>(tabs->tabContent(1));
QVERIFY(tab0 != nullptr);
QVERIFY(tab1 != nullptr);
const int id0 = tab0->values().value("id").toInt(-1);
const int id1 = tab1->values().value("id").toInt(-1);
QVERIFY(id0 >= 1);
QVERIFY(id1 > id0);
// Both ids must be present in the SettingsModel and linked to the adapter.
QVERIFY(model.hasDevice(static_cast<deviceId_t>(id0)));
QVERIFY(model.hasDevice(static_cast<deviceId_t>(id1)));
QCOMPARE(model.deviceSettings(static_cast<deviceId_t>(id0))->adapterId(), QStringLiteral("adapterA"));
QCOMPARE(model.deviceSettings(static_cast<deviceId_t>(id1))->adapterId(), QStringLiteral("adapterA"));
void TestAdapterDeviceSettings::addTabIncrementsDeviceId()
{
SettingsModel model;
setupAdapter(model, "adapterA", QJsonArray());
AdapterDeviceSettings w(&model);
auto* tabs = w.findChild<AddableTabWidget*>();
QVERIFY(tabs != nullptr);
QCOMPARE(tabs->count(), 0);
emit tabs->addTabRequested();
emit tabs->addTabRequested();
QCOMPARE(tabs->count(), 2);
auto* tab0 = qobject_cast<DeviceConfigTab*>(tabs->tabContent(0));
auto* tab1 = qobject_cast<DeviceConfigTab*>(tabs->tabContent(1));
QVERIFY(tab0 != nullptr);
QVERIFY(tab1 != nullptr);
const int id0 = tab0->values().value("id").toInt(-1);
const int id1 = tab1->values().value("id").toInt(-1);
QVERIFY(id0 >= 1);
QVERIFY(id1 > id0);
// Both ids must be present in the SettingsModel and linked to the adapter.
QVERIFY(model.hasDevice(static_cast<deviceId_t>(id0)));
QVERIFY(model.hasDevice(static_cast<deviceId_t>(id1)));
QCOMPARE(model.deviceSettings(static_cast<deviceId_t>(id0))->adapterId(), QStringLiteral("adapterA"));
QCOMPARE(model.deviceSettings(static_cast<deviceId_t>(id1))->adapterId(), QStringLiteral("adapterA"));
}
void TestAdapterDeviceSettings::adapterSwitchKeepsExistingDeviceId()
{
SettingsModel model;
setupAdapter(model, "adapterA", QJsonArray());
setupAdapter(model, "adapterB", QJsonArray());
AdapterDeviceSettings w(&model);
auto* tabs = w.findChild<AddableTabWidget*>();
QVERIFY(tabs != nullptr);
emit tabs->addTabRequested();
auto* tab = qobject_cast<DeviceConfigTab*>(tabs->tabContent(0));
QVERIFY(tab != nullptr);
const int before = tab->values().value("id").toInt(-1);
QVERIFY(before >= 1);
auto* combo = tab->findChild<QComboBox*>();
QVERIFY(combo != nullptr);
const int newIndex = combo->findData(QStringLiteral("adapterB"));
QVERIFY(newIndex >= 0);
combo->setCurrentIndex(newIndex);
const int after = tab->values().value("id").toInt(-1);
QCOMPARE(after, before);
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@tests/dialogs/tst_adapterdevicesettings.cpp` around lines 239 - 270, Add a
regression test that exercises the "ID must remain unchanged when switching
adapter" path in DeviceConfigTab::onAdapterChanged: extend or add a test (e.g.,
alongside TestAdapterDeviceSettings::addTabIncrementsDeviceId) that creates a
SettingsModel, sets up two adapters (e.g., "adapterA" and "adapterB") via
setupAdapter, adds a DeviceConfigTab through
AdapterDeviceSettings/AddableTabWidget, captures the assigned device id from
tab->values()["id"], then simulate changing the tab's adapter to the other
adapter (invoke the same adapter-change action used by
DeviceConfigTab::onAdapterChanged) and assert the tab's id remains equal to the
original id, the model still has that deviceId, and
deviceSettings(...)->adapterId() reflects the new adapter; reference
DeviceConfigTab, AdapterDeviceSettings, AddableTabWidget, SettingsModel and
TestAdapterDeviceSettings when locating where to add the test.

}

QTEST_MAIN(TestAdapterDeviceSettings)
1 change: 1 addition & 0 deletions tests/dialogs/tst_adapterdevicesettings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ private slots:
void acceptValuesSavesToAdapterConfig();
void acceptValuesSavesDeviceNameToModel();
void addTabUsesDeviceDefaults();
void addTabIncrementsDeviceId();

private:
//! Populate \a model with an adapter that has a minimal device schema and
Expand Down