* [pve-devel] [PATCH v2 installer] fix #4869: Make management interface selection more verbose
@ 2023-10-12 13:02 Filip Schauer
2023-10-13 10:36 ` Christoph Heiss
0 siblings, 1 reply; 3+ messages in thread
From: Filip Schauer @ 2023-10-12 13:02 UTC (permalink / raw)
To: pve-devel
Display a circle symbol indicating whether a network interface is up or
not. Also show the MAC address of each interface in the
proxmox-tui-installer, as was already the case in the GUI installer.
Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
Changes sinve v1:
* Use a circle symbol to display the state.
GUI: green circle -> UP
GUI: hollow circle -> DOWN
TUI: green circle -> UP
TUI: red circle -> DOWN
* Show the MAC address of each network interface in the
proxmox-tui-installer
Proxmox/Install/RunEnv.pm | 2 ++
debian/control | 1 +
proxinstall | 4 +++-
proxmox-tui-installer/src/main.rs | 28 ++++++++++++++++++----------
proxmox-tui-installer/src/setup.rs | 2 ++
5 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/Proxmox/Install/RunEnv.pm b/Proxmox/Install/RunEnv.pm
index 9878f55..0dabe3c 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -61,6 +61,7 @@ sub query_cpu_hvm_support : prototype() {
# mac => <mac address>,
# index => <index>,
# name => <ifname>,
+# state => <state>,
# addresses => [
# family => <inet|inet6>,
# address => <mac address>,
@@ -99,6 +100,7 @@ my sub query_netdevs : prototype() {
$ifs->{$name} = {
index => $index,
name => $name,
+ state => $state,
mac => $mac,
};
$ifs->{$name}->{addresses} = \@valid_addrs if @valid_addrs;
diff --git a/debian/control b/debian/control
index 3d13019..f863c91 100644
--- a/debian/control
+++ b/debian/control
@@ -19,6 +19,7 @@ Homepage: https://www.proxmox.com
Package: proxmox-installer
Architecture: any
Depends: chrony,
+ fonts-noto-color-emoji,
geoip-bin,
libgtk3-perl,
libgtk3-webkit2-perl,
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})";
};
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}"
+ };
+
+ let interface_label = format!("{0} - {1} {2}", value.name, value.mac, interface_state_symbol);
+ 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,
+
pub mac: String,
#[serde(default)]
--
2.39.2
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH v2 installer] fix #4869: Make management interface selection more verbose
2023-10-12 13:02 [pve-devel] [PATCH v2 installer] fix #4869: Make management interface selection more verbose Filip Schauer
@ 2023-10-13 10:36 ` Christoph Heiss
2023-10-16 11:29 ` Filip Schauer
0 siblings, 1 reply; 3+ messages in thread
From: Christoph Heiss @ 2023-10-13 10:36 UTC (permalink / raw)
To: Proxmox VE development discussion
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
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH v2 installer] fix #4869: Make management interface selection more verbose
2023-10-13 10:36 ` Christoph Heiss
@ 2023-10-16 11:29 ` Filip Schauer
0 siblings, 0 replies; 3+ messages in thread
From: Filip Schauer @ 2023-10-16 11:29 UTC (permalink / raw)
To: pve-devel
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
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-10-16 11:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-10-12 13:02 [pve-devel] [PATCH v2 installer] fix #4869: Make management interface selection more verbose Filip Schauer
2023-10-13 10:36 ` Christoph Heiss
2023-10-16 11:29 ` Filip Schauer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox