[pve-devel] [PATCH installer 0/4] tui: make disk options view tabbed on small screens

[Max Carrara]

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 <m.carrara at proxmox.com>
Tested-by: Max Carrara <m.carrara at proxmox.com>


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 <m.carrara at proxmox.com>
Tested-by: Max Carrara <m.carrara at proxmox.com>

>
> 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