From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 4EFEC1FF2A8 for ; Tue, 2 Jul 2024 18:49:32 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9CEF1324FC; Tue, 2 Jul 2024 18:49:46 +0200 (CEST) Mime-Version: 1.0 Date: Tue, 02 Jul 2024 18:49:12 +0200 Message-Id: To: "Proxmox VE development discussion" From: "Max Carrara" X-Mailer: aerc 0.17.0-72-g6a84f1331f1c References: <20240613115318.842583-1-c.heiss@proxmox.com> In-Reply-To: <20240613115318.842583-1-c.heiss@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.029 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 SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [mod.rs, bootdisk.rs] Subject: Re: [pve-devel] [PATCH installer 0/4] tui: make disk options view tabbed on small screens 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: , Reply-To: Proxmox VE development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" On Thu Jun 13, 2024 at 1:53 PM CEST, Christoph Heiss wrote: > This adds a tabbed view component, for usage in the advanced disk > options dialog when selecting ZFS or Btrfs. Works pretty much the same > as its GUI counterpart, as much as that is possible. > > It's currently only activated for small (<=80 columns) displays, to make > disk selection a lot more usable in these cases. This mostly affects > serial console installation, but possibly also installations using a > virtual screen via IPMI/BMC. > > Testing can be done using the `stty` to set specific terminal sizes, > e.g. `stty columns 80 rows 24` for a standard VT100-spec terminal. > > This componont/view may also be made the default for the advanced disk > options dialog, to align the TUI with it GUI in more cases - I'm open > for discussion on that. Would also simplify the code a lot, so there > are certainly other benefits to it as well. Pretty neat! I like this a lot. See my entire feedback below. Leaving these here already so they don't get lost ;) Reviewed-by: Max Carrara Tested-by: Max Carrara Building -------- * Changes applied cleanly, no issues here * Submitted code is formatted using `cargo fmt`, very nice * `cargo clippy` also didn't complain, very nice Code Review ----------- All changes are easy to follow, no surprises here. There's nothing that I have an issue with, pretty solid work! Also nice to see some comments and docstrings that provide actual context, explaining what something is for or why it is done. These kinds of things might seem small, but help others that aren't as familiar with the installer code (like me, e.g.) to grasp what's going on without having to "discover" everything themselves. Very much appreciated! Testing ------- * tty size: 60x15 - tabs work as intended, but the actual contents of the tabs get lost for ZFS and BTRFS configurations, because there's not enough vertical space - it's (unsurprisingly) still possible to navigate through those options, they're just not visible - my suggestion would be to add some kind of scrolling view here, though I'm not sure if we're aiming to support ttys that are *this* small * tty size: 60x20 - same as above, except that the first (four) configuration options in the advanced disk config tab are displayed, the other ones are still lost * tty size: 80x24 - works pretty great, as there now is enough vertical space to display everything - haven't tested with multiple disks, but if a user wanted to e.g. use ZFS with *a lot* of disks, there's still a risk that they might run out of vertical space in the disk selection tab * tty size: 81x24 - feels awkward (as it does without this series :P) because the right column in the advanced disk config tab gets squeezed (squoze?) too much - as you suggested above, the tab view could therefore IMO be made the default in general, it just looks really neat overall; I personally prefer it quite a lot over the old one Summary ------- Great work, great code, great results - can't really say more than that, can I? As mentioned before, the tab view could IMO be the new default for the disk stuff. One thing that should perhaps still be addressed is the case of there not being enough vertical space when the tty's height is rather small. I recognize however that this isn't really in the scope of this series, so unless someone else wants to chime in, this can be applied as-is. Reviewed-by: Max Carrara Tested-by: Max Carrara > > Christoph Heiss (4): > tui: fix some comment typos > tui: bootdisk: align btrfs dialog interface with zfs equivalent > tui: views: add new TabbedView component > tui: bootdisk: use tabbed view for disk options on small screens > > proxmox-tui-installer/src/views/bootdisk.rs | 260 +++++++++++------- > proxmox-tui-installer/src/views/mod.rs | 3 + > .../src/views/tabbed_view.rs | 196 +++++++++++++ > 3 files changed, 358 insertions(+), 101 deletions(-) > create mode 100644 proxmox-tui-installer/src/views/tabbed_view.rs _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel