* [pve-devel] [PATCH installer] tui: Add a cancel button to Advanced bootdisk options @ 2023-06-21 9:16 Maximiliano Sandoval 2023-06-21 10:32 ` Christoph Heiss 2023-06-21 10:40 ` Thomas Lamprecht 0 siblings, 2 replies; 4+ messages in thread From: Maximiliano Sandoval @ 2023-06-21 9:16 UTC (permalink / raw) To: pve-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH installer] tui: Add a cancel button to Advanced bootdisk options 2023-06-21 9:16 [pve-devel] [PATCH installer] tui: Add a cancel button to Advanced bootdisk options Maximiliano Sandoval @ 2023-06-21 10:32 ` Christoph Heiss 2023-06-21 10:40 ` Thomas Lamprecht 1 sibling, 0 replies; 4+ messages in thread From: Christoph Heiss @ 2023-06-21 10:32 UTC (permalink / raw) To: Proxmox VE development discussion 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 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH installer] tui: Add a cancel button to Advanced bootdisk options 2023-06-21 9:16 [pve-devel] [PATCH installer] tui: Add a cancel button to Advanced bootdisk options Maximiliano Sandoval 2023-06-21 10:32 ` Christoph Heiss @ 2023-06-21 10:40 ` Thomas Lamprecht 2023-06-21 11:21 ` Maximiliano Sandoval 1 sibling, 1 reply; 4+ messages in thread From: Thomas Lamprecht @ 2023-06-21 10:40 UTC (permalink / raw) To: Proxmox VE development discussion, Maximiliano Sandoval 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| { ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH installer] tui: Add a cancel button to Advanced bootdisk options 2023-06-21 10:40 ` Thomas Lamprecht @ 2023-06-21 11:21 ` Maximiliano Sandoval 0 siblings, 0 replies; 4+ messages in thread From: Maximiliano Sandoval @ 2023-06-21 11:21 UTC (permalink / raw) To: Thomas Lamprecht, Proxmox VE development discussion > 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| { ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-06-21 11:22 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-21 9:16 [pve-devel] [PATCH installer] tui: Add a cancel button to Advanced bootdisk options Maximiliano Sandoval 2023-06-21 10:32 ` Christoph Heiss 2023-06-21 10:40 ` Thomas Lamprecht 2023-06-21 11:21 ` Maximiliano Sandoval
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox