Device Page: support different distance units#2847
Device Page: support different distance units#2847ybott-qinetic wants to merge 6 commits intottomkins/unit-refactorfrom
Conversation
src/units.cpp
Outdated
| { DegreesSymbol, Victron::VenusOS::Enums::Units_CardinalDirection, Unit::Default, 0, false }, | ||
| { "km", Victron::VenusOS::Enums::Units_Distance_Kilometre, Unit::Default, 0, false }, | ||
| { "NM", Victron::VenusOS::Enums::Units_Distance_Nautical_Mile, Unit::Default, 0, false }, | ||
| { "m", Victron::VenusOS::Enums::Units_Distance_Metre, Unit::Metre, 0, false }, |
There was a problem hiding this comment.
Thinking about the renaming that we discussed yesterday, we can rename Units_Altitude_Metre to Units_Metre, and then that unit can be used regardless of whether we are measuring altitude or land distance. That will make sense for both /Units/Altitude and any potential future /Units/Distance. Same with Units_Altitude_Foot, it should be Units_Foot instead. Can you make this renaming change as a first (separate) commit in this PR?
There was a problem hiding this comment.
When adding Units_Kilometre, update units.cpp to avoid scaling for this unit, so that it's not possible to scale to a number like "1.2 kkm".
Also can you add unit tests for the new units? See tst_units.qml.
I wasn't sure initially about adding Units_Kilometre if we already have Units_Metre - will it cause confusion about which one to use? But I suppose it's convenient to have both if you want to don't want to convert the source VeQuickItem value when displaying a "km" value in a QuantityLabel. And then we could also use it for direct unit conversions with VeQuickItem::sourceUnit.
There was a problem hiding this comment.
I think I have addressed the above items.
| value: dataItem.valid ? dataItem.value * 1000 : NaN // convert raw km -> m | ||
| unit: VenusOS.Units_Altitude_Metre | ||
| value: dataItem.valid ? root._metresToSystemDistanceUnit(dataItem.value) : NaN // convert raw km -> m | ||
| unit: root._systemDistanceUnit() |
There was a problem hiding this comment.
Instead of having _metresToSystemDistanceUnit(), you can do unit conversions directly on the data item, using VeQuickItem::sourceUnit and VeQuickItem::displayUnit. To do this, update the veutil unit conversions:
- Add
Kilometre,MileandNauticalMilealongsideMetreandFoot - Update the
AltitudeConverterto do those new unit conversions
Then you can do the conversions directly on the data item and it will show the correct value:
ListQuantity {
//% "Range"
text: qsTrId("ev_range")
dataItem.uid: root.bindPrefix + "/RangeToGo"
dataItem.sourceUnit: Units.unitToVeUnit(VenusOS.Units_Kilometre)
dataItem.displayUnit: Units.unitToVeUnit(_systemDistanceUnit())
unit: root._systemDistanceUnit()
}
(See other uses of sourceUnit and displayUnit in gui-v2.)
|
Also, instead of "Device Page", use "EV" for the commit message subject (or "Units" for the unit-only commit) as this change does not generically apply to device pages in general. |
Removes altitude from constant names
f97a23f to
3c73552
Compare
3c73552 to
beea8cb
Compare
9263a7c to
5bf6768
Compare
5bf6768 to
f1ce8d8
Compare
Contributes to #2846