From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 08C508444 for ; Wed, 21 Jun 2023 13:22:06 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DC6261C2CF for ; Wed, 21 Jun 2023 13:22:05 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 21 Jun 2023 13:22:05 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 07AE342195 for ; Wed, 21 Jun 2023 13:22:05 +0200 (CEST) Message-ID: <27e4a1ba-ba66-c7d0-4237-3aebefa78bcd@proxmox.com> Date: Wed, 21 Jun 2023 13:21:39 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Content-Language: en-US To: Thomas Lamprecht , Proxmox VE development discussion References: <20230621091609.38721-1-m.sandoval@proxmox.com> <3bb5c697-cad2-e9d6-0bd1-d043eb28a57d@proxmox.com> From: Maximiliano Sandoval In-Reply-To: <3bb5c697-cad2-e9d6-0bd1-d043eb28a57d@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.052 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.102 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [bootdisk.rs] Subject: Re: [pve-devel] [PATCH installer] tui: Add a cancel button to Advanced bootdisk options X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 21 Jun 2023 11:22:06 -0000 > 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 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 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 so adding a 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 >> --- >> 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>) >> &(*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| {