Re: [pve-devel] [PATCH installer] tui: persist disk selection for zfs and btrfs

[Maximiliano Sandoval <m.sandoval@proxmox.com>]

> On 27.06.2023 14:54 CEST Stefan Sterz <s.sterz@proxmox.com> wrote:
> 
>  
> previously the disk selection was reset if the advanced options
> dialogue was re-opened. this commit adapts the behavior to restore
> the previous selection.
> 
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
>  proxmox-tui-installer/src/options.rs        | 11 +++++--
>  proxmox-tui-installer/src/views/bootdisk.rs | 36 +++++++++++++++------
>  2 files changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/proxmox-tui-installer/src/options.rs b/proxmox-tui-installer/src/options.rs
> index 5f0612e..56150c4 100644
> --- a/proxmox-tui-installer/src/options.rs
> +++ b/proxmox-tui-installer/src/options.rs
> @@ -118,12 +118,15 @@ impl LvmBootdiskOptions {
>  #[derive(Clone, Debug)]
>  pub struct BtrfsBootdiskOptions {
>      pub disk_size: f64,
> +    pub selected_disks: Vec<usize>,
>  }
>  
>  impl BtrfsBootdiskOptions {
> -    pub fn defaults_from(disk: &Disk) -> Self {
> +    pub fn defaults_from(disks: &[Disk]) -> Self {
> +        let disk = &disks[0];
>          Self {
>              disk_size: disk.size,
> +            selected_disks: (0..disks.len()).collect(),

Any reason not to use Vec::with_capacity(disks.len()) here?

>          }
>      }
>  }
> @@ -191,16 +194,20 @@ pub struct ZfsBootdiskOptions {
>      pub checksum: ZfsChecksumOption,
>      pub copies: usize,
>      pub disk_size: f64,
> +    pub selected_disks: Vec<usize>,
>  }
>  
>  impl ZfsBootdiskOptions {
> -    pub fn defaults_from(disk: &Disk) -> Self {
> +    pub fn defaults_from(disks: &[Disk]) -> Self {
> +        let disk = &disks[0];

I know this is an existing behavior, but would it make sense to handle the case when the slice is empty? Otherwise it could be documented that this crashes with empty slices.

> +
>          Self {
>              ashift: 12,
>              compress: ZfsCompressOption::default(),
>              checksum: ZfsChecksumOption::default(),
>              copies: 1,
>              disk_size: disk.size,
> +            selected_disks: (0..disks.len()).collect(),
>          }
>      }
>  }
> diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
> index 75f70a1..c3cf4b6 100644
> --- a/proxmox-tui-installer/src/views/bootdisk.rs
> +++ b/proxmox-tui-installer/src/views/bootdisk.rs
> @@ -137,11 +137,11 @@ impl AdvancedBootdiskOptionsView {
>                      )),
>                      FsType::Zfs(_) => view.add_child(ZfsBootdiskOptionsView::new(
>                          disks,
> -                        &ZfsBootdiskOptions::defaults_from(&disks[0]),
> +                        &ZfsBootdiskOptions::defaults_from(disks),
>                      )),
>                      FsType::Btrfs(_) => view.add_child(BtrfsBootdiskOptionsView::new(
>                          disks,
> -                        &BtrfsBootdiskOptions::defaults_from(&disks[0]),
> +                        &BtrfsBootdiskOptions::defaults_from(disks),
>                      )),
>                  }
>              }
> @@ -274,7 +274,7 @@ struct MultiDiskOptionsView<T> {
>  }
>  
>  impl<T: View> MultiDiskOptionsView<T> {
> -    fn new(avail_disks: &[Disk], options_view: T) -> Self {
> +    fn new(avail_disks: &[Disk], selected_disks: &Vec<usize>, options_view: T) -> Self {
>          let mut selectable_disks = avail_disks
>              .iter()
>              .map(|d| (d.to_string(), Some(d.clone())))
> @@ -289,7 +289,7 @@ impl<T: View> MultiDiskOptionsView<T> {
>                  SelectView::new()
>                      .popup()
>                      .with_all(selectable_disks.clone())
> -                    .selected(i),
> +                    .selected(selected_disks[i]),
>              );
>          }
>  
> @@ -323,7 +323,7 @@ impl<T: View> MultiDiskOptionsView<T> {
>          self
>      }
>  
> -    fn get_disks(&mut self) -> Option<Vec<Disk>> {
> +    fn get_disks(&mut self) -> Option<(Vec<Disk>, Vec<usize>)> {

Its not clear what the function does from the signature alone, imho either another type is returned or the function is documented.

>          let mut disks = vec![];
>          let view_top_index = usize::from(self.has_top_panel());
>  
> @@ -337,6 +337,8 @@ impl<T: View> MultiDiskOptionsView<T> {
>              .downcast_ref::<ScrollView<FormView>>()?
>              .get_inner();
>  
> +        let mut selected_disks = Vec::new();
> +
>          for i in 0..disk_form.len() {
>              let disk = disk_form.get_value::<SelectView<Option<Disk>>, _>(i)?;
>  
> @@ -344,9 +346,15 @@ impl<T: View> MultiDiskOptionsView<T> {
>              if let Some(disk) = disk {
>                  disks.push(disk);
>              }
> +
> +            selected_disks.push(
> +                disk_form
> +                    .get_child::<SelectView<Option<Disk>>>(i)?
> +                    .selected_id()?,
> +            );
>          }
>  
> -        Some(disks)
> +        Some((disks, selected_disks))
>      }
>  
>      fn inner_mut(&mut self) -> Option<&mut T> {
> @@ -382,6 +390,7 @@ impl BtrfsBootdiskOptionsView {
>      fn new(disks: &[Disk], options: &BtrfsBootdiskOptions) -> Self {
>          let view = MultiDiskOptionsView::new(
>              disks,
> +            &options.selected_disks,
>              FormView::new().child("hdsize", DiskSizeEditView::new().content(options.disk_size)),
>          )
>          .top_panel(TextView::new("Btrfs integration is a technology preview!").center());
> @@ -390,10 +399,16 @@ impl BtrfsBootdiskOptionsView {
>      }
>  
>      fn get_values(&mut self) -> Option<(Vec<Disk>, BtrfsBootdiskOptions)> {
> -        let disks = self.view.get_disks()?;
> +        let (disks, selected_disks) = self.view.get_disks()?;
>          let disk_size = self.view.inner_mut()?.get_value::<DiskSizeEditView, _>(0)?;
>  
> -        Some((disks, BtrfsBootdiskOptions { disk_size }))
> +        Some((
> +            disks,
> +            BtrfsBootdiskOptions {
> +                disk_size,
> +                selected_disks,
> +            },
> +        ))
>      }
>  }
>  
> @@ -437,7 +452,7 @@ impl ZfsBootdiskOptionsView {
>              .child("copies", IntegerEditView::new().content(options.copies))
>              .child("hdsize", DiskSizeEditView::new().content(options.disk_size));
>  
> -        let view = MultiDiskOptionsView::new(disks, inner)
> +        let view = MultiDiskOptionsView::new(disks, &options.selected_disks, inner)
>              .top_panel(TextView::new(
>                  "ZFS is not compatible with hardware RAID controllers, for details see the documentation."
>              ).center());
> @@ -446,7 +461,7 @@ impl ZfsBootdiskOptionsView {
>      }
>  
>      fn get_values(&mut self) -> Option<(Vec<Disk>, ZfsBootdiskOptions)> {
> -        let disks = self.view.get_disks()?;
> +        let (disks, selected_disks) = self.view.get_disks()?;
>          let view = self.view.inner_mut()?;
>  
>          let ashift = view.get_value::<IntegerEditView, _>(0)?;
> @@ -463,6 +478,7 @@ impl ZfsBootdiskOptionsView {
>                  checksum,
>                  copies,
>                  disk_size,
> +                selected_disks,
>              },
>          ))
>      }
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel