* [pve-devel] [PATCH installer v3 1/3] tui: NumericEditView: add optional placeholder value
2024-04-15 13:20 [pve-devel] [PATCH installer v3 0/3] expose zfs arc size setting for all products Christoph Heiss
@ 2024-04-15 13:20 ` Christoph Heiss
2024-04-15 13:20 ` [pve-devel] [PATCH installer v3 2/3] tui: expose arc size setting for zfs bootdisks for all products Christoph Heiss
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Christoph Heiss @ 2024-04-15 13:20 UTC (permalink / raw)
To: pve-devel
Enables to add an optional placeholder value to `NumericEditView`, which
will be displayed in a different (darker) color and not returned by
`.get_content*()`.
Can be used for having default values in the TUI, but with different
handling in the back.
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v2 -> v3:
* when empty & focused, do not show the placeholder value at all
Changes v1 -> v2:
* new patch
proxmox-tui-installer/src/views/mod.rs | 66 ++++++++++++++++++++++++--
1 file changed, 62 insertions(+), 4 deletions(-)
diff --git a/proxmox-tui-installer/src/views/mod.rs b/proxmox-tui-installer/src/views/mod.rs
index 3244e76..5e5f4fb 100644
--- a/proxmox-tui-installer/src/views/mod.rs
+++ b/proxmox-tui-installer/src/views/mod.rs
@@ -2,9 +2,10 @@ use std::{net::IpAddr, rc::Rc, str::FromStr};
use cursive::{
event::{Event, EventResult},
+ theme::BaseColor,
view::{Resizable, ViewWrapper},
views::{EditView, LinearLayout, NamedView, ResizedView, SelectView, TextView},
- Rect, Vec2, View,
+ Printer, Rect, Vec2, View,
};
use proxmox_installer_common::utils::CidrAddress;
@@ -24,6 +25,7 @@ pub use timezone::*;
pub struct NumericEditView<T> {
view: LinearLayout,
max_value: Option<T>,
+ placeholder: Option<T>,
max_content_width: Option<usize>,
allow_empty: bool,
}
@@ -36,6 +38,7 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
Self {
view,
max_value: None,
+ placeholder: None,
max_content_width: None,
allow_empty: false,
}
@@ -54,6 +57,7 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
Self {
view,
max_value: None,
+ placeholder: None,
max_content_width: None,
allow_empty: false,
}
@@ -84,15 +88,42 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
self
}
+ /// Sets a placeholder value for this view. Implies `allow_empty(true)`.
+ /// Implies `allow_empty(true)`.
+ ///
+ /// # Arguments
+ /// `placeholder` - The placeholder value to set for this view.
+ #[allow(unused)]
+ pub fn placeholder(mut self, placeholder: T) -> Self {
+ self.placeholder = Some(placeholder);
+ self.allow_empty(true)
+ }
+
+ /// Returns the current value of the view. If a placeholder is defined and
+ /// no value is currently set, the placeholder value is returned.
+ ///
+ /// **This should only be called when `allow_empty = false` or a placeholder
+ /// is set.**
pub fn get_content(&self) -> Result<T, <T as FromStr>::Err> {
- assert!(!self.allow_empty);
- self.inner().get_content().parse()
+ let content = self.inner().get_content();
+
+ if content.is_empty() {
+ if let Some(placeholder) = self.placeholder {
+ return Ok(placeholder);
+ }
+ }
+
+ assert!(!(self.allow_empty && self.placeholder.is_none()));
+ content.parse()
}
+ /// Returns the current value of the view, or [`None`] if the view is
+ /// currently empty.
pub fn get_content_maybe(&self) -> Option<Result<T, <T as FromStr>::Err>> {
let content = self.inner().get_content();
+
if !content.is_empty() {
- Some(self.inner().get_content().parse())
+ Some(content.parse())
} else {
None
}
@@ -157,6 +188,25 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
std::mem::swap(self.inner_mut(), &mut inner);
self
}
+
+ /// Generic `wrap_draw()` implementation for [`ViewWrapper`].
+ ///
+ /// # Arguments
+ /// * `printer` - The [`Printer`] to draw to the base view.
+ fn wrap_draw_inner(&self, printer: &Printer) {
+ self.view.draw(printer);
+
+ if self.inner().get_content().is_empty() && !printer.focused {
+ if let Some(placeholder) = self.placeholder {
+ let placeholder = placeholder.to_string();
+
+ printer.with_color(
+ (BaseColor::Blue.light(), BaseColor::Blue.dark()).into(),
+ |printer| printer.print((0, 0), &placeholder),
+ );
+ }
+ }
+ }
}
pub type FloatEditView = NumericEditView<f64>;
@@ -165,6 +215,10 @@ pub type IntegerEditView = NumericEditView<usize>;
impl ViewWrapper for FloatEditView {
cursive::wrap_impl!(self.view: LinearLayout);
+ fn wrap_draw(&self, printer: &Printer) {
+ self.wrap_draw_inner(printer);
+ }
+
fn wrap_on_event(&mut self, event: Event) -> EventResult {
let original = self.inner_mut().get_content();
@@ -204,6 +258,10 @@ impl FloatEditView {
impl ViewWrapper for IntegerEditView {
cursive::wrap_impl!(self.view: LinearLayout);
+ fn wrap_draw(&self, printer: &Printer) {
+ self.wrap_draw_inner(printer);
+ }
+
fn wrap_on_event(&mut self, event: Event) -> EventResult {
let original = self.inner_mut().get_content();
--
2.44.0
^ permalink raw reply [flat|nested] 5+ messages in thread* [pve-devel] [PATCH installer v3 2/3] tui: expose arc size setting for zfs bootdisks for all products
2024-04-15 13:20 [pve-devel] [PATCH installer v3 0/3] expose zfs arc size setting for all products Christoph Heiss
2024-04-15 13:20 ` [pve-devel] [PATCH installer v3 1/3] tui: NumericEditView: add optional placeholder value Christoph Heiss
@ 2024-04-15 13:20 ` Christoph Heiss
2024-04-15 13:20 ` [pve-devel] [PATCH installer v3 3/3] proxinstall: " Christoph Heiss
2024-05-24 10:30 ` [pve-devel] [PATCH installer v3 0/3] expose zfs arc size setting " Christoph Heiss
3 siblings, 0 replies; 5+ messages in thread
From: Christoph Heiss @ 2024-04-15 13:20 UTC (permalink / raw)
To: pve-devel
For non-PVE products, simply use the ZFS defaults (aka. 50%) and leave
unset, if the user never touches that setting.
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v2 -> v3:
* no changes
Changes v1 -> v2:
* use new placeholder functionality for non-PVE products
proxmox-installer-common/src/options.rs | 3 +-
proxmox-tui-installer/src/views/bootdisk.rs | 45 ++++++++++++---------
proxmox-tui-installer/src/views/mod.rs | 1 -
3 files changed, 29 insertions(+), 20 deletions(-)
diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 1aa8f65..a210142 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -205,7 +205,8 @@ impl ZfsBootdiskOptions {
/// The default ZFS maximum ARC size in MiB for this system.
fn default_zfs_arc_max(product: ProxmoxProduct, total_memory: usize) -> usize {
if product != ProxmoxProduct::PVE {
- // Use ZFS default for non-PVE
+ // For products other the PVE, just let ZFS decide on its own. Setting `0`
+ // causes the installer to skip writing the `zfs_arc_max` module parameter.
0
} else {
((total_memory as f64) / 10.)
diff --git a/proxmox-tui-installer/src/views/bootdisk.rs b/proxmox-tui-installer/src/views/bootdisk.rs
index 7e13e91..3d9be50 100644
--- a/proxmox-tui-installer/src/views/bootdisk.rs
+++ b/proxmox-tui-installer/src/views/bootdisk.rs
@@ -564,7 +564,18 @@ impl ZfsBootdiskOptionsView {
options: &ZfsBootdiskOptions,
product_conf: &ProductConfig,
) -> Self {
- let is_pve = product_conf.product == ProxmoxProduct::PVE;
+ let arc_max_view = {
+ let view = IntegerEditView::new_with_suffix("MiB").max_value(runinfo.total_memory);
+
+ // For PVE "force" the default value, for other products place the recommended value
+ // only in the placeholder. This causes for the latter to not write the module option
+ // if the value is never modified by the user.
+ if product_conf.product == ProxmoxProduct::PVE {
+ view.content(options.arc_max)
+ } else {
+ view.placeholder(runinfo.total_memory / 2)
+ }
+ };
let inner = FormView::new()
.child("ashift", IntegerEditView::new().content(options.ashift))
@@ -592,14 +603,11 @@ impl ZfsBootdiskOptionsView {
.unwrap_or_default(),
),
)
- .child("copies", IntegerEditView::new().content(options.copies).max_value(3))
- .child_conditional(
- is_pve,
- "ARC max size",
- IntegerEditView::new_with_suffix("MiB")
- .max_value(runinfo.total_memory)
- .content(options.arc_max),
+ .child(
+ "copies",
+ IntegerEditView::new().content(options.copies).max_value(3),
)
+ .child("ARC max size", arc_max_view)
.child("hdsize", DiskSizeEditView::new().content(options.disk_size));
let view = MultiDiskOptionsView::new(&runinfo.disks, &options.selected_disks, inner)
@@ -621,21 +629,22 @@ 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 has_arc_max = view.len() >= 6;
- let disk_size_index = if has_arc_max { 5 } else { 4 };
let ashift = view.get_value::<IntegerEditView, _>(0)?;
let compress = view.get_value::<SelectView<_>, _>(1)?;
let checksum = view.get_value::<SelectView<_>, _>(2)?;
let copies = view.get_value::<IntegerEditView, _>(3)?;
- let disk_size = view.get_value::<DiskSizeEditView, _>(disk_size_index)?;
-
- let arc_max = if has_arc_max {
- view.get_value::<IntegerEditView, _>(4)?
- .max(ZFS_ARC_MIN_SIZE_MIB)
- } else {
- 0 // use built-in ZFS default value
- };
+ let disk_size = view.get_value::<DiskSizeEditView, _>(5)?;
+
+ // If a value is set, return that and clamp it to at least [`ZFS_ARC_MIN_SIZE_MIB`].
+ //
+ // Otherwise, if no value was set or an error occured return `0`. The former simply means
+ // that the placeholder value is still there.
+ let arc_max = view
+ .get_child::<IntegerEditView>(4)?
+ .get_content_maybe()
+ .map_or(Ok(0), |v| v.map(|v| v.max(ZFS_ARC_MIN_SIZE_MIB)))
+ .unwrap_or(0);
Some((
disks,
diff --git a/proxmox-tui-installer/src/views/mod.rs b/proxmox-tui-installer/src/views/mod.rs
index 5e5f4fb..eab258c 100644
--- a/proxmox-tui-installer/src/views/mod.rs
+++ b/proxmox-tui-installer/src/views/mod.rs
@@ -93,7 +93,6 @@ impl<T: Copy + ToString + FromStr + PartialOrd> NumericEditView<T> {
///
/// # Arguments
/// `placeholder` - The placeholder value to set for this view.
- #[allow(unused)]
pub fn placeholder(mut self, placeholder: T) -> Self {
self.placeholder = Some(placeholder);
self.allow_empty(true)
--
2.44.0
^ permalink raw reply [flat|nested] 5+ messages in thread* [pve-devel] [PATCH installer v3 3/3] proxinstall: expose arc size setting for zfs bootdisks for all products
2024-04-15 13:20 [pve-devel] [PATCH installer v3 0/3] expose zfs arc size setting for all products Christoph Heiss
2024-04-15 13:20 ` [pve-devel] [PATCH installer v3 1/3] tui: NumericEditView: add optional placeholder value Christoph Heiss
2024-04-15 13:20 ` [pve-devel] [PATCH installer v3 2/3] tui: expose arc size setting for zfs bootdisks for all products Christoph Heiss
@ 2024-04-15 13:20 ` Christoph Heiss
2024-05-24 10:30 ` [pve-devel] [PATCH installer v3 0/3] expose zfs arc size setting " Christoph Heiss
3 siblings, 0 replies; 5+ messages in thread
From: Christoph Heiss @ 2024-04-15 13:20 UTC (permalink / raw)
To: pve-devel
For non-PVE products, simply use the ZFS defaults (aka. 50%) and leave
unset, if the user never touches that setting.
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v2 -> v3:
* rework based on Maximilano's suggestion using Gtk3::Adjustment
Changes v1 -> v2:
* add some more explanatory comments
Proxmox/Install/RunEnv.pm | 3 ++-
proxinstall | 26 +++++++++++++-------------
2 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/Proxmox/Install/RunEnv.pm b/Proxmox/Install/RunEnv.pm
index 25b6bb3..4e24da6 100644
--- a/Proxmox/Install/RunEnv.pm
+++ b/Proxmox/Install/RunEnv.pm
@@ -319,7 +319,8 @@ our $ZFS_ARC_SYSMEM_PERCENTAGE = 0.1; # use 10% of available system memory by de
# Calculates the default upper limit for the ZFS ARC size.
# Returns the default ZFS maximum ARC size in MiB.
sub default_zfs_arc_max {
- # Use ZFS default on non-PVE
+ # For products other the PVE, just let ZFS decide on its own. Setting `0`
+ # causes the installer to skip writing the `zfs_arc_max` module parameter.
return 0 if Proxmox::Install::ISOEnv::get('product') ne 'pve';
my $default_mib = get('total_memory') * $ZFS_ARC_SYSMEM_PERCENTAGE;
diff --git a/proxinstall b/proxinstall
index a6a4cfb..bc0e1e4 100755
--- a/proxinstall
+++ b/proxinstall
@@ -1138,20 +1138,20 @@ my $create_raid_advanced_grid = sub {
$spinbutton_copies->set_value($copies);
push @$labeled_widgets, ['copies', $spinbutton_copies];
- if ($iso_env->{product} eq 'pve') {
- my $total_memory = Proxmox::Install::RunEnv::get('total_memory');
+ my $total_memory = Proxmox::Install::RunEnv::get('total_memory');
+ my $arc_max = Proxmox::Install::Config::get_zfs_opt('arc_max') || ($total_memory * 0.5);
+
+ my $arc_max_adjustment = Gtk3::Adjustment->new(
+ $arc_max, $Proxmox::Install::RunEnv::ZFS_ARC_MIN_SIZE_MIB,
+ $total_memory, 1, 10, 0);
+ my $spinbutton_arc_max = Gtk3::SpinButton->new($arc_max_adjustment, 1, 0);
+ $spinbutton_arc_max->set_tooltip_text('Maximum ARC size in megabytes');
+ $spinbutton_arc_max->signal_connect('value-changed' => sub {
+ my $w = shift;
+ Proxmox::Install::Config::set_zfs_opt('arc_max', $w->get_value_as_int());
+ });
- my $spinbutton_arc_max = Gtk3::SpinButton->new_with_range(
- $Proxmox::Install::RunEnv::ZFS_ARC_MIN_SIZE_MIB, $total_memory, 1);
- $spinbutton_arc_max->set_tooltip_text('Maximum ARC size in megabytes');
- $spinbutton_arc_max->signal_connect('value-changed' => sub {
- my $w = shift;
- Proxmox::Install::Config::set_zfs_opt('arc_max', $w->get_value_as_int());
- });
- my $arc_max = Proxmox::Install::Config::get_zfs_opt('arc_max');
- $spinbutton_arc_max->set_value($arc_max);
- push @$labeled_widgets, ['ARC max size', $spinbutton_arc_max, 'MiB'];
- }
+ push @$labeled_widgets, ['ARC max size', $spinbutton_arc_max, 'MiB'];
push @$labeled_widgets, ['hdsize', $hdsize_btn, 'GB'];
return $create_label_widget_grid->($labeled_widgets);;
--
2.44.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [pve-devel] [PATCH installer v3 0/3] expose zfs arc size setting for all products
2024-04-15 13:20 [pve-devel] [PATCH installer v3 0/3] expose zfs arc size setting for all products Christoph Heiss
` (2 preceding siblings ...)
2024-04-15 13:20 ` [pve-devel] [PATCH installer v3 3/3] proxinstall: " Christoph Heiss
@ 2024-05-24 10:30 ` Christoph Heiss
3 siblings, 0 replies; 5+ messages in thread
From: Christoph Heiss @ 2024-05-24 10:30 UTC (permalink / raw)
Cc: Proxmox VE development discussion
v4: https://lists.proxmox.com/pipermail/pve-devel/2024-May/063957.html
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 5+ messages in thread