Merged
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 FormatDetector
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 & datalevel
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 project 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
+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.
- Resolve relative log-file paths against the project base directory instead of the process CWD; pass the base dir from the file handler through parseFile and use it in parseLogToFile - Report a parse error (instead of silently ignoring) when <valueaxis> contains a non-numeric value, consistent with all other numeric fields - Compute the real adapter index after appending rather than hardcoding 0; centralise adapterType/adapterId assignment in buildAdapterSettings and guard with adapterType.isEmpty() so pre-linked devices are skipped Adds tests: valueAxisInvalid, logFileRelativePath Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/importexport/projectfilexmlparser.cpp (3)
338-356:⚠️ Potential issue | 🟠 MajorSame 8-bit range check needed here.
Apply the same fix as in
parseLegacyConnectionTagto validate that parsed values are ≤ 255 before casting toquint8.Proposed fix
else 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); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importexport/projectfilexmlparser.cpp` around lines 338 - 356, When parsing child tags ProjectFileDefinitions::cSlaveIdTag and ::cConsecutiveMaxTag in src/importexport/projectfilexmlparser.cpp (the code that assigns slaveId and consecutiveMax), validate the parsed uint value is within 0..255 before casting to quint8: call child.text().toUInt(&bRet), check bRet and that the resulting uintValue <= 255, and only then static_cast to quint8; otherwise call parseErr.reportError with the existing descriptive message and break. Match the same numeric-range check logic used in parseLegacyConnectionTag to avoid truncation when casting.
243-261:⚠️ Potential issue | 🟠 MajorReject out-of-range 8-bit values before narrowing.
toUInt()only validates that the text is a valid unsigned integer. Values like300pass validation but silently wrap when cast toquint8, causingslaveidorconsecutivemaxto contain incorrect values.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); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importexport/projectfilexmlparser.cpp` around lines 243 - 261, The current parsing for ProjectFileDefinitions::cSlaveIdTag and ::cConsecutiveMaxTag uses child.text().toUInt(&bRet) then static_cast<quint8>, which accepts values like "300" and silently wraps; modify the parsing in projectfilexmlparser.cpp to first parse to a larger unsigned (e.g. quint32 or unsigned int) with toUInt(&bRet), then check bRet and ensure the parsed value is <= 0xFF before assigning to slaveId or consecutiveMax; if out of range report the error via parseErr.reportError (use the same human-friendly messages) and avoid the cast/assignment when invalid.
678-705:⚠️ Potential issue | 🟠 MajorUse a fixed locale for parsing persisted project file data.
QLocale()uses the system's current locale, causing project files written with.as the decimal separator to fail or misparse on systems configured for,(e.g., German, Dutch, French locales). 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; } // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/importexport/projectfilexmlparser.cpp` around lines 678 - 705, The parser currently uses QLocale() (system locale) to parse persisted numeric strings for pYAxisSettings->scaleMin and scaleMax (nodes ProjectFileDefinitions::cMinTag and ::cMaxTag), which breaks on systems using comma decimals; change QLocale().toDouble(...) to use the fixed C locale via QLocale::c().toDouble(...) for both scaleMin and scaleMax (and any other similar parsing sites in this file) so project files are parsed locale-independently, keeping the same error handling via parseErr.reportError when bRet is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/importexport/projectfilexmlparser.cpp`:
- Around line 338-356: When parsing child tags
ProjectFileDefinitions::cSlaveIdTag and ::cConsecutiveMaxTag in
src/importexport/projectfilexmlparser.cpp (the code that assigns slaveId and
consecutiveMax), validate the parsed uint value is within 0..255 before casting
to quint8: call child.text().toUInt(&bRet), check bRet and that the resulting
uintValue <= 255, and only then static_cast to quint8; otherwise call
parseErr.reportError with the existing descriptive message and break. Match the
same numeric-range check logic used in parseLegacyConnectionTag to avoid
truncation when casting.
- Around line 243-261: The current parsing for
ProjectFileDefinitions::cSlaveIdTag and ::cConsecutiveMaxTag uses
child.text().toUInt(&bRet) then static_cast<quint8>, which accepts values like
"300" and silently wraps; modify the parsing in projectfilexmlparser.cpp to
first parse to a larger unsigned (e.g. quint32 or unsigned int) with
toUInt(&bRet), then check bRet and ensure the parsed value is <= 0xFF before
assigning to slaveId or consecutiveMax; if out of range report the error via
parseErr.reportError (use the same human-friendly messages) and avoid the
cast/assignment when invalid.
- Around line 678-705: The parser currently uses QLocale() (system locale) to
parse persisted numeric strings for pYAxisSettings->scaleMin and scaleMax (nodes
ProjectFileDefinitions::cMinTag and ::cMaxTag), which breaks on systems using
comma decimals; change QLocale().toDouble(...) to use the fixed C locale via
QLocale::c().toDouble(...) for both scaleMin and scaleMax (and any other similar
parsing sites in this file) so project files are parsed locale-independently,
keeping the same error handling via parseErr.reportError when bRet is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec76d696-0207-463f-b27d-2fdbf1c195fe
📒 Files selected for processing (7)
src/importexport/projectfilehandler.cppsrc/importexport/projectfilexmlparser.cppsrc/importexport/projectfilexmlparser.htests/importexport/projectfilexmltestdata.cpptests/importexport/projectfilexmltestdata.htests/importexport/tst_projectfilexmlparser.cpptests/importexport/tst_projectfilexmlparser.h
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/importexport/projectfilexmltestdata.h
- tests/importexport/projectfilexmltestdata.cpp
- tests/importexport/tst_projectfilexmlparser.h
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
Tests