[pve-devel] [PATCH installer] tui: Add a cancel button to Advanced bootdisk options

[pve-devel] [PATCH installer] tui: Add a cancel button to Advanced bootdisk options

[Maximiliano Sandoval]

This matches the GUI installer which counts with a close (x) button.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 proxmox-tui-installer/src/views/bootdisk.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 3fdbe5b..eaf343d 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -456,6 +456,7 @@ fn advanced_options_view(disks: &[Disk], options: Rc<RefCell<BootdiskOptions>>)
         &(*options).borrow(),
     ))
     .title("Advanced bootdisk options")
+    .dismiss_button("Cancel")
     .button("Ok", {
         let options_ref = options.clone();
         move |siv| {
-- 
2.39.2

Re: [pve-devel] [PATCH installer] tui: Add a cancel button to Advanced bootdisk options

[Christoph Heiss]

LGTM.

On Wed, Jun 21, 2023 at 11:16:09AM +0200, Maximiliano Sandoval wrote:
> This matches the GUI installer which counts with a close (x) button.
>

Reviewed-by: Christoph Heiss <c.heiss@proxmox.com>
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  proxmox-tui-installer/src/views/bootdisk.rs | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
> index 3fdbe5b..eaf343d 100644
> --- a/proxmox-tui-installer/src/views/bootdisk.rs
> +++ b/proxmox-tui-installer/src/views/bootdisk.rs
> @@ -456,6 +456,7 @@ fn advanced_options_view(disks: &[Disk], options: Rc<RefCell<BootdiskOptions>>)
>          &(*options).borrow(),
>      ))
>      .title("Advanced bootdisk options")
> +    .dismiss_button("Cancel")
>      .button("Ok", {
>          let options_ref = options.clone();
>          move |siv| {
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>

Re: [pve-devel] [PATCH installer] tui: Add a cancel button to Advanced bootdisk options

[Thomas Lamprecht]

Am 21/06/2023 um 11:16 schrieb Maximiliano Sandoval:
> This matches the GUI installer which counts with a close (x) button.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  proxmox-tui-installer/src/views/bootdisk.rs | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
> index 3fdbe5b..eaf343d 100644
> --- a/proxmox-tui-installer/src/views/bootdisk.rs
> +++ b/proxmox-tui-installer/src/views/bootdisk.rs
> @@ -456,6 +456,7 @@ fn advanced_options_view(disks: &[Disk], options: Rc<RefCell<BootdiskOptions>>)
>          &(*options).borrow(),
>      ))
>      .title("Advanced bootdisk options")
> +    .dismiss_button("Cancel")

meh, this focuses first, before the Ok button, which is just makes the existing
non-ideal UX w.r.t. focus priority of buttons worse, so for now I rather have no
such button - user can simply press OK, which is also a bit easier to have a
clear understanding that the entered values are actually the ones then used.
As with a cancel we really need to ensure that no callback has already changed
data, not sure for the TUI, but the GTK UI would need quite some extra handling
here.

>      .button("Ok", {
>          let options_ref = options.clone();
>          move |siv| {

Re: [pve-devel] [PATCH installer] tui: Add a cancel button to Advanced bootdisk options

[Maximiliano Sandoval]

> meh, this focuses first, before the Ok button, which is just makes the existing
> non-ideal UX

They way I see it, changing the existing settings for a disk is quite a high-commitment action. In such cases I would generally focus by default the button associated to the negative action, so yes it will require an extra TAB to get to the <Ok> button but I think it makes sense considering the consequences of the action.

Note that from a UX perspective the user cannot tell that the dialog starts with the current values (since the dialog is obscuring the previous values) and that pressing <Ok> will have no consequences if no field has been changed, and in the case they have changed the values they have no way to go back to the previous values if they have forgotten them. Or at least that was my experience.

> As with a cancel we really need to ensure that no callback has already changed
> data, not sure for the TUI, but the GTK UI would need quite some extra handling
> here

At the moment closing the advanced options in the GUI (it has its x button visible) will just close the dialog, just as this patch. The only line that changes something before the uses closes or activates the "Ok" button are

```
     $fstypecb->signal_connect (changed => sub {
	my $new_filesys = $fstypecb->get_active_text();
	Proxmox::Install::Config::set_filesys($new_filesys);
	&$switch_view();
     });
```
I would argue that the config changes should only apply after the user confirms the changes by activating the "Ok" button. As far as I understand the TUI installer at the moment won't change anything, e.g. via some callback, until the user presses <Ok> so adding a <Cancel> button that just closes the dialog won't do any bad.

On 6/21/23 12:40, Thomas Lamprecht wrote:
> Am 21/06/2023 um 11:16 schrieb Maximiliano Sandoval:
>> This matches the GUI installer which counts with a close (x) button.
>>
>> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
>> ---
>>   proxmox-tui-installer/src/views/bootdisk.rs | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
>> index 3fdbe5b..eaf343d 100644
>> --- a/proxmox-tui-installer/src/views/bootdisk.rs
>> +++ b/proxmox-tui-installer/src/views/bootdisk.rs
>> @@ -456,6 +456,7 @@ fn advanced_options_view(disks: &[Disk], options: Rc<RefCell<BootdiskOptions>>)
>>           &(*options).borrow(),
>>       ))
>>       .title("Advanced bootdisk options")
>> +    .dismiss_button("Cancel")
> meh, this focuses first, before the Ok button, which is just makes the existing
> non-ideal UX w.r.t. focus priority of buttons worse, so for now I rather have no
> such button - user can simply press OK, which is also a bit easier to have a
> clear understanding that the entered values are actually the ones then used.
> As with a cancel we really need to ensure that no callback has already changed
> data, not sure for the TUI, but the GTK UI would need quite some extra handling
> here.
>
>>       .button("Ok", {
>>           let options_ref = options.clone();
>>           move |siv| {