[pve-devel] [PATCH v2 installer] fix #4869: Make management interface selection more verbose
Christoph Heiss
c.heiss at proxmox.com
Fri Oct 13 12:36:12 CEST 2023
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 at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
More information about the pve-devel
mailing list