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 08BC7A922 for ; Tue, 27 Jun 2023 15:34:42 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DD88236F5D for ; Tue, 27 Jun 2023 15:34:11 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 27 Jun 2023 15:34:11 +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 D709844F93 for ; Tue, 27 Jun 2023 15:34:10 +0200 (CEST) Date: Tue, 27 Jun 2023 15:34:09 +0200 (CEST) From: Maximiliano Sandoval To: Proxmox VE development discussion , Stefan Sterz Message-ID: <948378528.2288.1687872849696@webmail.proxmox.com> In-Reply-To: <20230627125439.533438-1-s.sterz@proxmox.com> References: <20230627125439.533438-1-s.sterz@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Priority: 3 Importance: Normal X-Mailer: Open-Xchange Mailer v7.10.6-Rev47 X-Originating-Client: open-xchange-appsuite X-SPAM-LEVEL: Spam detection results: 0 AWL 0.001 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 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. [proxmox.com, bootdisk.rs, options.rs] Subject: Re: [pve-devel] [PATCH installer] tui: persist disk selection for zfs and btrfs 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: Tue, 27 Jun 2023 13:34:42 -0000 > On 27.06.2023 14:54 CEST Stefan Sterz 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 > --- > 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, > } > > 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, > } > > 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 { > } > > impl MultiDiskOptionsView { > - fn new(avail_disks: &[Disk], options_view: T) -> Self { > + fn new(avail_disks: &[Disk], selected_disks: &Vec, options_view: T) -> Self { > let mut selectable_disks = avail_disks > .iter() > .map(|d| (d.to_string(), Some(d.clone()))) > @@ -289,7 +289,7 @@ impl MultiDiskOptionsView { > SelectView::new() > .popup() > .with_all(selectable_disks.clone()) > - .selected(i), > + .selected(selected_disks[i]), > ); > } > > @@ -323,7 +323,7 @@ impl MultiDiskOptionsView { > self > } > > - fn get_disks(&mut self) -> Option> { > + fn get_disks(&mut self) -> Option<(Vec, Vec)> { 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 MultiDiskOptionsView { > .downcast_ref::>()? > .get_inner(); > > + let mut selected_disks = Vec::new(); > + > for i in 0..disk_form.len() { > let disk = disk_form.get_value::>, _>(i)?; > > @@ -344,9 +346,15 @@ impl MultiDiskOptionsView { > if let Some(disk) = disk { > disks.push(disk); > } > + > + selected_disks.push( > + disk_form > + .get_child::>>(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, BtrfsBootdiskOptions)> { > - let disks = self.view.get_disks()?; > + let (disks, selected_disks) = self.view.get_disks()?; > let disk_size = self.view.inner_mut()?.get_value::(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, 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::(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