-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes and improvements for UI #5
Conversation
rucoder
commented
Dec 9, 2024
- Fix incorrect server URL set if device is already onboarded. Due to error in UI the URL was set to error message if the device is onborded
- Add human readable source of network configuration
- App details were not displayed
- CPUs field may be null so make it Option<String> Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
- Display a source of network configuration based on the current DPC key name Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
- There is a copy-paste error and "SetURL" is always sent from dialog box which leads to incorrect server URL Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
- Add tests with AppStatus - Add tests with Response - Add tests with AppSummary Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
- Add handling for AppStatus, AppSummary and Response Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
@@ -1205,7 +1203,7 @@ pub struct VmConfig { | |||
pub extra_args: String, | |||
pub boot_loader: String, | |||
#[serde(rename = "CPUs")] | |||
pub cpus: String, | |||
pub cpus: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this similar to omitempty in golang? Like if it's missing I'll just have None and if I didn't have it it'd default value it (or raised the error?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uncleDecart not exactly equal to 'omitempty', this is for handling null, the filed can be 'null' in json and Option converts it to None, otherwise you'll get 'string expected but null found'. It also handles the situation when the field doesn't exist at all but the later case was covered by other means of serde_json crate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay got it, thanks!
|
||
let configuration_string = match dpc_key.as_str() { | ||
"zedagent" => "From controller".green(), | ||
"manual" => "Set by local user".yellow(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of curiosity, .green(), yellow(), .red() are all mixin function you wrote for a &str?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uncleDecart no, this trait is imported from ratatui crate :) but yes, it is implemented as you think
.style(Style::default().fg(Color::White)) | ||
.alignment(Alignment::Left); | ||
|
||
frame.render_widget(paragraph, rect); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, silly question about the approach, which is more philosophical: was there a particular reason for you to create separate render functions instead of just storing layouts, iterating them and calling render widgets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uncleDecart very log explanation. Actually there is an approach you described implemented in IWidget trait. Some components follow it but I use it only for dynamic views like input field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, most probably has to do with dynamic rendering and not wanting to deep dive into optimizations, i.e. when you don't need to redraw the whole layout, got it, thanks :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just questions out of curiosity
- Integrate the latest eve-monitor-rs application. Following issues were fixed - update test files - Fix incorrect action sent on dialog box 'ok' key - Display current DPC key name - Fix deserialization of AppInstanceStatus See PR lf-edge/eve-monitor-rs#5 for details Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
- Integrate the latest eve-monitor-rs application. Following issues were fixed - update test files - Fix incorrect action sent on dialog box 'ok' key - Display current DPC key name - Fix deserialization of AppInstanceStatus See PR lf-edge/eve-monitor-rs#5 for details Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>