Re: [pve-devel] [PATCH installer 14/14] gui: add support for pinning network interface names

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

Thanks for the review!

On Tue Oct 14, 2025 at 5:04 PM CEST, Maximiliano Sandoval wrote:
> Christoph Heiss <c.heiss@proxmox.com> writes:
>
> Some comments bellow:
>> diff --git a/proxinstall b/proxinstall
>> index 5ba65fa..35e948a 100755
>> --- a/proxinstall
>> +++ b/proxinstall
[..]
>> +my sub create_network_interface_pin_view {
>> +    my ($done_cb) = @_;
>> +
>> +    my $dialog = Gtk3::Dialog->new();
>> +    $dialog->set_title('Interface Name Pinning Options');
>> +    $dialog->add_button('_OK', 1);
>
> The response argument is indeed a number (gint), but there is an enum
> [1] for this. In perl one can use the string 'ok' instead of
> GTK_RESPONSE_OK, for example.

I'll change it to 'ok', thanks!

>
> I do not see the value of the response being used during the
> `GtkDialog::response` signal handler, note that a dialog can be closed
> either be pressing ESC, clicking the X button, or by clicking the `OK`
> button as per the callback bellow. As it stands, all the methods I
> described above would run the handler equally, is this intended?

I tried to be consistent with the advanced disk dialog, which has the
exact same behaviour - so yes.

[..]
>> +
>> +    $scrolled_window->add($grid);
>> +    $scrolled_window->set_policy('never', 'automatic');
>> +    $scrolled_window->set_visible(1);
>
> The scrolled window is the child of hbox and gtk_widget_show_all is
> called on the later, it should not be necessary to call
> gtk_widget_set_visible on this one.

I see, I'll remove it.

>
>> +    $scrolled_window->set_min_content_height(200);
>> +    $scrolled_window->set_margin_end(10);
>
> It is a bit asymmetrical that there is no margin on the start.

Right, thanks for noticing!

[..]
>> +
>> +    $dialog->show();
>> +    $dialog->run();
>
> There are two ways to present dialogs, either by running
> `gtk_dialog_run` which will block until the dialog is done and will
> return the response (deprecated) and then close/destroy the dialog, or
> connect to the response signal which will be emitted once there is a
> response and the dialog can be closed (as done above) but instead of
> calling `gtk_dialog_run()` one would call `gtk_window_present()` on it.
> So please run `present` instead of `run` here.

Will do in v2.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel