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

[Filip Schauer <f.schauer@proxmox.com>]

On 13/10/2023 12:36, Christoph Heiss wrote:
> 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.


In the TUI installer, placing the symbol at the front changes the color
of the remaining text that comes after it. Resetting the color with
\x1b[1;30m is also not an option since it sets the text color to gray,
which makes it inconsistent with the selectable elements and hard to
read.


>> +        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
>>
>>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>