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

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

[Christoph Heiss]

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.

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

-- 
2.44.1



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer 1/4] tui: fix some comment typos

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-tui-installer/src/views/bootdisk.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index f6fdb31..1c7b2a4 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -29,7 +29,7 @@ use proxmox_installer_common::{
 /// https://openzfs.github.io/openzfs-docs/Performance%20and%20Tuning/Module%20Parameters.html#zfs-arc-max
 const ZFS_ARC_MIN_SIZE_MIB: usize = 64; // MiB
 
-/// Convience wrapper when needing to take a (interior-mutable) reference to `BootdiskOptions`.
+/// Convenience wrapper when needing to take a (interior-mutable) reference to `BootdiskOptions`.
 /// Interior mutability is safe for this case, as it is completely single-threaded.
 pub type BootdiskOptionsRef = Rc<RefCell<BootdiskOptions>>;
 
@@ -165,9 +165,9 @@ impl AdvancedBootdiskOptionsView {
         Self { view }
     }
 
-    /// Called when a new filesystem type is choosen by the user.
+    /// Called when a new filesystem type is chosen by the user.
     /// It first creates the inner (filesystem-specific) options view according to the selected
-    /// filesytem type.
+    /// filesystem type.
     /// Further, it replaces the (outer) bootdisk selector in the main dialog, either with a
     /// selector for LVM configurations or a simple label displaying the chosen RAID for ZFS and
     /// Btrfs configurations.
-- 
2.44.1



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer 2/4] tui: bootdisk: align btrfs dialog interface with zfs equivalent

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-tui-installer/src/views/bootdisk.rs | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 1c7b2a4..29a2854 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -158,7 +158,7 @@ impl AdvancedBootdiskOptionsView {
                 view.add_child(ZfsBootdiskOptionsView::new(runinfo, zfs, &product_conf))
             }
             AdvancedBootdiskOptions::Btrfs(btrfs) => {
-                view.add_child(BtrfsBootdiskOptionsView::new(&runinfo.disks, btrfs))
+                view.add_child(BtrfsBootdiskOptionsView::new(runinfo, btrfs))
             }
         };
 
@@ -210,7 +210,7 @@ impl AdvancedBootdiskOptionsView {
                         &product_conf,
                     )),
                     FsType::Btrfs(_) => {
-                        view.add_child(BtrfsBootdiskOptionsView::new_with_defaults(&runinfo.disks))
+                        view.add_child(BtrfsBootdiskOptionsView::new_with_defaults(&runinfo))
                     }
                 }
             }
@@ -520,9 +520,9 @@ struct BtrfsBootdiskOptionsView {
 }
 
 impl BtrfsBootdiskOptionsView {
-    fn new(disks: &[Disk], options: &BtrfsBootdiskOptions) -> Self {
+    fn new(runinfo: &RuntimeInfo, options: &BtrfsBootdiskOptions) -> Self {
         let view = MultiDiskOptionsView::new(
-            disks,
+            &runinfo.disks,
             &options.selected_disks,
             FormView::new().child("hdsize", DiskSizeEditView::new().content(options.disk_size)),
         )
@@ -531,8 +531,11 @@ impl BtrfsBootdiskOptionsView {
         Self { view }
     }
 
-    fn new_with_defaults(disks: &[Disk]) -> Self {
-        Self::new(disks, &BtrfsBootdiskOptions::defaults_from(disks))
+    fn new_with_defaults(runinfo: &RuntimeInfo) -> Self {
+        Self::new(
+            runinfo,
+            &BtrfsBootdiskOptions::defaults_from(&runinfo.disks),
+        )
     }
 
     fn get_values(&mut self) -> Option<(Vec<Disk>, BtrfsBootdiskOptions)> {
-- 
2.44.1



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer 3/4] tui: views: add new TabbedView component

[Christoph Heiss]

Add a tabbed view component, for usage in the advanced disk options
dialog when selecting ZFS or Btrfs (for now). Works pretty much the same
as its GUI counterpart, as much as that is possible.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-tui-installer/src/views/mod.rs        |   3 +
 .../src/views/tabbed_view.rs                  | 196 ++++++++++++++++++
 2 files changed, 199 insertions(+)
 create mode 100644 proxmox-tui-installer/src/views/tabbed_view.rs

diff --git a/proxmox-tui-installer/src/views/mod.rs b/proxmox-tui-installer/src/views/mod.rs
index 3244e76..a098d1f 100644
--- a/proxmox-tui-installer/src/views/mod.rs
+++ b/proxmox-tui-installer/src/views/mod.rs
@@ -15,6 +15,9 @@ pub use bootdisk::*;
 mod install_progress;
 pub use install_progress::*;
 
+mod tabbed_view;
+pub use tabbed_view::*;
+
 mod table_view;
 pub use table_view::*;
 
diff --git a/proxmox-tui-installer/src/views/tabbed_view.rs b/proxmox-tui-installer/src/views/tabbed_view.rs
new file mode 100644
index 0000000..2aeea6a
--- /dev/null
+++ b/proxmox-tui-installer/src/views/tabbed_view.rs
@@ -0,0 +1,196 @@
+use std::borrow::{Borrow, BorrowMut};
+
+use cursive::{
+    direction::Direction,
+    event::{AnyCb, Event, EventResult, Key},
+    theme::{ColorStyle, PaletteColor},
+    utils::{markup::StyledString, span::SpannedStr},
+    view::{CannotFocus, IntoBoxedView, Selector, ViewNotFound},
+    Printer, Vec2, View,
+};
+
+pub struct TabbedView {
+    /// All tab views in format (name, view)
+    views: Vec<(String, Box<dyn View>)>,
+    /// Currently active tab index
+    current: usize,
+    /// Whether the tab bar has focus currently
+    bar_has_focus: bool,
+}
+
+impl TabbedView {
+    /// Creates a view with multiple tabs.
+    pub fn new() -> Self {
+        Self {
+            views: vec![],
+            current: 0,
+            bar_has_focus: false,
+        }
+    }
+
+    /// Adds a tab to the view. The `name` is the string displayed at the top.
+    ///
+    /// Chainable variant.
+    pub fn tab<V>(mut self, name: &str, view: V) -> Self
+    where
+        V: 'static + IntoBoxedView,
+    {
+        assert!(!name.is_empty());
+        self.views.push((name.to_owned(), view.into_boxed_view()));
+        self
+    }
+
+    /// Returns a reference to the specified tab content view.
+    pub fn get(&self, index: usize) -> Option<&dyn View> {
+        self.views.get(index).map(|(_, view)| view.borrow())
+    }
+
+    /// Returns a mutable reference to the specified tab content view.
+    pub fn get_mut(&mut self, index: usize) -> Option<&mut dyn View> {
+        self.views.get_mut(index).map(|(_, view)| view.borrow_mut())
+    }
+
+    /// Draws the border around the tab view and the name header for each tab.
+    fn draw_border(&self, p: &Printer) {
+        let names_len: usize = self.views.iter().map(|(name, _)| name.len() + 2).sum();
+        let tabbar_width = names_len + self.views.len() + 1;
+
+        let top_border_width = (p.output_size.x - tabbar_width) / 2;
+        p.print_box((0, 0), p.output_size, false);
+
+        self.print_tab_names(p.offset((top_border_width, 0)));
+    }
+
+    /// Draws all tab names with appropriate highlighting, depending on its state,
+    /// to the specified printer `p` at `(0, 0)`.
+    fn print_tab_names(&self, p: Printer) {
+        let mut pos = Vec2::zero();
+        for (index, name) in self.views.iter().map(|(name, _)| name).enumerate() {
+            p.print(pos, "| ");
+            pos.x += 2;
+
+            if index == self.current {
+                self.print_active_tab_name(name, p.offset(pos));
+            } else {
+                p.print(pos, name);
+            }
+
+            pos.x += name.len();
+            p.print(pos, " ");
+            pos.x += 1;
+
+            p.print(pos, "|");
+        }
+    }
+
+    /// Draws the active tab name to the printer `p`, with its highlighting
+    /// additionally depending upon whether the tab bar currently has focus or not.
+    fn print_active_tab_name(&self, name: &str, p: Printer) {
+        let background = if self.bar_has_focus {
+            PaletteColor::Highlight
+        } else {
+            PaletteColor::HighlightInactive
+        };
+
+        p.print_styled(
+            (0, 0),
+            SpannedStr::from(&StyledString::styled(
+                name,
+                ColorStyle::new(PaletteColor::HighlightText, background),
+            )),
+        )
+    }
+}
+
+impl View for TabbedView {
+    fn draw(&self, printer: &Printer) {
+        self.draw_border(printer);
+
+        if let Some(view) = self.get(self.current) {
+            view.draw(&printer.offset((1, 1)).focused(!self.bar_has_focus));
+        }
+    }
+
+    fn layout(&mut self, size: Vec2) {
+        for (_, view) in self.views.iter_mut() {
+            view.layout(size.saturating_sub((2, 2)));
+        }
+    }
+
+    fn required_size(&mut self, constraint: Vec2) -> Vec2 {
+        if let Some(view) = self.get_mut(self.current) {
+            view.required_size(constraint) + (2, 2)
+        } else {
+            constraint
+        }
+    }
+
+    fn on_event(&mut self, event: Event) -> EventResult {
+        match event {
+            Event::Key(Key::Right) if self.bar_has_focus => {
+                self.current = (self.current + 1) % self.views.len();
+                EventResult::consumed()
+            }
+            Event::Key(Key::Left) if self.bar_has_focus => {
+                self.current = if self.current == 0 {
+                    self.views.len() - 1
+                } else {
+                    self.current - 1
+                };
+                EventResult::consumed()
+            }
+            Event::Key(Key::Down) if self.bar_has_focus => {
+                self.bar_has_focus = false;
+                self.get_mut(self.current)
+                    .and_then(|v| v.take_focus(Direction::up()).ok())
+                    .unwrap_or(EventResult::Ignored)
+            }
+            Event::Key(Key::Up) if self.bar_has_focus => EventResult::Ignored,
+            Event::FocusLost if self.bar_has_focus => {
+                self.bar_has_focus = false;
+                EventResult::consumed()
+            }
+            Event::Key(Key::Up) if !self.bar_has_focus => {
+                let result = self
+                    .get_mut(self.current)
+                    .map(|v| v.on_event(event))
+                    .unwrap_or(EventResult::Ignored);
+
+                match result {
+                    EventResult::Ignored => {
+                        self.bar_has_focus = true;
+                        if let Some(view) = self.get_mut(self.current) {
+                            view.on_event(Event::FocusLost);
+                        }
+                        EventResult::consumed()
+                    }
+                    ev => ev,
+                }
+            }
+            _ if !self.bar_has_focus => self
+                .get_mut(self.current)
+                .map(|v| v.on_event(event))
+                .unwrap_or_else(EventResult::consumed),
+            _ => EventResult::Ignored,
+        }
+    }
+
+    fn call_on_any(&mut self, selector: &Selector, callback: AnyCb) {
+        for (_, view) in &mut self.views {
+            view.call_on_any(selector, callback);
+        }
+    }
+
+    fn focus_view(&mut self, selector: &Selector) -> Result<EventResult, ViewNotFound> {
+        if let Some(view) = self.get_mut(self.current) {
+            view.focus_view(selector)
+        } else {
+            Err(ViewNotFound)
+        }
+    }
+
+    fn take_focus(&mut self, direction: Direction) -> Result<EventResult, CannotFocus> {
+        self.bar_has_focus = direction == Direction::up();
+        Ok(EventResult::consumed())
+    }
+}
-- 
2.44.1



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH installer 4/4] tui: bootdisk: use tabbed view for disk options on small screens

[Christoph Heiss]

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.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-tui-installer/src/views/bootdisk.rs | 239 ++++++++++++--------
 1 file changed, 147 insertions(+), 92 deletions(-)

diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 29a2854..1e22105 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -4,12 +4,12 @@ use cursive::{
     view::{Nameable, Resizable, ViewWrapper},
     views::{
         Button, Dialog, DummyView, LinearLayout, NamedView, PaddedView, Panel, ScrollView,
-        SelectView, TextView,
+        SelectView, TextView, ViewRef,
     },
-    Cursive, View,
+    Cursive, Vec2, View,
 };
 
-use super::{DiskSizeEditView, FormView, IntegerEditView};
+use super::{DiskSizeEditView, FormView, IntegerEditView, TabbedView};
 use crate::options::FS_TYPES;
 use crate::InstallerState;
 
@@ -67,11 +67,15 @@ impl BootdiskOptionsView {
                 let runinfo = runinfo.clone();
                 let options = advanced_options.clone();
                 move |siv| {
-                    siv.add_layer(advanced_options_view(
-                        &runinfo,
-                        options.clone(),
-                        product_conf.clone(),
-                    ));
+                    let mut view =
+                        advanced_options_view(&runinfo, options.clone(), product_conf.clone());
+
+                    // Pre-compute the child's layout, since it might depend on the size. Without this,
+                    // the view will be empty until focused.
+                    // The screen size never changes in our case, so this is completely OK.
+                    view.layout(siv.screen_size());
+
+                    siv.add_layer(view);
                 }
             }));
 
@@ -193,6 +197,7 @@ impl AdvancedBootdiskOptionsView {
             .unwrap_or_else(|| runinfo.disks[0].clone());
 
         // Update the (inner) options view
+        let screen_size = siv.screen_size();
         siv.call_on_name("advanced-bootdisk-options-dialog", |view: &mut Dialog| {
             if let Some(AdvancedBootdiskOptionsView { view }) =
                 view.get_content_mut().downcast_mut()
@@ -203,7 +208,7 @@ impl AdvancedBootdiskOptionsView {
                         view.add_child(LvmBootdiskOptionsView::new_with_defaults(
                             &selected_lvm_disk,
                             &product_conf,
-                        ));
+                        ))
                     }
                     FsType::Zfs(_) => view.add_child(ZfsBootdiskOptionsView::new_with_defaults(
                         &runinfo,
@@ -213,6 +218,11 @@ impl AdvancedBootdiskOptionsView {
                         view.add_child(BtrfsBootdiskOptionsView::new_with_defaults(&runinfo))
                     }
                 }
+
+                // Pre-compute the child's layout, since it might depend on the size. Without this,
+                // the view will be empty until focused.
+                // The screen size never changes in our case, so this is completely OK.
+                view.layout(screen_size);
             }
         });
 
@@ -373,11 +383,98 @@ impl ViewWrapper for LvmBootdiskOptionsView {
 
 struct MultiDiskOptionsView<T> {
     view: LinearLayout,
+    layout_data: Option<(Vec<Disk>, Vec<usize>, T)>,
     phantom: PhantomData<T>,
 }
 
 impl<T: View> MultiDiskOptionsView<T> {
+    const DISK_FORM_VIEW_ID: &'static str = "multidisk-disk-form";
+
     fn new(avail_disks: &[Disk], selected_disks: &[usize], options_view: T) -> Self {
+        Self {
+            view: LinearLayout::vertical().child(DummyView).child(DummyView),
+            layout_data: Some((avail_disks.to_vec(), selected_disks.to_vec(), options_view)),
+            phantom: PhantomData,
+        }
+    }
+
+    fn top_panel(mut self, view: impl View) -> Self {
+        self.view.remove_child(0);
+        self.view.insert_child(0, Panel::new(view));
+        self
+    }
+
+    fn get_options_view(&mut self) -> Option<&T> {
+        let inner = self.view.get_child(1)?;
+
+        if let Some(view) = inner.downcast_ref::<LinearLayout>() {
+            view.get_child(2)?
+                .downcast_ref::<LinearLayout>()?
+                .get_child(2)?
+                .downcast_ref::<T>()
+        } else if let Some(view) = inner.downcast_ref::<TabbedView>() {
+            view.get(1)?
+                .downcast_ref::<PaddedView<T>>()
+                .map(|v| v.get_inner())
+        } else {
+            None
+        }
+    }
+
+    fn get_disk_form(&mut self) -> Option<ViewRef<FormView>> {
+        let inner = self.view.get_child_mut(1)?;
+
+        let view = if let Some(view) = inner.downcast_mut::<LinearLayout>() {
+            view.get_child_mut(0)?
+                .downcast_mut::<LinearLayout>()?
+                .get_child_mut(2)
+        } else if let Some(view) = inner.downcast_mut::<TabbedView>() {
+            view.get_mut(0)?
+                .downcast_mut::<PaddedView<LinearLayout>>()
+                .map(|v| v.get_inner_mut())?
+                .get_child_mut(0)
+        } else {
+            None
+        };
+
+        view?
+            .downcast_mut::<ScrollView<NamedView<FormView>>>()
+            .map(ScrollView::get_inner_mut)
+            .map(NamedView::get_mut)
+    }
+
+    /// This function returns a tuple of vectors. The first vector contains the currently selected
+    /// disks in order of their selection slot. Empty slots are filtered out. The second vector
+    /// contains indices of each slot's selection, which enables us to restore the selection even
+    /// for empty slots.
+    fn get_disks_and_selection(&mut self) -> Option<(Vec<Disk>, Vec<usize>)> {
+        let mut disks = vec![];
+        let mut selected_disks = Vec::new();
+        let disk_form = self.get_disk_form()?;
+
+        for i in 0..disk_form.len() {
+            let disk = disk_form.get_value::<SelectView<Option<Disk>>, _>(i)?;
+
+            // `None` means no disk was selected for this slot
+            if let Some(disk) = disk {
+                disks.push(disk);
+            }
+
+            selected_disks.push(
+                disk_form
+                    .get_child::<SelectView<Option<Disk>>>(i)?
+                    .selected_id()?,
+            );
+        }
+
+        Some((disks, selected_disks))
+    }
+
+    fn do_layout(&mut self, size: Vec2) {
+        let Some((avail_disks, selected_disks, options_view)) = self.layout_data.take() else {
+            panic!("cannot do layout without data!");
+        };
+
         let mut selectable_disks = avail_disks
             .iter()
             .map(|d| (d.to_string(), Some(d.clone())))
@@ -396,15 +493,14 @@ impl<T: View> MultiDiskOptionsView<T> {
             );
         }
 
-        let mut disk_select_view = LinearLayout::vertical()
-            .child(TextView::new("Disk setup").center())
-            .child(DummyView)
-            .child(ScrollView::new(disk_form.with_name("multidisk-disk-form")));
+        let mut disk_select_view = LinearLayout::vertical().child(ScrollView::new(
+            disk_form.with_name(Self::DISK_FORM_VIEW_ID),
+        ));
 
         if avail_disks.len() > 3 {
             let do_not_use_index = selectable_disks.len() - 1;
             let deselect_all_button = Button::new("Deselect all", move |siv| {
-                siv.call_on_name("multidisk-disk-form", |view: &mut FormView| {
+                siv.call_on_name(Self::DISK_FORM_VIEW_ID, |view: &mut FormView| {
                     view.call_on_childs(&|v: &mut SelectView<Option<Disk>>| {
                         // As there is no .on_select() callback defined on the
                         // SelectView's, the returned callback here can be safely
@@ -425,96 +521,52 @@ impl<T: View> MultiDiskOptionsView<T> {
             ));
         }
 
-        let options_view = LinearLayout::vertical()
-            .child(TextView::new("Advanced options").center())
-            .child(DummyView)
-            .child(options_view);
+        self.view.remove_child(1);
 
-        let view = LinearLayout::horizontal()
-            .child(disk_select_view)
-            .child(DummyView.fixed_width(3))
-            .child(options_view);
+        if size.x > 80 {
+            disk_select_view.insert_child(0, DummyView);
+            disk_select_view.insert_child(0, TextView::new("Disk setup").center());
 
-        Self {
-            view: LinearLayout::vertical().child(view),
-            phantom: PhantomData,
-        }
-    }
+            let view = LinearLayout::horizontal()
+                .child(disk_select_view)
+                .child(DummyView.fixed_width(3))
+                .child(
+                    LinearLayout::vertical()
+                        .child(TextView::new("Advanced options").center())
+                        .child(DummyView)
+                        .child(options_view),
+                );
 
-    fn top_panel(mut self, view: impl View) -> Self {
-        if self.has_top_panel() {
-            self.view.remove_child(0);
+            self.view.add_child(view);
+        } else {
+            let view = TabbedView::new()
+                .tab("Disk setup", PaddedView::lrtb(0, 0, 1, 0, disk_select_view))
+                .tab(
+                    "Advanced options",
+                    PaddedView::lrtb(0, 0, 1, 0, options_view),
+                );
+
+            self.view.add_child(view);
         }
-
-        self.view.insert_child(0, Panel::new(view));
-        self
     }
+}
 
-    ///
-    /// This function returns a tuple of vectors. The first vector contains the currently selected
-    /// disks in order of their selection slot. Empty slots are filtered out. The second vector
-    /// contains indices of each slot's selection, which enables us to restore the selection even
-    /// for empty slots.
-    ///
-    fn get_disks_and_selection(&mut self) -> Option<(Vec<Disk>, Vec<usize>)> {
-        let mut disks = vec![];
-        let view_top_index = usize::from(self.has_top_panel());
-
-        let disk_form = self
-            .view
-            .get_child_mut(view_top_index)?
-            .downcast_mut::<LinearLayout>()?
-            .get_child_mut(0)?
-            .downcast_mut::<LinearLayout>()?
-            .get_child_mut(2)?
-            .downcast_mut::<ScrollView<NamedView<FormView>>>()?
-            .get_inner_mut()
-            .get_mut();
-
-        let mut selected_disks = Vec::new();
-
-        for i in 0..disk_form.len() {
-            let disk = disk_form.get_value::<SelectView<Option<Disk>>, _>(i)?;
-
-            // `None` means no disk was selected for this slot
-            if let Some(disk) = disk {
-                disks.push(disk);
-            }
+impl<T: 'static + View> ViewWrapper for MultiDiskOptionsView<T> {
+    cursive::wrap_impl!(self.view: LinearLayout);
 
-            selected_disks.push(
-                disk_form
-                    .get_child::<SelectView<Option<Disk>>>(i)?
-                    .selected_id()?,
-            );
+    fn wrap_layout(&mut self, size: Vec2) {
+        if self.layout_data.is_some() {
+            self.do_layout(size);
         }
 
-        Some((disks, selected_disks))
-    }
-
-    fn inner_mut(&mut self) -> Option<&mut T> {
-        let view_top_index = usize::from(self.has_top_panel());
-
-        self.view
-            .get_child_mut(view_top_index)?
-            .downcast_mut::<LinearLayout>()?
-            .get_child_mut(2)?
-            .downcast_mut::<LinearLayout>()?
-            .get_child_mut(2)?
-            .downcast_mut::<T>()
+        self.view.layout(size)
     }
 
-    fn has_top_panel(&self) -> bool {
-        // The root view should only ever have one or two children
-        assert!([1, 2].contains(&self.view.len()));
-
-        self.view.len() == 2
+    fn wrap_needs_relayout(&self) -> bool {
+        self.layout_data.is_some() || self.view.needs_relayout()
     }
 }
 
-impl<T: 'static> ViewWrapper for MultiDiskOptionsView<T> {
-    cursive::wrap_impl!(self.view: LinearLayout);
-}
-
 struct BtrfsBootdiskOptionsView {
     view: MultiDiskOptionsView<FormView>,
 }
@@ -540,7 +592,10 @@ impl BtrfsBootdiskOptionsView {
 
     fn get_values(&mut self) -> Option<(Vec<Disk>, BtrfsBootdiskOptions)> {
         let (disks, selected_disks) = self.view.get_disks_and_selection()?;
-        let disk_size = self.view.inner_mut()?.get_value::<DiskSizeEditView, _>(0)?;
+        let disk_size = self
+            .view
+            .get_options_view()?
+            .get_value::<DiskSizeEditView, _>(0)?;
 
         Some((
             disks,
@@ -626,7 +681,7 @@ impl ZfsBootdiskOptionsView {
 
     fn get_values(&mut self) -> Option<(Vec<Disk>, ZfsBootdiskOptions)> {
         let (disks, selected_disks) = self.view.get_disks_and_selection()?;
-        let view = self.view.inner_mut()?;
+        let view = self.view.get_options_view()?;
         let has_arc_max = view.len() >= 6;
         let disk_size_index = if has_arc_max { 5 } else { 4 };
 
-- 
2.44.1



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Christoph Heiss]

On Tue, Jul 02, 2024 at 06:49:12PM GMT, Max Carrara wrote:
> On Thu Jun 13, 2024 at 1:53 PM CEST, Christoph Heiss wrote:
> > [..]
>
> 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@proxmox.com>
> Tested-by: Max Carrara <m.carrara@proxmox.com>
>

Thanks for the review!

[..]
> 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

Well, 80x24 is basically the minimum size I'd reasonable support. It's
the resolution specified by the VT10x, so I would say it's safe to
assume that every video output supports at _least_ that. It's also
what's used for serial output. See also e.g. commit 3facbe51c [0] or the
VT100 technical manual [1] some more in-depth information.

And since the target is pretty defined for this (always fullscreen on
some video output, not some resizable graphical terminal window or
such), so I think supporting anything less would be a lot of work for
pretty much no return.

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

The disk view is scrollable and should scroll as needed. Tested this
with ~12 disks.

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

Yeah, that's what I thought, but I wasn't sure. It would also match the
GTK installer in that regard. The side-by-side is quite nice on wide
screens though, but no hard feelings :^)

[0] https://git.proxmox.com/?p=pve-installer.git;a=commitdiff;h=3facbe51c
[1] https://vt100.net/dec/ek-vt100-tm-002.pdf (page 21, "Format")



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Thomas Lamprecht]

Am 13/06/2024 um 13:53 schrieb Christoph Heiss:
> 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.
> 
> 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
> 


applied series with Max's review trailers, thanks to both of you!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel