Re: [PATCH installer 10/15] gui: network: add bond setup options to advanced options dialog

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

Thanks for the thorough review!

On 10/21/26 17:36, Lukas Sichert wrote:
> [..]
>> +    my $set_sensitive = sub {
>> +        my ($active) = @_;
>> +
>> +        $opts_grid->foreach(sub {
>> +            $_[0]->set_sensitive($active);
>> +        });
>> +
>> +        if ($active) {
>> +            # if activating all inputs, observe the rules applied by the mode change handler
>> +            $on_mode_changed->(Proxmox::Install::Config::get_mngmt_bond_opt('mode'));
>> +        }
> 
> 
> Currently the font of the label of the bond primary textfield and the
> hash policy field are not greyed out if they are deactivated. This is
> especially confusing for the bond primary, because greyed out textfield
> and sensitive textfield look really similar. I think this could either
> be done in the on_mode_changed function or even in the foreach loop
> above.

I'll implement that for v3, definitely makes things a bit more clear.

> 
> Also I am a bit confused as to why the on_mode changed function is
> called with a parameter, as it does not process one (see above).

Good catch, that's just some leftover from a previous iteration.

>> [..]
>> +            my $name =
>> +                $pinning_active
>> +                ? $name_mapping->{ $iface->{mac} }
>> +                : $iface->{name};
> 
> Here probably $mapping was intended to be used instead of $name_mapping,
> but I am not sure if this will actually refresh interface names,
> as the map is only set on OK.

Yep, should have been `$mapping` indeed.

> 
>> +            my $label = "$name ($iface->{mac}, $iface->{driver}, $iface->{state})";
>> +            $checkboxes->{ $iface->{mac} }->set_label($label);
>> +        }
>> +    };
>> +    $refresh_if_names->($gtk_state->{network_pinning_enabled});
>> +
> 
>> +    my ($device_active_map, $device_active_reverse_map) = ({}, {});
> 
>> -    my ($device_active_map, $device_active_reverse_map) = ({}, {});
> 
> I am not too sure about this refactor here. I think it should be done in
> a separate commit as it touches the non bonding setup as well. Also you
> use
> $device_active_map->{$i} = $label;
> compared to
> $device_active_map->{$i} = $name;
> before without changing the $device_change_handler.

I already removed that bit locally in my working copy for v3.
Initially this was due to some issue with the bond implementation, which 
was actually fixed with the

```
[..]
     if defined($device_active_reverse_map->{$nic})
```

snippet below - just didn't re-check it with that bit removed again.