Conversation
WalkthroughAdds Draft‑7 JSON Schema conditional (if/then/else) support to adapter describe schema and the schema-driven UI; SchemaFormWidget parses/applies conditionals to show/hide branch fields and filter returned values; adapter connection schema was converted to conditionals; AdapterConnectionSettings removed and replaced by a generic AdapterSettings that builds pages per property. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsDialog
participant AdapterSettings
participant SchemaFormWidget
participant SettingsModel
User->>SettingsDialog: open dialog
SettingsDialog->>AdapterSettings: construct per property (schema, config)
AdapterSettings->>SchemaFormWidget: setSchema(schema), setValues(config)
SchemaFormWidget->>SchemaFormWidget: parseConditional(if/then/else)
User->>SchemaFormWidget: change trigger field (e.g., type combo)
SchemaFormWidget->>SchemaFormWidget: onTriggerChanged -> applyConditional (show/hide fields)
User->>SettingsDialog: accept dialog
SettingsDialog->>AdapterSettings: acceptValues()
AdapterSettings->>SchemaFormWidget: values()
SchemaFormWidget-->>AdapterSettings: filtered JSON (only visible fields)
AdapterSettings->>SettingsModel: setAdapterCurrentConfig(adapterId, updatedConfig)
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
tests/dialogs/tst_adapterdevicesettings.cpp (1)
134-137: Remove the redundant null-check afterQVERIFY.Line 133 already fails-and-returns on null in Qt Test, so Lines 134-137 are dead-path noise.
🧹 Optional cleanup
- if (tabs == nullptr) - { - return; - }🤖 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 134 - 137, Remove the redundant null-check block that follows the Qt Test assertion: after the QVERIFY(tabs) call, delete the if (tabs == nullptr) { return; } block so execution relies on the test assertion rather than a dead-path return; this targets the local variable tabs in tst_adapterdevicesettings.cpp (the code surrounding the QVERIFY(tabs) check).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@adapters/json-rpc-spec.md`:
- Line 123: Update the sentence in adapters/json-rpc-spec.md that reads
"Otherwise the fields in `else.properties` apply (serial-specific)." to insert a
comma after "Otherwise" so it becomes "Otherwise, the fields in
`else.properties` apply (serial-specific)." This is a small punctuation/clarity
fix in the paragraph describing the JSON Schema Draft 7 `if`/`then`/`else`
behavior for the `type`-dependent connection fields.
In `@src/customwidgets/schemaformwidget.cpp`:
- Around line 107-120: The active branch must be derived from the actual trigger
widget state instead of the incoming JSON: find the trigger widget in _fields
(the same loop that qobject_casts to QComboBox) and after connecting the combo's
currentIndexChanged to SchemaFormWidget::onTriggerChanged, read the combo's
current value (e.g. combo->currentText() or combo->currentData()) and set
_currentTriggerValue accordingly, then call applyConditional using that
widget-derived value rather than values.value(_conditionalTriggerKey); ensure
this update happens before leaving the constructor/initialization so the visible
branch and values() are in sync.
In `@src/dialogs/adaptersettings.cpp`:
- Around line 66-79: The add-tab lambda attached to
AddableTabWidget::addTabRequested should be extracted into a named helper (e.g.,
a private method like createAndAddItemTab or addItemTab) and a monotonic counter
field (e.g., _nextItemTabIndex) should be introduced on the class to generate
unique tab titles instead of using _pItemTabs->count() + 1; inside the new
helper create the SchemaFormWidget, read defaults via
_pSettingsModel->adapterData(_adapterId)->defaults().value(_propertyKey).toArray(),
set the schema with defaultValues, build the title from _propertyKey and the
monotonic counter, increment the counter, and call _pItemTabs->addNewTab(name,
form); finally replace the multi-statement lambda passed to connect(...) with a
simple call to this new helper.
- Around line 17-18: The code creates a QVBoxLayout with this as parent and then
redundantly calls setLayout(layout); remove the unnecessary call to
setLayout(layout) so the QVBoxLayout(this) installation is relied upon—i.e.,
delete the setLayout(layout) line where QVBoxLayout is constructed to avoid
duplicate layout assignment.
In `@src/dialogs/settingsdialog.cpp`:
- Around line 99-120: The dialog currently persists settings on navigation and
on done regardless of accept/reject: remove the side-effect by stopping calls to
acceptAllValues() from settingsStackSwitch(int newRow) (leave it to only change
_pUi->settingsStack->setCurrentIndex(newRow)) and change
SettingsDialog::done(int r) to only call acceptAllValues() when r ==
QDialog::Accepted (i.e. gate persistence on explicit accept); keep references to
the functions acceptAllValues, done, and settingsStackSwitch to locate and
update the calls.
In `@tests/customwidgets/tst_schemaformwidget.cpp`:
- Around line 402-420: The test TestSchemaFormWidget::labelHiddenWithWidget uses
QLabel::isVisible() which is false for all children because the parent
SchemaFormWidget w is not shown; change the assertion to check the explicit
hidden state using QLabel::isHidden() instead (i.e., when iterating labels
inside labelHiddenWithWidget, replace the QVERIFY(!lbl->isVisible()) check with
QVERIFY(lbl->isHidden()) so the conditional hide logic is properly verified).
---
Nitpick comments:
In `@tests/dialogs/tst_adapterdevicesettings.cpp`:
- Around line 134-137: Remove the redundant null-check block that follows the Qt
Test assertion: after the QVERIFY(tabs) call, delete the if (tabs == nullptr) {
return; } block so execution relies on the test assertion rather than a
dead-path return; this targets the local variable tabs in
tst_adapterdevicesettings.cpp (the code surrounding the QVERIFY(tabs) check).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4a9174d3-1a15-4a46-86a0-939b276e763b
⛔ Files ignored due to path filters (3)
adapters/dummymodbusadapter.exeis excluded by!**/*.exeadapters/modbusadapter.exeis excluded by!**/*.exeadapters/schemadump.exeis excluded by!**/*.exe
📒 Files selected for processing (25)
adapters/describe.jsonadapters/dummymodbusadapteradapters/json-rpc-spec.mdadapters/modbusadaptersrc/customwidgets/deviceconfigtab.cppsrc/customwidgets/schemaformwidget.cppsrc/customwidgets/schemaformwidget.hsrc/dialogs/adapterconnectionsettings.cppsrc/dialogs/adapterconnectionsettings.hsrc/dialogs/adapterdevicesettings.cppsrc/dialogs/adaptersettings.cppsrc/dialogs/adaptersettings.hsrc/dialogs/settingsdialog.cppsrc/dialogs/settingsdialog.htests/customwidgets/tst_deviceconfigtab.cpptests/customwidgets/tst_deviceconfigtab.htests/customwidgets/tst_schemaformwidget.cpptests/customwidgets/tst_schemaformwidget.htests/dialogs/CMakeLists.txttests/dialogs/tst_adapterconnectionsettings.cpptests/dialogs/tst_adapterconnectionsettings.htests/dialogs/tst_adapterdevicesettings.cpptests/dialogs/tst_adapterdevicesettings.htests/dialogs/tst_adaptersettings.cpptests/dialogs/tst_adaptersettings.h
💤 Files with no reviewable changes (4)
- tests/dialogs/tst_adapterconnectionsettings.h
- src/dialogs/adapterconnectionsettings.cpp
- src/dialogs/adapterconnectionsettings.h
- tests/dialogs/tst_adapterconnectionsettings.cpp
- adapters/json-rpc-spec.md: add comma after "Otherwise" (grammar) - schemaformwidget.cpp: derive initial conditional branch from the trigger combo's current value instead of the incoming JSON values, preventing visible/values() mismatch on first render - adaptersettings.cpp/h: remove redundant setLayout() call; extract add-tab lambda into named addItemTab() method with monotonic _nextItemTabIndex counter to avoid duplicate tab titles after a tab is closed - settingsdialog.cpp: iterate all adapters (not just the first) when building dynamic settings pages; only persist settings when dialog is accepted (gate acceptAllValues on r == QDialog::Accepted) and remove side-effectful acceptAllValues() call from settingsStackSwitch - tst_schemaformwidget.cpp: use isHidden() instead of isVisible() so the conditional-hide assertion works even when the parent is not shown https://claude.ai/code/session_013ytSJa46akaAHkox9rjeRa
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/dialogs/adaptersettings.cpp (1)
57-57: Consider defensive check for empty_propertyKey.Accessing
_propertyKey[0]on an empty QString returns a null QChar, which would produce an unexpected tab name. While the caller (SettingsDialog) iterates schema properties ensuring non-empty keys, a defensive check would make this more robust.🛡️ Proposed defensive check
+QString AdapterSettings::formatTabName(int index) const +{ + if (_propertyKey.isEmpty()) + { + return QString("Item %1").arg(index); + } + return QString("%1 %2").arg(_propertyKey[0].toUpper() + _propertyKey.mid(1)).arg(index); +}Then use
formatTabName(i + 1)at line 57 andformatTabName(_nextItemTabIndex)at lines 91-92.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dialogs/adaptersettings.cpp` at line 57, The tab-name construction uses _propertyKey[0] without guarding for an empty QString; add a defensive helper (e.g., formatTabName) that returns a safe display name when _propertyKey is empty and otherwise builds "%1 %2" using the capitalized first character plus mid(1) and the index. Replace the direct expression in names.append(...) (and the usages at the sites that use _nextItemTabIndex) with a call to formatTabName(i + 1) (and formatTabName(_nextItemTabIndex)) so _propertyKey is never indexed when empty and tab names remain predictable.tests/customwidgets/tst_schemaformwidget.cpp (1)
346-350: Redundant null check afterQVERIFY.
QVERIFY(typeCombo != nullptr)at line 346 already fails the test if the pointer is null, making the subsequentif (typeCombo == nullptr) { return; }block unreachable. The same pattern appears at lines 376-380.♻️ Proposed simplification
QVERIFY(typeCombo != nullptr); - if (typeCombo == nullptr) - { - return; - } typeCombo->setCurrentIndex(typeCombo->findData(QStringLiteral("serial")));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/customwidgets/tst_schemaformwidget.cpp` around lines 346 - 350, The test contains redundant null checks after assertions: remove the unreachable if (typeCombo == nullptr) { return; } blocks that follow QVERIFY(typeCombo != nullptr) (and the same pattern around the later occurrence at lines referencing typeCombo/assertions), leaving only the QVERIFY checks; this means locate the QVERIFY(typeCombo != nullptr) assertions and delete the subsequent defensive if-return blocks so the test relies on the assertion failure to stop execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/dialogs/adaptersettings.cpp`:
- Line 57: The tab-name construction uses _propertyKey[0] without guarding for
an empty QString; add a defensive helper (e.g., formatTabName) that returns a
safe display name when _propertyKey is empty and otherwise builds "%1 %2" using
the capitalized first character plus mid(1) and the index. Replace the direct
expression in names.append(...) (and the usages at the sites that use
_nextItemTabIndex) with a call to formatTabName(i + 1) (and
formatTabName(_nextItemTabIndex)) so _propertyKey is never indexed when empty
and tab names remain predictable.
In `@tests/customwidgets/tst_schemaformwidget.cpp`:
- Around line 346-350: The test contains redundant null checks after assertions:
remove the unreachable if (typeCombo == nullptr) { return; } blocks that follow
QVERIFY(typeCombo != nullptr) (and the same pattern around the later occurrence
at lines referencing typeCombo/assertions), leaving only the QVERIFY checks;
this means locate the QVERIFY(typeCombo != nullptr) assertions and delete the
subsequent defensive if-return blocks so the test relies on the assertion
failure to stop execution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 36c596bb-e7ec-4509-bf58-aef8fb40fd51
📒 Files selected for processing (6)
adapters/json-rpc-spec.mdsrc/customwidgets/schemaformwidget.cppsrc/dialogs/adaptersettings.cppsrc/dialogs/adaptersettings.hsrc/dialogs/settingsdialog.cpptests/customwidgets/tst_schemaformwidget.cpp
✅ Files skipped from review due to trivial changes (1)
- adapters/json-rpc-spec.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/dialogs/settingsdialog.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/customwidgets/tst_schemaformwidget.cpp (1)
372-376: Remove redundant null-guard afterQVERIFY.After
QVERIFY(typeCombo != nullptr);, theif (typeCombo == nullptr) { return; }branch is unnecessary and can be dropped for cleaner test flow.Suggested simplification
QVERIFY(typeCombo != nullptr); - if (typeCombo == nullptr) - { - return; - } typeCombo->setCurrentIndex(typeCombo->findData(QStringLiteral("tcp")));As per coding guidelines,
**/*.cpp: "Use early return only for error handling; avoid deep nesting".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/customwidgets/tst_schemaformwidget.cpp` around lines 372 - 376, The if-check guarding typeCombo is redundant because QVERIFY(typeCombo != nullptr) already aborts the test on null; remove the block "if (typeCombo == nullptr) { return; }" from tst_schemaformwidget.cpp so the test flow relies on the QVERIFY assertion and avoids unnecessary early return after the QCOMPARE/verification of typeCombo.src/dialogs/adaptersettings.cpp (1)
80-87: Avoid non-error early return informatTabName().This method uses an early return for normal control flow; please refactor to a single return path to match the
.cppguideline.As per coding guidelines, "
**/*.cpp: Use early return only for error handling; avoid deep nesting".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dialogs/adaptersettings.cpp` around lines 80 - 87, AdapterSettings::formatTabName currently returns early when _propertyKey.isEmpty(); refactor to a single return by computing the result in a local QString (e.g., result), set result to "Item %1".arg(index) when _propertyKey.isEmpty(), otherwise set result to QString("%1 %2").arg(_propertyKey[0].toUpper() + _propertyKey.mid(1)).arg(index), and return result at the end; keep the same logic and use the existing symbols (_propertyKey, formatTabName) without adding branching returns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/dialogs/adaptersettings.cpp`:
- Around line 29-31: The array check currently treats any non-empty "items" as
renderable but the code assumes object items; update the checks in
isRenderableProperty (and the same checks at the other two occurrences) to
verify the items schema is an object schema before returning true — e.g. get
propSchema.value("items").toObject() and require either
itemsObj.value("type").toString() == "object" or itemsObj.contains("properties")
(or both) instead of just !itemsObj.isEmpty(), so primitive item schemas are
excluded.
- Around line 11-127: Add brief Doxygen comment blocks for each public function
in this file to satisfy the repo rule: place a /** `@brief` ... */ (optionally
with `@param` and `@return` where appropriate) immediately above the definitions of
AdapterSettings::AdapterSettings, AdapterSettings::isRenderableProperty,
AdapterSettings::buildSection, AdapterSettings::formatTabName,
AdapterSettings::addItemTab, and AdapterSettings::acceptValues describing their
purpose and parameters/return values concisely; ensure comments use Doxygen
syntax and are short one-line briefs to match existing style.
---
Nitpick comments:
In `@src/dialogs/adaptersettings.cpp`:
- Around line 80-87: AdapterSettings::formatTabName currently returns early when
_propertyKey.isEmpty(); refactor to a single return by computing the result in a
local QString (e.g., result), set result to "Item %1".arg(index) when
_propertyKey.isEmpty(), otherwise set result to QString("%1
%2").arg(_propertyKey[0].toUpper() + _propertyKey.mid(1)).arg(index), and return
result at the end; keep the same logic and use the existing symbols
(_propertyKey, formatTabName) without adding branching returns.
In `@tests/customwidgets/tst_schemaformwidget.cpp`:
- Around line 372-376: The if-check guarding typeCombo is redundant because
QVERIFY(typeCombo != nullptr) already aborts the test on null; remove the block
"if (typeCombo == nullptr) { return; }" from tst_schemaformwidget.cpp so the
test flow relies on the QVERIFY assertion and avoids unnecessary early return
after the QCOMPARE/verification of typeCombo.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 764b1e67-c1ad-4164-8559-9a2a669d7a31
⛔ Files ignored due to path filters (2)
adapters/dummymodbusadapter.exeis excluded by!**/*.exeadapters/modbusadapter.exeis excluded by!**/*.exe
📒 Files selected for processing (7)
adapters/describe.jsonadapters/dummymodbusadapteradapters/json-rpc-spec.mdadapters/modbusadaptersrc/dialogs/adaptersettings.cppsrc/dialogs/adaptersettings.htests/customwidgets/tst_schemaformwidget.cpp
✅ Files skipped from review due to trivial changes (2)
- adapters/json-rpc-spec.md
- src/dialogs/adaptersettings.h
🚧 Files skipped from review as they are similar to previous changes (1)
- adapters/describe.json
Comment on lines
+11
to
+127
| AdapterSettings::AdapterSettings(SettingsModel* pSettingsModel, | ||
| const QString& adapterId, | ||
| const QString& propertyKey, | ||
| QWidget* parent) | ||
| : QWidget(parent), _pSettingsModel(pSettingsModel), _adapterId(adapterId), _propertyKey(propertyKey) | ||
| { | ||
| auto* layout = new QVBoxLayout(this); | ||
|
|
||
| const AdapterData* pAdapter = pSettingsModel->adapterData(adapterId); | ||
| const QJsonObject propSchema = pAdapter->schema().value("properties").toObject().value(propertyKey).toObject(); | ||
| const QJsonValue configValue = pAdapter->effectiveConfig().value(propertyKey); | ||
|
|
||
| buildSection(propSchema, configValue, layout); | ||
| } | ||
|
|
||
| bool AdapterSettings::isRenderableProperty(const QJsonObject& propSchema) | ||
| { | ||
| const QString type = propSchema.value("type").toString(); | ||
| if (type == "array") | ||
| { | ||
| return !propSchema.value("items").toObject().isEmpty(); | ||
| } | ||
| if (type == "object") | ||
| { | ||
| return !propSchema.value("properties").toObject().isEmpty(); | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| void AdapterSettings::buildSection(const QJsonObject& propSchema, const QJsonValue& configValue, QVBoxLayout* layout) | ||
| { | ||
| const QString type = propSchema.value("type").toString(); | ||
|
|
||
| if (type == "array") | ||
| { | ||
| _itemSchema = propSchema.value("items").toObject(); | ||
| _pItemTabs = new AddableTabWidget(this); | ||
|
|
||
| const QJsonArray itemsArray = configValue.toArray(); | ||
| QList<QWidget*> pages; | ||
| QStringList names; | ||
| for (int i = 0; i < itemsArray.size(); ++i) | ||
| { | ||
| auto* form = new SchemaFormWidget(_pItemTabs); | ||
| form->setSchema(_itemSchema, itemsArray.at(i).toObject()); | ||
| pages.append(form); | ||
| names.append(formatTabName(i + 1)); | ||
| } | ||
|
|
||
| if (!pages.isEmpty()) | ||
| { | ||
| _pItemTabs->setTabs(pages, names); | ||
| } | ||
|
|
||
| _nextItemTabIndex = itemsArray.size() + 1; | ||
|
|
||
| connect(_pItemTabs, &AddableTabWidget::addTabRequested, this, &AdapterSettings::addItemTab); | ||
|
|
||
| layout->addWidget(_pItemTabs, 1); | ||
| } | ||
| else | ||
| { | ||
| _pForm = new SchemaFormWidget(this); | ||
| _pForm->setSchema(propSchema, configValue.toObject()); | ||
| layout->addWidget(_pForm); | ||
| layout->addStretch(); | ||
| } | ||
| } | ||
|
|
||
| QString AdapterSettings::formatTabName(int index) const | ||
| { | ||
| if (_propertyKey.isEmpty()) | ||
| { | ||
| return QString("Item %1").arg(index); | ||
| } | ||
| return QString("%1 %2").arg(_propertyKey[0].toUpper() + _propertyKey.mid(1)).arg(index); | ||
| } | ||
|
|
||
| void AdapterSettings::addItemTab() | ||
| { | ||
| auto* form = new SchemaFormWidget(_pItemTabs); | ||
| QJsonObject defaultValues; | ||
| const QJsonArray defaultItems = _pSettingsModel->adapterData(_adapterId)->defaults().value(_propertyKey).toArray(); | ||
| if (!defaultItems.isEmpty()) | ||
| { | ||
| defaultValues = defaultItems.first().toObject(); | ||
| } | ||
| form->setSchema(_itemSchema, defaultValues); | ||
| const QString name = formatTabName(_nextItemTabIndex); | ||
| _nextItemTabIndex++; | ||
| _pItemTabs->addNewTab(name, form); | ||
| } | ||
|
|
||
| void AdapterSettings::acceptValues() | ||
| { | ||
| QJsonObject config = _pSettingsModel->adapterData(_adapterId)->effectiveConfig(); | ||
|
|
||
| if (_pItemTabs) | ||
| { | ||
| QJsonArray itemsArray; | ||
| for (int i = 0; i < _pItemTabs->count(); ++i) | ||
| { | ||
| auto* form = qobject_cast<SchemaFormWidget*>(_pItemTabs->tabContent(i)); | ||
| if (form) | ||
| { | ||
| itemsArray.append(form->values()); | ||
| } | ||
| } | ||
| config[_propertyKey] = itemsArray; | ||
| } | ||
| else if (_pForm) | ||
| { | ||
| config[_propertyKey] = _pForm->values(); | ||
| } | ||
|
|
||
| _pSettingsModel->setAdapterCurrentConfig(_adapterId, config); | ||
| } |
There was a problem hiding this comment.
Add Doxygen comments for public functions in this source file.
The public functions here are missing brief Doxygen documentation blocks, which breaks the repo’s source-level documentation rule.
As per coding guidelines, "Document all public functions with brief Doxygen comments in the source file".
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 28-28: There is an unknown macro here somewhere. Configuration is required. If slots is a macro then please configure it.
(unknownMacro)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/dialogs/adaptersettings.cpp` around lines 11 - 127, Add brief Doxygen
comment blocks for each public function in this file to satisfy the repo rule:
place a /** `@brief` ... */ (optionally with `@param` and `@return` where appropriate)
immediately above the definitions of AdapterSettings::AdapterSettings,
AdapterSettings::isRenderableProperty, AdapterSettings::buildSection,
AdapterSettings::formatTabName, AdapterSettings::addItemTab, and
AdapterSettings::acceptValues describing their purpose and parameters/return
values concisely; ensure comments use Doxygen syntax and are short one-line
briefs to match existing style.
Comment on lines
+29
to
+31
| if (type == "array") | ||
| { | ||
| return !propSchema.value("items").toObject().isEmpty(); |
There was a problem hiding this comment.
Constrain renderable array items to object schemas to avoid silent value corruption.
isRenderableProperty() currently accepts any array with non-empty items, but the array flow assumes each item is an object (toObject() on read; form->values() object on write). For primitive array schemas, saving can silently replace values with {} entries.
Proposed fix
bool AdapterSettings::isRenderableProperty(const QJsonObject& propSchema)
{
- const QString type = propSchema.value("type").toString();
- if (type == "array")
- {
- return !propSchema.value("items").toObject().isEmpty();
- }
- if (type == "object")
- {
- return !propSchema.value("properties").toObject().isEmpty();
- }
- return false;
+ bool renderable = false;
+ const QString type = propSchema.value("type").toString();
+ if (type == "array")
+ {
+ const QJsonObject itemSchema = propSchema.value("items").toObject();
+ renderable = itemSchema.value("type").toString() == "object"
+ && !itemSchema.value("properties").toObject().isEmpty();
+ }
+ else if (type == "object")
+ {
+ renderable = !propSchema.value("properties").toObject().isEmpty();
+ }
+ return renderable;
}Also applies to: 55-56, 116-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/dialogs/adaptersettings.cpp` around lines 29 - 31, The array check
currently treats any non-empty "items" as renderable but the code assumes object
items; update the checks in isRenderableProperty (and the same checks at the
other two occurrences) to verify the items schema is an object schema before
returning true — e.g. get propSchema.value("items").toObject() and require
either itemsObj.value("type").toString() == "object" or
itemsObj.contains("properties") (or both) instead of just !itemsObj.isEmpty(),
so primitive item schemas are excluded.
This was referenced Apr 16, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Tests