From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id EA0901FF141 for ; Tue, 05 May 2026 13:11:33 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2060824031; Tue, 5 May 2026 13:11:33 +0200 (CEST) Message-ID: Date: Tue, 5 May 2026 13:11:24 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH yew-comp v4 13/40] widget: kvlist: add widget for user-modifiable data tables To: Christoph Heiss , pdm-devel@lists.proxmox.com References: <20260430124712.1614305-1-c.heiss@proxmox.com> <20260430124712.1614305-14-c.heiss@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20260430124712.1614305-14-c.heiss@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1777979378894 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.050 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [lib.rs] Message-ID-Hash: HWTR363CWTOC4IXOPXJ2TVL2N4ZOUSRY X-Message-ID-Hash: HWTR363CWTOC4IXOPXJ2TVL2N4ZOUSRY X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Datacenter Manager development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: looks mostly ok, but there is at least one graver issue (could be fixed as a follow up though) the overall structure and interface looks fine to me though. If the one grave issue is fixed (either as follow up or in a next version) consider this Reviewed-by: Dominik Csapak comments inline On 4/30/26 2:47 PM, Christoph Heiss wrote: > Signed-off-by: Christoph Heiss > --- > Changes v3 -> v4: > * moved to proxmox-yew-comp > * use serde_json::Value instead of generic type > > Changes v2 -> v3: > * new patch > > src/key_value_list.rs | 348 ++++++++++++++++++++++++++++++++++++++++++ > src/lib.rs | 3 + > 2 files changed, 351 insertions(+) > create mode 100644 src/key_value_list.rs > > diff --git a/src/key_value_list.rs b/src/key_value_list.rs > new file mode 100644 > index 0000000..e6062b1 > --- /dev/null > +++ b/src/key_value_list.rs > @@ -0,0 +1,348 @@ > +use anyhow::{bail, Error}; > +use serde_json::Value; > +use std::{ > + fmt::Debug, > + rc::Rc, > + sync::atomic::{AtomicU32, Ordering}, > +}; > +use yew::{html::IntoPropValue, virtual_dom::Key}; > + > +use pwt::{ > + css::{AlignItems, ColorScheme, FlexFit, FontColor}, > + prelude::*, > + props::FieldStdProps, > + props::RenderFn, > + state::Store, > + widget::{ > + data_table::{DataTable, DataTableColumn, DataTableHeader}, > + form::{ > + Field, IntoSubmitValidateFn, ManagedField, ManagedFieldContext, ManagedFieldMaster, > + ManagedFieldScopeExt, ManagedFieldState, SubmitValidateFn, > + }, > + ActionIcon, Button, Column, Container, Fa, Row, > + }, > +}; > +use pwt_macros::{builder, widget}; nit, we usually do one module per line such as: use pwt::css::{..}; use pwt::preluse::*; use pwt::props::{...}; and so on can be fixed up as a follow up though > + > +#[widget(comp = ManagedFieldMaster, @input)] > +#[derive(Clone, PartialEq, Properties)] > +#[builder] > +/// A [`DataTable`]-based grid to hold a list of user-enterable key-value pairs. > +/// > +/// Displays a [`DataTable`] with three columns; key, value and a delete button, with an add button > +/// below to create new rows. > +/// Both key and value are modifiable by the user. > +pub struct KeyValueList { > + #[builder] > + #[prop_or_default] > + /// Initial value pairs to display. > + pub value: Vec<(String, Value)>, > + > + #[builder(IntoPropValue, into_prop_value)] > + #[prop_or(tr!("Name").into())] > + /// Label for the key column, defaults to "Name". > + pub key_label: AttrValue, > + > + #[builder(IntoPropValue, into_prop_value)] > + #[prop_or_default] > + /// Placeholder to display in the key columns fields, default is no placeholder. > + pub key_placeholder: AttrValue, nit: i'd use a Option here, otherwise we always pass an empty string around, and the placeholder already accepts Option<> anyway. > + > + #[builder(IntoPropValue, into_prop_value)] > + #[prop_or(tr!("Value").into())] > + /// Label for the value column. > + pub value_label: AttrValue, > + > + #[builder] > + #[prop_or(default_value_renderer.into())] > + pub value_renderer: RenderFn<(String, Value, FieldStdProps, Callback), Html>, this parameter looks a bit scary a better way to do this is to introduce a 'ValueRenderArgs' struct which has all the required (named!) fields and use that as input > + > + #[builder_cb(IntoSubmitValidateFn, into_submit_validate_fn, Vec<(String, Value)>)] > + #[prop_or_default] > + /// Callback to run on submit on the data in the table. > + pub submit_validate: Option>>, > +} > + > +impl KeyValueList { > + pub fn new() -> Self { > + yew::props!(Self {}) > + } > +} > + > +#[derive(Clone, Debug, PartialEq)] > +struct Entry { > + /// Only used as key for the store, since that needs a stable value > + index: u32, > + key: String, > + value: Value, > +} > + > +pub struct KeyValueListField { > + state: ManagedFieldState, > + store: Store, > + index_counter: AtomicU32, > +} > + > +pwt::impl_deref_mut_property!(KeyValueListField, state, ManagedFieldState); > + > +pub enum Message { > + DataChange, > + UpdateKey(String, String), > + UpdateValue(String, Value), > + RemoveEntry(String), > +} > + > +impl KeyValueListField { > + fn reset_data(&mut self, data: &[(String, Value)]) { nit: i'd rename this to set_data, since a reset has a very special meaning for fields (reset it to the initial value) but we set here the given data can be fixed up though > + self.store.set_data( > + data.iter() > + .enumerate() > + .map(|(i, (k, v))| Entry { > + index: i as u32, > + key: k.clone(), > + value: v.clone(), > + }) > + .collect(), > + ); > + } > + > + fn columns(ctx: &ManagedFieldContext) -> Rc>> { > + let props = ctx.props().clone(); > + let link = ctx.link().clone(); > + > + Rc::new(vec![ > + DataTableColumn::new(props.key_label.clone()) > + .flex(1) > + .render({ > + let link = link.clone(); > + let props = props.clone(); > + move |item: &Entry| { > + let key = item.key.clone(); > + Field::new() > + .on_change( > + link.callback({ > + move |value| Message::UpdateKey(key.clone(), value) > + }), > + ) > + .required(true) > + .disabled(props.input_props.disabled) > + .placeholder(props.key_placeholder.clone()) > + .validate(|s: &String| { > + if s.is_empty() { > + bail!("Field may not be empty"); > + } else { > + Ok(()) > + } > + }) > + .value(item.key.clone()) > + .into() > + } > + }) > + .sorter(|a: &Entry, b: &Entry| a.key.cmp(&b.key)) > + .into(), > + DataTableColumn::new(props.value_label.clone()) > + .flex(1) > + .render({ > + let link = link.clone(); > + let props = props.clone(); > + move |item: &Entry| { > + let on_change = link.callback({ > + let key = item.key.clone(); > + move |value: String| { > + Message::UpdateValue(key.clone(), Value::String(value)) > + } > + }); > + props.value_renderer.apply(&( > + item.key.clone(), > + item.value.clone(), > + props.input_props.clone(), > + on_change, > + )) > + } > + }) > + .into(), > + DataTableColumn::new("") > + .width("50px") > + .render(move |item: &Entry| { > + let key = item.key.clone(); > + ActionIcon::new("fa fa-lg fa-trash-o") > + .tabindex(0) > + .on_activate(link.callback(move |_| Message::RemoveEntry(key.clone()))) > + .disabled(props.input_props.disabled) > + .into() > + }) > + .into(), > + ]) > + } > +} > + > +impl ManagedField for KeyValueListField { > + type Message = Message; > + type Properties = KeyValueList; > + type ValidateClosure = (bool, Option>>); > + > + fn create(ctx: &ManagedFieldContext) -> Self { > + let store = Store::with_extract_key(|entry: &Entry| Key::from(entry.index)) > + .on_change(ctx.link().callback(|_| Message::DataChange)); > + > + // put the default value through the validator fn, to allow for correct dirty checking > + let default = if let Some(f) = &ctx.props().submit_validate { > + f.apply(&ctx.props().value).unwrap_or_default() > + } else { > + serde_json::to_value(ctx.props().value.clone()).unwrap_or_default() > + }; > + > + let mut this = Self { > + state: ManagedFieldState::new(Value::Null, default), > + store, > + index_counter: AtomicU32::new(ctx.props().value.len() as u32), > + }; > + this.reset_data(&ctx.props().value); > + this > + } > + > + fn validation_args(props: &Self::Properties) -> Self::ValidateClosure { > + (props.input_props.required, props.submit_validate.clone()) > + } > + > + fn validator(props: &Self::ValidateClosure, value: &Value) -> Result { > + let data = serde_json::from_value::>(value.clone())?; > + > + if data.is_empty() && props.0 { > + bail!(tr!("at least one entry required!")); > + } > + > + if data.iter().any(|(k, _)| k.is_empty()) { > + bail!(tr!("name must not be empty!")); > + } > + > + if let Some(cb) = &props.1 { > + cb.apply(&data) > + } else { > + Ok(value.clone()) > + } > + } > + > + fn changed(&mut self, ctx: &ManagedFieldContext, old_props: &Self::Properties) -> bool { > + let props = ctx.props(); > + if old_props.value != props.value { > + let data: Value = props > + .value > + .iter() > + .filter_map(|n| serde_json::to_value(n).ok()) > + .collect(); > + > + ctx.link().update_default(data.clone()); > + } > + true > + } > + > + fn value_changed(&mut self, _ctx: &ManagedFieldContext) { > + match &self.state.value { > + Value::Null => { > + let data = > + serde_json::from_value::>(self.state.default.clone()) > + .unwrap(); > + self.reset_data(&data); > + } > + value => { > + let data = serde_json::from_value::>(value.clone()).unwrap(); > + self.reset_data(&data); > + } > + } > + } > + > + fn update(&mut self, ctx: &ManagedFieldContext, msg: Self::Message) -> bool { > + match msg { > + Message::DataChange => { > + let list: Vec<(String, Value)> = self > + .store > + .read() > + .iter() > + .map(|Entry { key, value, .. }| (key.clone(), value.clone())) > + .collect(); > + > + ctx.link().update_value(serde_json::to_value(list).unwrap()); > + true > + } > + Message::RemoveEntry(key) => { > + self.store.write().retain(|item| item.key != key); > + true > + } > + Message::UpdateKey(old_name, new_name) => { > + let mut data = self.store.write(); > + if let Some(item) = data.iter_mut().find(|item| item.key == old_name) { > + item.key = new_name; > + } > + true > + } > + Message::UpdateValue(key, value) => { > + let mut data = self.store.write(); > + if let Some(item) = data.iter_mut().find(|item| item.key == key) { > + item.value = value; > + } > + true > + } Here is the one grave issue/bug: these three update messages are "dangerous" if i have multiple entries with the same 'key' value, i'd e.g. delete all of them, or update the wrong one. we have the index field in the Entry struct, so just use that instead of key. this would also mean we don't have to clone the whole key string around for the message. > + } > + } > + > + fn view(&self, ctx: &ManagedFieldContext) -> Html { > + let props = ctx.props(); > + > + let table = DataTable::new(Self::columns(ctx), self.store.clone()) since the columsn only depend on the ctx/properties, we can save them in 'fn create' and only have to update them in 'fn changed'. This saves us a bit of work here and the vie method should be faster (it's just an Rc::clone(&self.columns) here) can be fixed up later though. > + .border(true) > + .class(FlexFit); > + > + let button_row = Row::new() > + .with_child( > + Button::new(tr!("Add")) > + .class(ColorScheme::Primary) > + .icon_class("fa fa-plus-circle") > + .disabled(props.input_props.disabled) > + .on_activate({ > + let store = self.store.clone(); > + let index = self.index_counter.fetch_add(1, Ordering::Relaxed); > + move |_| { > + store.write().push(Entry { > + index, > + key: String::new(), > + value: String::new().into(), > + }); > + } > + }), > + ) > + .with_flex_spacer() > + .with_optional_child(self.state.result.clone().err().map(|err| { > + Row::new() > + .class(AlignItems::Center) > + .gap(2) > + .with_child(Fa::new("exclamation-triangle").class(FontColor::Error)) > + .with_child(err) > + })); > + > + Column::new() > + .class(FlexFit) > + .gap(2) > + .with_child( > + Container::from_widget_props(ctx.props().std_props.clone(), None) > + .class(FlexFit) > + .with_child(table), > + ) > + .with_child(button_row) > + .into() > + } > +} > + > +fn default_value_renderer( > + (_key, value, input_props, on_change): &(String, Value, FieldStdProps, Callback), > +) -> Html { > + Field::new() > + .value(match value { > + Value::String(s) => s.to_owned(), > + Value::Number(n) => n.as_i64().unwrap_or_default().to_string(), > + other => other.to_string(), > + }) > + .disabled(input_props.disabled) > + .on_change(on_change) > + .into() > +} > diff --git a/src/lib.rs b/src/lib.rs > index 2b09bf4..dd2ccb0 100644 > --- a/src/lib.rs > +++ b/src/lib.rs > @@ -234,6 +234,9 @@ pub mod utils; > mod xtermjs; > pub use xtermjs::{ConsoleType, ProxmoxXTermJs, XTermJs}; > > +mod key_value_list; > +pub use key_value_list::KeyValueList; > + > use pwt::gettext_noop; > use pwt::state::{LanguageInfo, TextDirection}; >