Show battery SoC inside circular multi gauge on Brief Page #1788
Conversation
|
Still TODO: add settings to control when/whether to show, and also whether to show the icon+label above. Edit: those settings have been added now. |
chriadam
force-pushed
the
chriadam/battery-soc-brief-multigauge
branch
from
a7a4b1e to
8d1d52a
Compare
chriadam
requested review from
blammit,
DanielMcInnes and
MikeTrahearn-Qinetic
Will you be adding the settings as part of this PR, or in a different one? |
|
I did add them already, that comment is now out of date. Will remove it. |
| horizontalCenter: parent.horizontalCenter | ||
| } | ||
| font.pixelSize: root.isTemperature && gaugeCount > 2 ? Theme.font_briefPage_battery_percentage_pixelSize - 12 | ||
| : root.isTemperature && gaugeCount > 1 ? Theme.font_briefPage_battery_percentage_pixelSize - 8 // temperature units takes lots of space. |
There was a problem hiding this comment.
Should these pixel sizes be different Theme values, rather than ... - 12, ... - 8, ... + 4 etc? Do these offsets look right for a 7" screen?
There was a problem hiding this comment.
Currently, we don't have Theme values. Should we add them? I personally don't know that there's much need to. They all look correct in both geometries, yes.
There was a problem hiding this comment.
I don't like the millions of theme values, but I think it'd be better to have theme values than to use hardcoded constants. In the Figma there is a standard set of font sizes so it'd be great if we could use them, or add new ones that are slightly different.
There was a problem hiding this comment.
Or just use fitMode to fit to the available width?
There was a problem hiding this comment.
Is there room enough for 100% (four characters)?
There was a problem hiding this comment.
Re theme values, it would be better if the Figma design had fewer of them overall and that they were nice intervals of 4 or 8. Figma Variables can allow you to define numerical constants and then bind them to other things like border.large, font.small, spacing.cosy etc. That way, the constants can be "modified" for smaller screens and everything follows - and this can be checked in Figma very easily FIRST. It reduces the number of uncontrolled numerical values all over the theme and brings a focus on the design system's consistency over having lots of Theme.spacingInTheCaseOfThisSpecificThingy type properties.
Contributes to issue #1491
chriadam
force-pushed
the
chriadam/battery-soc-brief-multigauge
branch
from
8d1d52a to
7544415
Compare
|
There is no design, currently - I was asked to prototype something up, then show it to Serj for feedback and improvement. I will attach screenshots to the task and contact him. |
|
We can't merge this until we get proper feedback from Serj, and iterate it. Also, we need to get whatever properties are required, added into the base system. |
| horizontalCenter: parent.horizontalCenter | ||
| } | ||
| font.pixelSize: root.isTemperature && gaugeCount > 2 ? Theme.font_briefPage_battery_percentage_pixelSize - 12 | ||
| : root.isTemperature && gaugeCount > 1 ? Theme.font_briefPage_battery_percentage_pixelSize - 8 // temperature units takes lots of space. |
There was a problem hiding this comment.
I don't like the millions of theme values, but I think it'd be better to have theme values than to use hardcoded constants. In the Figma there is a standard set of font sizes so it'd be great if we could use them, or add new ones that are slightly different.
| } | ||
| } | ||
|
|
||
| ListRadioButtonGroup { |
There was a problem hiding this comment.
Could you split this out into a separate ListDeviceRadioButtonGroup or such? I know we don't need a device-picker elsewhere at the moment but it would be nice to have less code directly in this page and this is one of the better candidates for future reuse elsewhere.
| implicitHeight: label.visible ? icon.height + quantity.height : quantity.height | ||
| visible: (!root._valueDisplay.isValid || (root.gaugeCount <= root._valueDisplay.value)) && (_device.isValid && !isNaN(_device.value)) | ||
|
|
||
| property VeQuickItem _valueDisplay: VeQuickItem { |
There was a problem hiding this comment.
These don't need to be properties since the root is not a plain old QtObject.
There was a problem hiding this comment.
The number of times we have property blah inside a QtObject, it would be far more readable if we invented:
QMLObject.qml:
import QtQml
QtObject {
default property alias _children
property list<QtObject> _children
}
Then you could do for example:
QMLObject {
VeQuickItem {
id: valueDisplay
uid: Global.systemSettings.serviceUid + "/Settings/Gui/BriefView/CenterValueDisplay"
}
}
Which then brings some nice advantages:
- Much more readable
- Private object ids no longer needs an underscore (we only have one for _children we never directly use)
- Private objects is not [directly] accessible outside the component (unless you are misbehaving and use _children)
- The ambiguity of using the property name rather than the id of the object assigned to it is removed
- The potential for overwriting or shadowing the property is removed.
- No need for an inner QtObject to keep things private - and then, no need for a property to assign it
Even better: if the QMLObject was defined in C++ (not hard) then the property could be declared FINAL to avoid property shadowing of the _children property.
| } | ||
| model: AggregateDeviceModel { | ||
| sourceModels: [ | ||
| batteryDevices, |
There was a problem hiding this comment.
Can just use Global.allDevicesModel.batteryDevices instead of making a list here.
|
|
||
| VeQuickItem { | ||
| id: activeBatteryService | ||
| uid: Global.system.serviceUid + "/ActiveBatteryService" |
There was a problem hiding this comment.
What is the reason for needing to check the active battery service? I thought it would show up in the list of battery services but is that not the case?
| dataItem.uid: Global.systemSettings.serviceUid + "/Settings/Gui/BriefView/CenterValueDisplay" | ||
| updateDataOnClick: false // don't update data automatically. | ||
| optionModel: [ | ||
| //% "No center value displayed" |
There was a problem hiding this comment.
I don't think "mode" makes sense here as e.g. the user doesn't choose between two-gauge mode vs three-gauge mode, but I guess the terminology will be reworded when the Design is updated?
Also wonder if something like this would be nicer:
- None
- When gauges present Maximum [3] // spin box
There was a problem hiding this comment.
Do we really need separate settings for 1 gauge, 2 gauge etc? Could we just have a switch with on/off for whether the display and label should be shown? If the CenterValueDisplay uses a horizontal size-to-fit for the text, then it could be shown regardless of the number of gauges present.
| id: settingsModel | ||
| ListRadioButtonGroup { | ||
| //: Whether to show a value in the center of the circular gauges | ||
| //% "Center value display" |
There was a problem hiding this comment.
Would be nice to just have a single "Center display" list item, that takes you to another page with the various center display options.
|
|
||
| ListRadioButtonGroup { | ||
| //: Whether to show a descriptive label above the value in the center of the circular gauges | ||
| //% "Center value label" |
There was a problem hiding this comment.
It's not clear what the "display" vs "label" means in the settings page; would be nice to have bottomChildren captions for both of them to briefly expand on what these configurations do.
| horizontalCenter: parent.horizontalCenter | ||
| } | ||
| font.pixelSize: root.isTemperature && gaugeCount > 2 ? Theme.font_briefPage_battery_percentage_pixelSize - 12 | ||
| : root.isTemperature && gaugeCount > 1 ? Theme.font_briefPage_battery_percentage_pixelSize - 8 // temperature units takes lots of space. |
There was a problem hiding this comment.
Or just use fitMode to fit to the available width?
| source: root.isTemperature ? "qrc:/images/icon_temp_32.svg" : "qrc:/images/icon_battery_24.svg" | ||
| } | ||
| Label { | ||
| id: label |
There was a problem hiding this comment.
It's been mentioned to be about removing extra lines used just for readability so I'm just checking here; what's the QML coding style? New line after id only or not?
There was a problem hiding this comment.
Also do we keep spacing around operators for example ? * / etc?
There was a problem hiding this comment.
ShinyProgressArc: Consider changing
progressColor: Theme.color_darkOk,Theme.statusColorValue(gauges.status) to
progressColor: {Theme.color_darkOk; return Theme.statusColorValue(gauges.status);}
to get rid of the M30 "Don't use comma expressions" warning - or - consider why this is necessary and do it a different way.
| @@ -164,6 +164,11 @@ QtObject { | |||
| { type: VenusOS.Tank_Type_FreshWater, level: 75, capacity: 20 }, | |||
| ] | |||
| }, | |||
| { | |||
| name: "No tanks", | |||
There was a problem hiding this comment.
"yes peas" ;-)
| currentIndex: { | ||
| let systemBatteryIndex = -1 | ||
| let i | ||
| for (i = 0; i < devices.length; ++i) { |
There was a problem hiding this comment.
consider moving the scope of access of "i" inside the for loop:
'for (let i = 0; i < devices.length; ++i) {'
else "i" can be used outside the loop which doesn't look like the intention.
|
|
||
| Component.onCompleted: { | ||
| var tempDevices = devices | ||
| devices = tempDevices // force update signal. |
There was a problem hiding this comment.
Be careful! QTBUG-131901 has been fixed so this code is a ticking time bomb. Please consider a different means of forcing the update signal if required.
Contributes to issue #1491