Re: [pve-devel] [PATCH v2 installer] fix #4869: Make management interface selection more verbose

[Christoph Heiss <c.heiss@proxmox.com>]

See inline comments. Also, `cargo fmt` please :^)

On Thu, Oct 12, 2023 at 03:02:08PM +0200, Filip Schauer wrote:
> [..]
> diff --git a/proxinstall b/proxinstall
> index d5b2565..51170cd 100755
> --- a/proxinstall
> +++ b/proxinstall
> @@ -347,7 +347,9 @@ sub create_ipconf_view {
>
>      my $get_device_desc = sub {
>  	my $iface = shift;
> -	return "$iface->{name} - $iface->{mac} ($iface->{driver})";
> +	my $iface_state_symbol = "\N{U+25EF}";
> +	$iface_state_symbol = "\N{U+1F7E2}" if ($iface->{state} eq "UP");
> +	return "$iface_state_symbol $iface->{name} - $iface->{mac} ($iface->{driver})";
^ Here we have e.g. "<symbol> eno1 - 12:34:56:78:9a:bc (r8169)".

>      };
>
>      my $run_env = Proxmox::Install::RunEnv::get();
> diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
> index 3f01713..7e46f33 100644
> --- a/proxmox-tui-installer/src/main.rs
> +++ b/proxmox-tui-installer/src/main.rs
> @@ -548,20 +548,28 @@ fn password_dialog(siv: &mut Cursive) -> InstallerView {
>  fn network_dialog(siv: &mut Cursive) -> InstallerView {
>      let state = siv.user_data::<InstallerState>().unwrap();
>      let options = &state.options.network;
> -    let ifnames = state.runtime_info.network.interfaces.keys();
> +
> +    let mut management_interface_select_view = SelectView::new().popup();
> +    for (key, value) in state.runtime_info.network.interfaces.iter() {
> +        let interface_state_symbol = if value.state == "UP" {
> +            "\x1b[1;92m\u{25CF}"
> +        } else {
> +            "\x1b[1;31m\u{25CF}"
> +        };
One thing I noticed while testing the TUI installer is the
setting of the color here fsck up the colors of the other entries.
Colors/effects applied using ANSI escape sequences do not reset
automatically, so this must be done after the unicode symbol manually
using "\x1b[1;30m".

E.g. with multiple NICS, nics in UP are sometimes displayed entirely in
green, in non-UP entirely red. And scrolling through the list changes
that completely.

[ To properly display colors in Cursive, you'd normally use a
  `StyledString`, but - after quickly trying it out - seems broken for
  SelectView (not sure why, I might investigate that some time). ]

So although a bit hacky, seems to work here fine (still seems a bit
broken under VGA for me, but that is just some random artifact it seems,
as 30 is the correct color code in any case).

> +
> +        let interface_label = format!("{0} - {1} {2}", value.name, value.mac, interface_state_symbol);
^ And here we have "eno1 - 12:34:56:78:9a:bc <symbol>".

Just for the sake of consistency they should be the same between GUI and
TUI.
I would propose "<symbol> <name> - <mac>", and just drop the driver part
completely in the GUI. The latter does not provide any real value IMO,
at least not I could come up with any.

> +        management_interface_select_view.add_item(interface_label, key.to_string());
> +    }
>
>      let inner = FormView::new()
>          .child(
>              "Management interface",
> -            SelectView::new()
> -                .popup()
> -                .with_all_str(ifnames.clone())
> -                .selected(
> -                    ifnames
> -                        .clone()
> -                        .position(|ifname| ifname == &options.ifname)
> -                        .unwrap_or_default(),
> -                ),
> +            management_interface_select_view.selected(
> +                state.runtime_info.network.interfaces.keys()
> +                    .clone()
> +                    .position(|ifname| ifname == &options.ifname)
> +                    .unwrap_or_default(),
> +            ),
>          )
>          .child(
>              "Hostname (FQDN)",
> diff --git a/proxmox-tui-installer/src/setup.rs b/proxmox-tui-installer/src/setup.rs
> index 942e319..dbff3da 100644
> --- a/proxmox-tui-installer/src/setup.rs
> +++ b/proxmox-tui-installer/src/setup.rs
> @@ -443,6 +443,8 @@ pub struct Interface {
>
>      pub index: usize,
>
> +    pub state: String,
This can be an enum for cleanliness, as this field is well defined. All
possible states as recognized (and thus can be put out) by iproute2 are
defined here:
https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/ip/ipaddress.c?h=v6.5.0#n119

> +
>      pub mac: String,
>
>      #[serde(default)]
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>