Fix SetDPC request and implement SetServer request and UI #3

rucoder · 2024-10-31T12:26:43Z

Add NTP server to IpDialog
handle IpNet correctly
Update to the latest ratatui/crossterm
Implement better panic handling
Disable some keystrokes in release mode e.g CTRL+a to trigger panic
many more

raw-model is really redundant and has to be removed Also remove dummy set_dpc function in preparation for real implementation Signed-off-by: Mikhail Malyshev <[email protected]>

Signed-off-by: Mikhail Malyshev <[email protected]>

- Also fix some tests. NOTE: not all tests are passed because test data has to be updated Signed-off-by: Mikhail Malyshev <[email protected]>

Signed-off-by: Mikhail Malyshev <[email protected]>

- Linux assigns a Node Local IPv6 to every interface. They are not interesting for end users so filter them out - fix a bug with IPs not displayed if only a single IP is assigned Signed-off-by: Mikhail Malyshev <[email protected]>

- Move IpDialog state into a separate struct - Save initial state to compare later and make decision whether we need to change settings or not Signed-off-by: Mikhail Malyshev <[email protected]>

- use subnet instead of hard-coded values - use NtpServer instead of NtpServers for static IP configuration Signed-off-by: Mikhail Malyshev <[email protected]>

- add human friendli panic printing in debug mode - add panic logging in release mode - rework TerminalWrapper to reset terminam settings on panic Signed-off-by: Mikhail Malyshev <[email protected]>

Signed-off-by: Mikhail Malyshev <[email protected]>

- it was used for debugging debug Signed-off-by: Mikhail Malyshev <[email protected]>

- ratatui is a false-positive since it is a package name Signed-off-by: Mikhail Malyshev <[email protected]>

- use serde_as to simplify deserialization. Especially IpNet from GO - get rid of manually written tests and load real json files from the device Signed-off-by: Mikhail Malyshev <[email protected]>

- also pass old and new state to understand the difference and make a decision Signed-off-by: Mikhail Malyshev <[email protected]>

- Called on CTRL+s - TODO: prompt user to reboot the device if it was already onboarded Signed-off-by: Mikhail Malyshev <[email protected]>

Signed-off-by: Mikhail Malyshev <[email protected]>

- it is better to use constants than magic numbers Signed-off-by: Mikhail Malyshev <[email protected]>

uncleDecart

Big PR, will need a second time to go through it, do I get it right, that some tests were removed completely?

uncleDecart · 2024-10-31T13:32:10Z

src/application.rs

@@ -38,7 +41,8 @@ pub struct Application {
    ui: Ui,
    // this is our model :)
    model: Rc<Model>,
-    raw_model: RawModel,
+    // pending requests
+    pending_requests: HashMap<u64, Rc<dyn Fn(&mut Application)>>,


Rc, because you're not sharing this via threads, right?

because dyn object doesn't have size. Rc does. you need something to wrap your closure into. Since we do not pass across threads Rc is enough

uncleDecart · 2024-10-31T13:36:57Z

src/application.rs

+                    debug!("Pending response for: {:?}", request);
+                    self.pending_requests.insert(*id, Rc::new(handle_response));
+                }
+                _ => {}


Do you want to send any message here through the channel or just Request one? Or you just want to put Requests inside pending_requests? If former then you need to print error and return here, if latter -- you might want to use

if let IpcMessage::Request(request, id) = msg { ... }

uncleDecart · 2024-10-31T13:38:29Z

src/application.rs

+                            },
+                        );
+                }
+                _ => {} // do nothing


print log message that there's some weird combination that is not handled?

yes, makes sense. In theory we could have DHCP->DHCP transition with manual DNS for example but it is not supported in UI anyways

True, I'd say better lay hay for future self in this case, things gets forgotten :) And in this case this won't add any overhead, neither computational, nor readability

uncleDecart · 2024-10-31T13:39:29Z

src/application.rs

+                }
+                _ => {} // do nothing
+            }
+            self.send_ipc_message(IpcMessage::new_request(Request::SetDPC(new_dpc)), |_| {});


so if nothing changes you still want to send message with "new" dpc, which hasn't been changed?

no, this function is called here

UiActions::AppAction(app_action) => match app_action { MonActions::NetworkInterfaceUpdated(old, new) => { debug!("Setting DPC for {}", &old.iface_name); debug!("OLD DPC: {:#?}", &old); debug!("NEW DPC: {:#?}", &new); if old == new { debug!("Not changed, not sending DPC"); } else { self.send_dpc(old, new); } self.ui.pop_layer(); } MonActions::ServerUpdated(url) => { debug!("Setting server URL to: {}", &url); self.send_ipc_message( IpcMessage::new_request(Request::SetServer(url.clone())), move |app| { app.model.borrow_mut().node_status.server = Some(url.clone()); }, ); self.ui.pop_layer(); } },

Okay, missed that, you handle it there, makes sense

uncleDecart · 2024-10-31T13:39:45Z

src/application.rs

@@ -363,6 +461,7 @@ impl Application {
        // send initial redraw event
        self.invalidate();

+        #[allow(unused_assignments)]


read the comment above.

uncleDecart · 2024-10-31T13:41:38Z

src/main.rs

@@ -50,6 +50,54 @@ fn init_logging() -> log2::Handle {
    handle
 }

+pub fn initialize_panic_handler() -> Result<()> {


👏 👏 👏

panic handlers are good thing

uncleDecart · 2024-10-31T13:42:56Z

src/ui/app_page.rs

@@ -173,7 +173,7 @@ impl IPresenter for ApplicationsPage {
        area: &ratatui::prelude::Rect,
        frame: &mut ratatui::Frame<'_>,
        model: &std::rc::Rc<Model>,
-        focused: bool,
+        _focused: bool,


why keep it?

because it is used in other implementation of this trait but not in this one

uncleDecart · 2024-10-31T13:46:16Z

src/ui/window.rs

@@ -90,11 +90,13 @@ impl<D> WindowBuilder<D> {
        self
    }

+    #[allow(dead_code)]


why do you want to keep them?

this is an API of UI widgets. I have plans for them

Okay, gotcha

OhmSpectator · 2024-11-01T09:37:38Z

I guess we should ask @jsfakian to take a look. At least he knows how to read rust code...

jsfakian · 2024-11-01T09:43:28Z

I guess we should ask @jsfakian to take a look. At least he knows how to read rust code...

I will do my best.

uncleDecart · 2024-11-01T10:00:06Z

src/ui/input_dialog.rs

+    hint: String,
+}
+
+fn on_init(w: &mut Window<InputDialogState>) {


Do you think that it makes sense to make input dialog as a trait? Say you want to input ip addr which has some checks to be valid, mac addr, which also has some other checks, etc. etc. It's more about near future implementations rather than current PR

@uncleDecart if that would be so simple... PRs are always welcome ;)

uncleDecart

LGTM, some questions which are more discussions and one fix with printing error.

- in theory crossterm shouldn't depend on libc but there is a bug when crossterm cannot be built with event-stream and dev-tty without libc Signed-off-by: Mikhail Malyshev <[email protected]>

- we need to update some fields in the model only if our request was successful - on error we can show a message to the user Signed-off-by: Mikhail Malyshev <[email protected]>

- this is a simple dialog with Label that can show a message to the user - do not allow changing of server URL if the device is onboarded Signed-off-by: Mikhail Malyshev <[email protected]>

- We do not want user to accidentally cause a panic or exit the application - Do not strip symbol names from release build to see readable stack trace Signed-off-by: Mikhail Malyshev <[email protected]>

Signed-off-by: Mikhail Malyshev <[email protected]>

- this tab is useful for debugging only - cleanup some warnings Signed-off-by: Mikhail Malyshev <[email protected]>

- Such transition may be valid if we ever support DHCP with e.g. static DNS. For now log an error Signed-off-by: Mikhail Malyshev <[email protected]>

uncleDecart · 2024-11-08T09:40:29Z

src/application.rs

+                debug!("Got response: {:?}", result);
+                match result {
+                    Ok(_) => {
+                        debug!("Response OK");


does debug! macros adds function name? Would be useful to have it IMO

uncleDecart · 2024-11-08T09:43:11Z

src/application.rs

@@ -48,6 +50,7 @@ impl Application {
        let terminal = TerminalWrapper::open_terminal()?;
        let mut ui = Ui::new(action_tx.clone(), terminal)?;
        let model = Rc::new(RefCell::new(MonitorModel::default()));


Why Rc<RefCell> can't it be just Rc<MonitorModel> ?

@uncleDecart no, we need interior mutability here or we cannot pass model to other functions because 'it is already borrowed as mut'.

uncleDecart · 2024-11-08T09:47:05Z

src/application.rs

@@ -137,20 +174,85 @@ impl Application {
                self.model.borrow_mut().update_app_list(app_list);
            }

+            IpcMessage::ZedAgentStatus(status) => {


I can't comment on code above, but there's IpcMessage::LedBlinkCounter(led) case which just prints that we have LedBlinkCounter, is that intended?

if you want to keep variable you can just write _led to silent compiler warning

uncleDecart · 2024-11-08T09:51:15Z

src/application.rs

+        let current_dpc = self.model.borrow().get_current_dpc().cloned();
+        if let Some(current_dpc) = current_dpc {
+            info!("send_dpc: Sending DPC for iface {}", &new.iface_name);
+            let dpc = current_dpc.clone();


you clone current_dpc twice here, once

let current_dpc = self.model.borrow().get_current_dpc().cloned();

once here, doesn't make sense to clone twice, maybe even to clone at all actually

uncleDecart · 2024-11-08T09:52:58Z

src/application.rs

@@ -310,7 +412,7 @@ impl Application {
    }

    fn create_terminal_task(&mut self) -> (JoinHandle<()>, CancellationToken) {
-        let mut terminal_event_stream = self.ui.terminal.get_stream();
+        let mut terminal_event_stream = TerminalWrapper::get_stream();


so now stream is singleton?

well, it always was de-facto. the ownership of the stream is pretty complex here but the real underlining stream is a singleton in crossterm. I can explain offline if you wish, very long explanation

uncleDecart · 2024-11-08T09:57:31Z

src/application.rs

+            UiActions::ChangeServer => {
+                let is_onboarded = self.model.borrow().node_status.is_onboarded();
+
+                if is_onboarded {


you're not using the variable here, why not

if self.model.borrow().node_status.is_onboarded() { // ... }

no specific reason really. I can change it

no reason :) i'll change

uncleDecart · 2024-11-08T10:00:20Z

src/application.rs

+                    self.send_ipc_message(
+                        IpcMessage::new_request(Request::SetServer(url.clone())),
+                        move |app| {
+                            app.model.borrow_mut().node_status.server = Some(url.clone());


clone is not needed, it's the last place where you use url variable in this branch

uncleDecart · 2024-11-08T10:06:08Z

src/model/model.rs

@@ -182,6 +192,7 @@ impl MonitorModel {
    }

    pub fn update_network_status(&mut self, net_status: DeviceNetworkStatus) {
+        self.dpc_key = Some(net_status.dpc_key.clone());


why clone dpc_key?

well, the answer to "why clone()..." question in 90% of cases is "to satisfy borrow checker". But this is a really good catch! I rewrote it without cloning.

uncleDecart · 2024-11-08T10:09:56Z

src/ui/ipdialog.rs

@@ -289,7 +324,7 @@ fn update_current_layout(w: &mut Window<IpDialogState>, rect: &Rect) {
    }
 }

-fn ip_dialog_layout(w: &mut Window<IpDialogState>, rect: &Rect, model: &Rc<Model>) {
+fn ip_dialog_layout(w: &mut Window<IpDialogState>, rect: &Rect, _model: &Rc<Model>) {


It's not a trait, right? Why keep model as a parameter?

because it is a function which is used as closure later. in .with_layout(....) call

uncleDecart · 2024-11-08T10:12:34Z

src/ui/networkpage.rs

        .collect::<Vec<_>>()
        .join("\n");

    // cell #3 IP address list
-    if height > 1 {
+    if height > 0 {


I'm not that good with the code base (yet :D) why 0 instead of 1?

this is a real bug here: if we have a single IP (height 1) we display ... nothing :)

uncleDecart

LGTM, some small comments removing unnecessary cloning and clarifications about code base :)

- remove two unnecessary clone() calls - remove verbose log Signed-off-by: Mikhail Malyshev <[email protected]>

rucoder requested review from OhmSpectator, uncleDecart and jsfakian October 31, 2024 12:26

rucoder added 17 commits October 31, 2024 14:03

Remove raw-model for good

3122bc1

raw-model is really redundant and has to be removed Also remove dummy set_dpc function in preparation for real implementation Signed-off-by: Mikhail Malyshev <[email protected]>

Update types to match the latest EVE

4fd5913

Signed-off-by: Mikhail Malyshev <[email protected]>

Add ZedAgentStatus handling

ee70e8a

- Also fix some tests. NOTE: not all tests are passed because test data has to be updated Signed-off-by: Mikhail Malyshev <[email protected]>

Optimize release build for size

13c5bc1

Signed-off-by: Mikhail Malyshev <[email protected]>

Fix typos

3385e78

Signed-off-by: Mikhail Malyshev <[email protected]>

Split Ipv4 and Ipv6

4254f1d

- Linux assigns a Node Local IPv6 to every interface. They are not interesting for end users so filter them out - fix a bug with IPs not displayed if only a single IP is assigned Signed-off-by: Mikhail Malyshev <[email protected]>

Use IpNet to represent subnet

71169ad

- Move IpDialog state into a separate struct - Save initial state to compare later and make decision whether we need to change settings or not Signed-off-by: Mikhail Malyshev <[email protected]>

Fix subnet and ntpservers fields usage

fc6187d

- use subnet instead of hard-coded values - use NtpServer instead of NtpServers for static IP configuration Signed-off-by: Mikhail Malyshev <[email protected]>

Add better panic handling

117afd1

- add human friendli panic printing in debug mode - add panic logging in release mode - rework TerminalWrapper to reset terminam settings on panic Signed-off-by: Mikhail Malyshev <[email protected]>

Cleanup unused imports

bb76db6

Signed-off-by: Mikhail Malyshev <[email protected]>

Remove obsolete MainWnd

cac07c4

- it was used for debugging debug Signed-off-by: Mikhail Malyshev <[email protected]>

Add .typos.toml

cce96d7

- ratatui is a false-positive since it is a package name Signed-off-by: Mikhail Malyshev <[email protected]>

Rework eve_types again

ecc6fd7

- use serde_as to simplify deserialization. Especially IpNet from GO - get rid of manually written tests and load real json files from the device Signed-off-by: Mikhail Malyshev <[email protected]>

Add NTP server field to ip_dialog

5b743c2

- also pass old and new state to understand the difference and make a decision Signed-off-by: Mikhail Malyshev <[email protected]>

Add dialog to change server URL

97ba375

- Called on CTRL+s - TODO: prompt user to reboot the device if it was already onboarded Signed-off-by: Mikhail Malyshev <[email protected]>

Cleanup cargo check warnings

248f255

Signed-off-by: Mikhail Malyshev <[email protected]>

Use constants from libc

90fbb73

- it is better to use constants than magic numbers Signed-off-by: Mikhail Malyshev <[email protected]>

rucoder force-pushed the rucoder/set-dpc-new branch from 529d673 to 8e6c46a Compare October 31, 2024 13:04

uncleDecart reviewed Oct 31, 2024

View reviewed changes

rucoder mentioned this pull request Oct 31, 2024

Add monitor application to EVE lf-edge/eve#4410

Merged

5 tasks

uncleDecart reviewed Nov 1, 2024

View reviewed changes

uncleDecart approved these changes Nov 1, 2024

View reviewed changes

rucoder added 3 commits November 6, 2024 17:12

Update crossterm and ratatui to the latest version

6d30d20

- in theory crossterm shouldn't depend on libc but there is a bug when crossterm cannot be built with event-stream and dev-tty without libc Signed-off-by: Mikhail Malyshev <[email protected]>

Implement callbacks for send_ipc_message

1865eaf

- we need to update some fields in the model only if our request was successful - on error we can show a message to the user Signed-off-by: Mikhail Malyshev <[email protected]>

Add message box dialog

32632f8

- this is a simple dialog with Label that can show a message to the user - do not allow changing of server URL if the device is onboarded Signed-off-by: Mikhail Malyshev <[email protected]>

rucoder added 3 commits November 6, 2024 17:12

Disable debugging key strokes in a release build

cc2529c

- We do not want user to accidentally cause a panic or exit the application - Do not strip symbol names from release build to see readable stack trace Signed-off-by: Mikhail Malyshev <[email protected]>

Cleanup some linter errors

9a6fe4c

Signed-off-by: Mikhail Malyshev <[email protected]>

Keep Home tab in debug build only

2a407bf

- this tab is useful for debugging only - cleanup some warnings Signed-off-by: Mikhail Malyshev <[email protected]>

rucoder force-pushed the rucoder/set-dpc-new branch from 8e6c46a to 2a407bf Compare November 6, 2024 16:19

Log error on DHCP-DHCP DPC transition

3c1c098

- Such transition may be valid if we ever support DHCP with e.g. static DNS. For now log an error Signed-off-by: Mikhail Malyshev <[email protected]>

rucoder requested a review from uncleDecart November 6, 2024 16:30

uncleDecart reviewed Nov 8, 2024

View reviewed changes

uncleDecart approved these changes Nov 8, 2024

View reviewed changes

Optimize some clone() calls

7314d10

- remove two unnecessary clone() calls - remove verbose log Signed-off-by: Mikhail Malyshev <[email protected]>

rucoder merged commit 79e1f65 into lf-edge:main Nov 18, 2024
2 checks passed

Fix SetDPC request and implement SetServer request and UI #3

Fix SetDPC request and implement SetServer request and UI #3

Conversation

rucoder commented Oct 31, 2024

uncleDecart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OhmSpectator commented Nov 1, 2024

jsfakian commented Nov 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uncleDecart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uncleDecart Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uncleDecart Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uncleDecart left a comment

Choose a reason for hiding this comment

uncleDecart Nov 8, 2024 •

edited

Loading

uncleDecart Nov 8, 2024 •

edited

Loading