Open
Conversation
… 3-5) Reintroduce read-only support for the old XML-based .mbs project file format. The XML parser converts legacy files into the same ProjectSettings structure used by the JSON parser, including constructing the adapter settings JSON blob from XML connection/device elements. Saving always produces JSON v6, effectively auto-migrating old files. All XML-specific code is isolated in separate files (ProjectFileXmlParser) for easy future removal. The only change to existing code is a small format-detection branch in ProjectFileHandler::openProjectFile(). https://claude.ai/code/session_01Lyu9U25MtH34zG7gUFLe2u
WalkthroughThis pull request introduces XML project file parsing for legacy ModbusScope project files (data levels 3–5). It adds a new Changes
Sequence DiagramsequenceDiagram
actor User
participant App as Application
participant Handler as ProjectFileHandler
participant Detector as Format Detection
participant JsonParser as ProjectFileJsonParser
participant XmlParser as ProjectFileXmlParser
participant DOM as QDomDocument
participant Settings as ProjectSettings
User->>App: Open project file
App->>Handler: openProjectFile(fileContent)
Handler->>Detector: Trim & inspect first character
alt First char is '{'
Detector->>JsonParser: Select JSON parser
JsonParser->>Settings: parseFile(content, pSettings)
else First char is '<'
Detector->>XmlParser: Select XML parser
XmlParser->>DOM: Load XML content
DOM-->>XmlParser: QDomDocument parsed
XmlParser->>XmlParser: Validate root element
XmlParser->>XmlParser: Extract dataLevel attribute
XmlParser->>XmlParser: Parse modbus/scope/view tags
XmlParser->>Settings: populateSettings(pSettings)
else Invalid format
Detector->>Handler: Report parse error
Handler->>App: Display "not a valid MBS file" error
end
XmlParser-->>Handler: GeneralError result
Handler->>App: Display result or error
Possibly Related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 5
🤖 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/importexport/projectfilexmlparser.cpp`:
- Around line 238-255: The parsing currently casts the unsigned int from
child.text().toUInt(&bRet) directly to quint8 for
ProjectFileDefinitions::cSlaveIdTag and ::cConsecutiveMaxTag (variables slaveId
and consecutiveMax), allowing out-of-range values (e.g. 300) to wrap; change the
logic to first parse to an unsigned integer, verify bRet is true and that the
parsed value <= 255 (or std::numeric_limits<quint8>::max()), report the same
parseErr on failure, and only then assign/cast to quint8; apply the identical
range check to the other occurrence noted (around lines 334-351) so both parsing
sites reject out-of-range 8-bit values before narrowing.
- Around line 429-444: The parser currently validates log file paths using
QFileInfo(child.text()) which treats relative paths as relative to the process
CWD and can drop valid project-relative paths; modify the logic in
projectfilexmlparser.cpp (the block building QFileInfo from child.text() and
setting pLogSettings->bLogToFileFile / pLogSettings->logFile) to stop using CWD
for relative-path checks: either accept the path without checking
QFileInfo.dir().exists() for relative paths or construct QFileInfo/QDir relative
to the project file base directory (pass in the project base dir into the parser
and use QFileInfo(projectBaseDir, child.text()) or
QDir(projectBaseDir).exists()) before marking bValid and assigning
pLogSettings->logFile so project-relative targets are not rejected.
- Around line 511-519: The code path handling
ProjectFileDefinitions::cValueAxisTag currently ignores malformed numbers and
leaves pRegisterSettings->valueAxis at its default; update the parsing in
projectfilexmlparser.cpp so that when child.tagName() ==
ProjectFileDefinitions::cValueAxisTag and child.text().toUInt(&bOk) yields bOk
== false you report a parsing error (same mechanism used by the other numeric
fields) and abort parsing (e.g. set the parser error/emit an XML error
referencing the tag and return false/upstream failure) instead of silently
continuing; reference pRegisterSettings, child,
ProjectFileDefinitions::cValueAxisTag and the valueAxis member when implementing
the check.
- Around line 836-843: The code currently hardcodes adapterId = 0 and overwrites
every device; instead compute the actual new adapter index via auto
newAdapterIndex = pGeneralSettings->adapterList.size() - 1 after appending
adapterSettings and only assign that index/type to devices that are truly new or
uninitialized (e.g. whose deviceSettings[].adapterId is invalid/placeholder or
whose adapterType is empty) rather than iterating all entries; update the loop
that sets adapterId/adapterType to check deviceSettings[i].adapterId (or
adapterType) before modifying so existing devices are not rewritten and new
devices point to newAdapterIndex with adapterType = "modbus".
- Around line 671-685: The parser is using the system default locale (QLocale())
when converting persisted numeric strings, so decimals written with '.' can fail
on locales using ','; update the parsing to use the fixed C locale instead:
replace uses of QLocale().toDouble(...) when filling pYAxisSettings->scaleMin
and pYAxisSettings->scaleMax (and any similar conversions in this file) with
QLocale::c().toDouble(...), and keep the existing error reporting
(parseErr.reportError) behavior unchanged.
🪄 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: 3c1c5984-6205-451a-a398-66c166895208
📒 Files selected for processing (11)
src/communication/modbuspoll.cppsrc/dialogs/addregisterwidget.cppsrc/importexport/projectfiledefinitions.hsrc/importexport/projectfilehandler.cppsrc/importexport/projectfilexmlparser.cppsrc/importexport/projectfilexmlparser.htests/importexport/CMakeLists.txttests/importexport/projectfilexmltestdata.cpptests/importexport/projectfilexmltestdata.htests/importexport/tst_projectfilexmlparser.cpptests/importexport/tst_projectfilexmlparser.h
💤 Files with no reviewable changes (1)
- src/communication/modbuspoll.cpp
Comment on lines
+238
to
+255
| if (child.tagName() == ProjectFileDefinitions::cSlaveIdTag) | ||
| { | ||
| slaveId = static_cast<quint8>(child.text().toUInt(&bRet)); | ||
| if (!bRet) | ||
| { | ||
| parseErr.reportError(QString("Slave id ( %1 ) is not a valid number").arg(child.text())); | ||
| break; | ||
| } | ||
| } | ||
| else if (child.tagName() == ProjectFileDefinitions::cConsecutiveMaxTag) | ||
| { | ||
| consecutiveMax = static_cast<quint8>(child.text().toUInt(&bRet)); | ||
| if (!bRet) | ||
| { | ||
| parseErr.reportError( | ||
| QString("Consecutive register maximum ( %1 ) is not a valid number").arg(child.text())); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Reject out-of-range 8-bit values before narrowing.
toUInt() only proves the text is unsigned. Values like 300 still pass and then wrap when cast to quint8, so an invalid XML file can silently turn into the wrong slaveid or consecutivemax.
Proposed fix
if (child.tagName() == ProjectFileDefinitions::cSlaveIdTag)
{
- slaveId = static_cast<quint8>(child.text().toUInt(&bRet));
- if (!bRet)
+ const quint32 value = child.text().toUInt(&bRet);
+ if (!bRet || value > 255)
{
parseErr.reportError(QString("Slave id ( %1 ) is not a valid number").arg(child.text()));
break;
}
+ slaveId = static_cast<quint8>(value);
}
else if (child.tagName() == ProjectFileDefinitions::cConsecutiveMaxTag)
{
- consecutiveMax = static_cast<quint8>(child.text().toUInt(&bRet));
- if (!bRet)
+ const quint32 value = child.text().toUInt(&bRet);
+ if (!bRet || value > 255)
{
parseErr.reportError(
QString("Consecutive register maximum ( %1 ) is not a valid number").arg(child.text()));
break;
}
+ consecutiveMax = static_cast<quint8>(value);
}Apply the same check in both parsing functions.
Also applies to: 334-351
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/importexport/projectfilexmlparser.cpp` around lines 238 - 255, The
parsing currently casts the unsigned int from child.text().toUInt(&bRet)
directly to quint8 for ProjectFileDefinitions::cSlaveIdTag and
::cConsecutiveMaxTag (variables slaveId and consecutiveMax), allowing
out-of-range values (e.g. 300) to wrap; change the logic to first parse to an
unsigned integer, verify bRet is true and that the parsed value <= 255 (or
std::numeric_limits<quint8>::max()), report the same parseErr on failure, and
only then assign/cast to quint8; apply the identical range check to the other
occurrence noted (around lines 334-351) so both parsing sites reject
out-of-range 8-bit values before narrowing.
Comment on lines
+429
to
+444
| QFileInfo fileInfo = QFileInfo(child.text()); | ||
| bool bValid = true; | ||
|
|
||
| if (!fileInfo.isFile()) | ||
| { | ||
| if (fileInfo.exists() || !fileInfo.dir().exists()) | ||
| { | ||
| bValid = false; | ||
| } | ||
| } | ||
|
|
||
| if (bValid) | ||
| { | ||
| pLogSettings->bLogToFileFile = true; | ||
| pLogSettings->logFile = fileInfo.filePath(); | ||
| } |
There was a problem hiding this comment.
Don't validate log paths against the current working directory.
This parser only receives file contents, so QFileInfo(child.text()).dir().exists() resolves relative paths against the process CWD, not the project file's directory. Valid project-relative log targets can therefore be dropped during import.
Proposed fix
if (child.tagName() == ProjectFileDefinitions::cFilenameTag)
{
- QFileInfo fileInfo = QFileInfo(child.text());
- bool bValid = true;
-
- if (!fileInfo.isFile())
- {
- if (fileInfo.exists() || !fileInfo.dir().exists())
- {
- bValid = false;
- }
- }
-
- if (bValid)
+ const QString fileName = child.text().trimmed();
+ if (!fileName.isEmpty())
{
pLogSettings->bLogToFileFile = true;
- pLogSettings->logFile = fileInfo.filePath();
+ pLogSettings->logFile = fileName;
}
}If you want validation, it needs the project file's base directory passed in explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/importexport/projectfilexmlparser.cpp` around lines 429 - 444, The parser
currently validates log file paths using QFileInfo(child.text()) which treats
relative paths as relative to the process CWD and can drop valid
project-relative paths; modify the logic in projectfilexmlparser.cpp (the block
building QFileInfo from child.text() and setting pLogSettings->bLogToFileFile /
pLogSettings->logFile) to stop using CWD for relative-path checks: either accept
the path without checking QFileInfo.dir().exists() for relative paths or
construct QFileInfo/QDir relative to the project file base directory (pass in
the project base dir into the parser and use QFileInfo(projectBaseDir,
child.text()) or QDir(projectBaseDir).exists()) before marking bValid and
assigning pLogSettings->logFile so project-relative targets are not rejected.
Comment on lines
+511
to
+519
| else if (child.tagName() == ProjectFileDefinitions::cValueAxisTag) | ||
| { | ||
| bool bOk = false; | ||
| quint32 axis = child.text().toUInt(&bOk); | ||
| if (bOk) | ||
| { | ||
| pRegisterSettings->valueAxis = axis; | ||
| } | ||
| } |
There was a problem hiding this comment.
Malformed <valueaxis> should fail fast.
Every other numeric field reports bad input, but this one silently ignores it. That makes an invalid project load with the register bound to the default axis instead of surfacing the XML error.
Proposed fix
else if (child.tagName() == ProjectFileDefinitions::cValueAxisTag)
{
bool bOk = false;
quint32 axis = child.text().toUInt(&bOk);
- if (bOk)
+ if (!bOk)
{
- pRegisterSettings->valueAxis = axis;
+ parseErr.reportError(QString("Value axis ( %1 ) is not a valid number").arg(child.text()));
+ break;
}
+ pRegisterSettings->valueAxis = axis;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/importexport/projectfilexmlparser.cpp` around lines 511 - 519, The code
path handling ProjectFileDefinitions::cValueAxisTag currently ignores malformed
numbers and leaves pRegisterSettings->valueAxis at its default; update the
parsing in projectfilexmlparser.cpp so that when child.tagName() ==
ProjectFileDefinitions::cValueAxisTag and child.text().toUInt(&bOk) yields bOk
== false you report a parsing error (same mechanism used by the other numeric
fields) and abort parsing (e.g. set the parser error/emit an XML error
referencing the tag and return false/upstream failure) instead of silently
continuing; reference pRegisterSettings, child,
ProjectFileDefinitions::cValueAxisTag and the valueAxis member when implementing
the check.
Comment on lines
+671
to
+685
| pYAxisSettings->scaleMin = QLocale().toDouble(child.text(), &bRet); | ||
| if (bRet) | ||
| { | ||
| bMin = true; | ||
| } | ||
| else | ||
| { | ||
| parseErr.reportError( | ||
| QString("Scale (y-axis) has an incorrect minimum. \"%1\" is not a valid double").arg(child.text())); | ||
| break; | ||
| } | ||
| } | ||
| else if (child.tagName() == ProjectFileDefinitions::cMaxTag) | ||
| { | ||
| pYAxisSettings->scaleMax = QLocale().toDouble(child.text(), &bRet); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the specific file and lines mentioned
cat -n src/importexport/projectfilexmlparser.cpp | sed -n '665,690p'Repository: ModbusScope/NotOnlyModbusScope
Length of output: 1084
🏁 Script executed:
# Search for all QLocale() usages in the codebase to understand the scope
rg 'QLocale\(\)' --type cpp --type hpp -nRepository: ModbusScope/NotOnlyModbusScope
Length of output: 101
🏁 Script executed:
# Search specifically for QLocale().toDouble patterns
rg 'QLocale\(\)\.toDouble' --type cpp --type hpp -nRepository: ModbusScope/NotOnlyModbusScope
Length of output: 101
🏁 Script executed:
# Search for all QLocale() usages without file type restrictions
rg 'QLocale\(\)' -nRepository: ModbusScope/NotOnlyModbusScope
Length of output: 2337
🏁 Script executed:
# Search for QLocale::c() usage to see if it's used elsewhere
rg 'QLocale::c' -nRepository: ModbusScope/NotOnlyModbusScope
Length of output: 56
🏁 Script executed:
# Search for .toDouble patterns in general to understand parsing patterns
rg '\.toDouble\(' -n | head -30Repository: ModbusScope/NotOnlyModbusScope
Length of output: 2232
🏁 Script executed:
# Check projectfilejsonparser.cpp for how it parses doubles
cat -n src/importexport/projectfilejsonparser.cpp | sed -n '465,475p'Repository: ModbusScope/NotOnlyModbusScope
Length of output: 544
Use a fixed locale for parsing persisted project file data.
QLocale() uses the system's current locale, causing project files written with . decimals to fail or misparse on systems configured for , as the decimal separator. Project data stored on disk must be locale-independent.
Proposed fix
if (child.tagName() == ProjectFileDefinitions::cMinTag)
{
- pYAxisSettings->scaleMin = QLocale().toDouble(child.text(), &bRet);
+ pYAxisSettings->scaleMin = QLocale::c().toDouble(child.text(), &bRet);
if (bRet)
{
bMin = true;
@@
else if (child.tagName() == ProjectFileDefinitions::cMaxTag)
{
- pYAxisSettings->scaleMax = QLocale().toDouble(child.text(), &bRet);
+ pYAxisSettings->scaleMax = QLocale::c().toDouble(child.text(), &bRet);
if (bRet)
{
bMax = true;📝 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
| pYAxisSettings->scaleMin = QLocale().toDouble(child.text(), &bRet); | |
| if (bRet) | |
| { | |
| bMin = true; | |
| } | |
| else | |
| { | |
| parseErr.reportError( | |
| QString("Scale (y-axis) has an incorrect minimum. \"%1\" is not a valid double").arg(child.text())); | |
| break; | |
| } | |
| } | |
| else if (child.tagName() == ProjectFileDefinitions::cMaxTag) | |
| { | |
| pYAxisSettings->scaleMax = QLocale().toDouble(child.text(), &bRet); | |
| pYAxisSettings->scaleMin = QLocale::c().toDouble(child.text(), &bRet); | |
| if (bRet) | |
| { | |
| bMin = true; | |
| } | |
| else | |
| { | |
| parseErr.reportError( | |
| QString("Scale (y-axis) has an incorrect minimum. \"%1\" is not a valid double").arg(child.text())); | |
| break; | |
| } | |
| } | |
| else if (child.tagName() == ProjectFileDefinitions::cMaxTag) | |
| { | |
| pYAxisSettings->scaleMax = QLocale::c().toDouble(child.text(), &bRet); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/importexport/projectfilexmlparser.cpp` around lines 671 - 685, The parser
is using the system default locale (QLocale()) when converting persisted numeric
strings, so decimals written with '.' can fail on locales using ','; update the
parsing to use the fixed C locale instead: replace uses of
QLocale().toDouble(...) when filling pYAxisSettings->scaleMin and
pYAxisSettings->scaleMax (and any similar conversions in this file) with
QLocale::c().toDouble(...), and keep the existing error reporting
(parseErr.reportError) behavior unchanged.
Comment on lines
+836
to
+843
| pGeneralSettings->adapterList.append(adapterSettings); | ||
|
|
||
| /* Set adapterId on all devices to point to adapter index 0 */ | ||
| for (int i = 0; i < pGeneralSettings->deviceSettings.size(); i++) | ||
| { | ||
| pGeneralSettings->deviceSettings[i].adapterId = 0; | ||
| pGeneralSettings->deviceSettings[i].adapterType = "modbus"; | ||
| } |
There was a problem hiding this comment.
Use the actual adapter index and avoid rewriting pre-existing devices.
This helper always assigns adapterId = 0 and updates every entry in deviceSettings. If GeneralSettings is not empty, new devices will point at the wrong adapter and older devices get rewritten as well.
Proposed fix
- pGeneralSettings->adapterList.append(adapterSettings);
-
- /* Set adapterId on all devices to point to adapter index 0 */
- for (int i = 0; i < pGeneralSettings->deviceSettings.size(); i++)
+ const int adapterIndex = pGeneralSettings->adapterList.size();
+ const int firstNewDevice = pGeneralSettings->deviceSettings.size() - static_cast<int>(devicesArray.size());
+ pGeneralSettings->adapterList.append(adapterSettings);
+
+ for (int i = firstNewDevice; i < pGeneralSettings->deviceSettings.size(); ++i)
{
- pGeneralSettings->deviceSettings[i].adapterId = 0;
+ pGeneralSettings->deviceSettings[i].adapterId = adapterIndex;
pGeneralSettings->deviceSettings[i].adapterType = "modbus";
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/importexport/projectfilexmlparser.cpp` around lines 836 - 843, The code
currently hardcodes adapterId = 0 and overwrites every device; instead compute
the actual new adapter index via auto newAdapterIndex =
pGeneralSettings->adapterList.size() - 1 after appending adapterSettings and
only assign that index/type to devices that are truly new or uninitialized (e.g.
whose deviceSettings[].adapterId is invalid/placeholder or whose adapterType is
empty) rather than iterating all entries; update the loop that sets
adapterId/adapterType to check deviceSettings[i].adapterId (or adapterType)
before modifying so existing devices are not rewritten and new devices point to
newAdapterIndex with adapterType = "modbus".
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
Release Notes
New Features
Tests