Initialize Fact QVariant as invalid #13642
Conversation
Adjusted the default Fact _rawValue from 0 to QVariant::Invalid and updated _variantToString to return placeholders for Facts that haven't received a value yet. This makes it possible to distinguish them from those that are legitimately 0 (or the equivalent for their type). It also avoids starting every Fact's QVariant lifecycle as an integer, which is incorrect in most cases. Related code that set initial values for a few VehicleFactGroup Facts (Hobbs meter, altitude above terrain, flight distance & time) has been updated or removed for consistency.
|
Looking forward to Copilot thrashing this |
There was a problem hiding this comment.
Pull Request Overview
This PR improves the Fact system's initialization by using invalid QVariant instead of defaulting to integer 0, making it possible to distinguish uninitialized Facts from those with legitimate zero values. This change also corrects the QVariant lifecycle, avoiding the incorrect initial state where all Facts started as integers.
Key Changes:
- Changed Fact's default
_rawValuefrom integer 0 toQVariant::Invalid - Added invalid QVariant handling in
_variantToStringwith type-appropriate placeholders - Updated Hobbs meter placeholder from "0000:00:00" to "----:--:--" for consistency
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/FactSystem/Fact.h | Changed default _rawValue initialization from 0 to invalid QVariant with clarifying comment |
| src/FactSystem/Fact.cc | Added invalid QVariant check in _variantToString returning type-appropriate placeholders ("--.--" for floats/doubles, "--:--:--" for elapsed time, "-" for others) |
| src/Vehicle/FactGroups/VehicleFactGroup.cc | Updated Hobbs meter placeholder string from "0000:00:00" to "----:--:--" to indicate uninitialized state |
| src/Vehicle/Vehicle.cc | Removed redundant initializations for altitude above terrain (NaN), flight distance (0), and flight time (0) that are now handled by invalid QVariant default or explicit initialization when needed |
Implemented Fact::placeholderString to centralize the placeholder String generation logic and take into account the number of decimal places. Changed various hardcoded fallback Strings in QML for consistency with the new placeholder String format.
|
I've pushed a small improvement: the number of decimal places is now taken into account for the placeholder String generation, which is also now centralized. For consistency, I've also changed a couple places in the QML where placeholder strings were hardcoded just in case the corresponding Facts weren't available at all (which is a bit unrealistic given that they're just loaded from JSON, but whatever). Here's how it's looking now: After after:
|
| return valueString; | ||
| } | ||
|
|
||
| QString Fact::placeholderString(int decimalPlaces) const { |
There was a problem hiding this comment.
I think this would be better named invalidValueString. What do you think?
| case FactMetaData::valueTypeFloat: | ||
| case FactMetaData::valueTypeDouble: | ||
| if (decimalPlaces <= 0) { | ||
| return QStringLiteral("-"); |
There was a problem hiding this comment.
I would say that using '--' for this would make it clearer this is a missing value. Given the fact that '-' can kinda mean negative and may confuse folks.
| case FactMetaData::valueTypeElapsedTimeInSeconds: | ||
| return QStringLiteral("--:--:--"); | ||
| default: | ||
| return QStringLiteral("-"); |
There was a problem hiding this comment.
Same thing here '--' is clearer.
|
|
||
| property var _distanceSensors: vehicle ? vehicle.distanceSensors : null | ||
| property string _noValueStr: qsTr("--.--") | ||
| property string _noValueStr: qsTr("-") |
There was a problem hiding this comment.
These values are shown with decimal places in the ui when available. So I would keep the --.-- to make it clearer.
| property var activeVehicle: QGroundControl.multiVehicleManager.activeVehicle | ||
| property string na: qsTr("N/A", "No data to display") | ||
| property string valueNA: qsTr("--.--", "No data to display") | ||
| property string valueNA: qsTr("-", "No data to display") |
There was a problem hiding this comment.
These are also shown normally with decimal places. So I would keep it as is to make it more clear. That said, given the way the control works this string is never shown since that sections of values is hidden when no vehicle is available.
2 participants
Initialize Fact QVariant as invalid
Description
Adjusted the default Fact _rawValue from 0 to QVariant::Invalid and updated _variantToString to return placeholders for Facts that haven't received a value yet. This makes it possible to distinguish them from those that are legitimately 0 (or the equivalent for their type). It also avoids starting every Fact's QVariant lifecycle as an integer, which is incorrect in most cases.
Related code that set initial values for a few VehicleFactGroup Facts (Hobbs meter, altitude above terrain, flight distance & time) has been updated or removed for consistency.
Telemetry bar before/after
The following screenshots use these telemetry bar settings, that include all Facts from the VehicleFactGroup:
Click to show telemetry bar settings used
Before:
After:
Checklist:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.