[CPP-297] Add INS settings confirmation popup #162
Conversation
Adds a popup dialog that asks the user to confirm that the INS mode should be changed, showing any necessary IMU settings changes if needed. Accepting this dialog will configure the IMU as needed, save the settings then reset the device.
|
|
||
| self.msg_sender.send( | ||
| MsgReset { | ||
| flags: 1, |
There was a problem hiding this comment.
What happens when flags are 0 and a MsgReset is sent? Just reset the device but not the settings?
There was a problem hiding this comment.
Just reset the device but not the settings?
Yep, exactly
console_backend/src/settings_tab.rs
Outdated
| ("imu", "imu_raw_output", SettingValue::Boolean(true)), | ||
| ("imu", "gyro_range", SettingValue::String("125".to_string())), | ||
| ("imu", "acc_range", SettingValue::String("8g".to_string())), | ||
| ("imu", "imu_rate", SettingValue::String("100".to_string())), |
There was a problem hiding this comment.
I think it would make sense to have these as constants either at the top of this file or in the constants.rs file so they are easier to find if someone needed to make changes.
There was a problem hiding this comment.
Definitely agree - I did try to do this initially but wasn't able to mark the array as static or const due to SettingValue::String values needing to be Strings. Do you know a nice way of doing this?
There was a problem hiding this comment.
There may be a way to do this with a lazy static but I could be wrong? https://docs.rs/lazy_static/1.4.0/lazy_static/
You could alternatively just make constants for the values true/125/8g/100 and use them here.
This may also be something you could create once in the constructor for the struct then just clone it as needed.
resources/Constants/Constants.qml
Outdated
|
|
||
| insSettingsPopup: QtObject { | ||
| readonly property var columnHeaders: ["Name", "Current Value", "Recommended Value"] | ||
| readonly property int dialogWidth: 500 |
There was a problem hiding this comment.
To me it seems like the third column is a little cramped. I could see possibly increasing the dialogwidth or renaming "Recommended Value" to "New Value" or something of the like.
There was a problem hiding this comment.
Alternatively you could play around with deriving the width. Possibly parent.width/2
There was a problem hiding this comment.
Good point, I've extended this by another 50 pixels now, which helps with the cramping. Having a fixed width sounds a bit nicer to me, as otherwise I think the width of the popup would be controlled by how the wide the main app is?
Use an alias to get to the dialog box directly, instead
2 participants
Adds a popup dialog that asks the user to confirm that the INS mode should be changed, showing any necessary IMU settings changes if needed. Accepting this dialog will configure the IMU as needed, save the settings then reset the device.
Compared to the old console, I've tried to streamline things a little here by combining the "do you wish to change IMU settings" and "do you want to save and restart" dialogs into the one dialog. I can't think of a reason that a user would only want to go through with only half of this process, so I think for simplicity (both in implementation and from a UI standpoint) it probably makes sense to combine.
The GIFs in the table below show this behaviour and compare it to how it was in the old console. The "Change INS mode" popup will display when the INS mode is turned off, or when it is turned on but no IMU settings need changing. The "Change INS mode + IMU settings" popup will display when the INS mode is turned on and IMU settings are needed to change.