* [pve-devel] [PATCH installer] tui: persist disk selection for zfs and btrfs
@ 2023-06-27 12:54 Stefan Sterz
2023-06-27 13:34 ` Maximiliano Sandoval
0 siblings, 1 reply; 4+ messages in thread
From: Stefan Sterz @ 2023-06-27 12:54 UTC (permalink / raw)
To: pve-devel
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(),
}
}
}
@@ -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];
+
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>)> {
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH installer] tui: persist disk selection for zfs and btrfs
2023-06-27 12:54 [pve-devel] [PATCH installer] tui: persist disk selection for zfs and btrfs Stefan Sterz
@ 2023-06-27 13:34 ` Maximiliano Sandoval
2023-06-27 13:57 ` Lukas Wagner
0 siblings, 1 reply; 4+ messages in thread
From: Maximiliano Sandoval @ 2023-06-27 13:34 UTC (permalink / raw)
To: Proxmox VE development discussion, Stefan Sterz
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH installer] tui: persist disk selection for zfs and btrfs
2023-06-27 13:34 ` Maximiliano Sandoval
@ 2023-06-27 13:57 ` Lukas Wagner
2023-06-27 13:59 ` Stefan Sterz
0 siblings, 1 reply; 4+ messages in thread
From: Lukas Wagner @ 2023-06-27 13:57 UTC (permalink / raw)
To: Proxmox VE development discussion, Maximiliano Sandoval, Stefan Sterz
On 6/27/23 15:34, Maximiliano Sandoval wrote:
>>
>> 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?
>
I haven't really examined the rest of the code, but wouldn't that change the behavior
completely? E.g., if `disk.len()` is 3, then
`(0..disks.len()).collect()` will give you a Vec [0, 1, 2], while
`Vec::with_capacity(disks.len())` would give you an empty Vec with an initial capacity
of at least 3.
--
- Lukas
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pve-devel] [PATCH installer] tui: persist disk selection for zfs and btrfs
2023-06-27 13:57 ` Lukas Wagner
@ 2023-06-27 13:59 ` Stefan Sterz
0 siblings, 0 replies; 4+ messages in thread
From: Stefan Sterz @ 2023-06-27 13:59 UTC (permalink / raw)
To: Lukas Wagner, Proxmox VE development discussion, Maximiliano Sandoval
On 27.06.23 15:57, Lukas Wagner wrote:
>
>
> On 6/27/23 15:34, Maximiliano Sandoval wrote:
>>> 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?
>>
>
> I haven't really examined the rest of the code, but wouldn't that change
> the behavior
> completely? E.g., if `disk.len()` is 3, then
> `(0..disks.len()).collect()` will give you a Vec [0, 1, 2], while
> `Vec::with_capacity(disks.len())` would give you an empty Vec with an
> initial capacity
> of at least 3.
>
>
yes. we've already discussed this off list. this is needed here because
otherwise you panic out in `MultiDiskOptionsView::new()` because
`selected_disk` would have a length of zero. the ascending numbers are
needed to have the same initial selection as we currently do.
i'll send a patch with the other nits resolved in a minute.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-06-27 13:59 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-27 12:54 [pve-devel] [PATCH installer] tui: persist disk selection for zfs and btrfs Stefan Sterz
2023-06-27 13:34 ` Maximiliano Sandoval
2023-06-27 13:57 ` Lukas Wagner
2023-06-27 13:59 ` Stefan Sterz
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox