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 6A25E1FF13C for ; Thu, 16 Apr 2026 14:23:54 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 968374580; Thu, 16 Apr 2026 14:23:50 +0200 (CEST) Message-ID: <3d4e0b0e-cd51-4041-87f9-9782be422357@proxmox.com> Date: Thu, 16 Apr 2026 14:23:43 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH yew-widget-toolkit v3 13/38] widget: kvlist: add widget for user-modifiable data tables To: Christoph Heiss , pdm-devel@lists.proxmox.com References: <20260403165437.2166551-1-c.heiss@proxmox.com> <20260403165437.2166551-14-c.heiss@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20260403165437.2166551-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: 1776342144907 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.048 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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. [mod.rs] Message-ID-Hash: PPLD7VK2MHOH3QAZU6JB5WJLGL6IYLVU X-Message-ID-Hash: PPLD7VK2MHOH3QAZU6JB5WJLGL6IYLVU 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: high level comment: i think this would better fit in the yew-comp crate, since it's not a general ui pattern. Not a big deal for me though. other comments inline On 4/3/26 6:55 PM, Christoph Heiss wrote: > A yew-based variant of the existing extjs > `Proxmox.form.WebhookKeyValueList`, but also generic over the value > type. > > Signed-off-by: Christoph Heiss > --- > Changes v2 -> v3: > * new patch > > src/widget/key_value_list.rs | 429 +++++++++++++++++++++++++++++++++++ > src/widget/mod.rs | 3 + > 2 files changed, 432 insertions(+) > create mode 100644 src/widget/key_value_list.rs > > diff --git a/src/widget/key_value_list.rs b/src/widget/key_value_list.rs > new file mode 100644 > index 0000000..80f69a0 > --- /dev/null > +++ b/src/widget/key_value_list.rs > @@ -0,0 +1,429 @@ > +use anyhow::{Error, bail}; > +use serde::{Deserialize, Serialize, de::DeserializeOwned}; > +use serde_json::Value; > +use std::{ > + fmt::{Debug, Display}, > + ops::{Deref, DerefMut}, > + rc::Rc, > + str::FromStr, > +}; > +use yew::virtual_dom::Key; > + > +use crate::{ > + css::{AlignItems, ColorScheme, FlexFit, FontColor}, > + prelude::*, > + state::Store, > + widget::{ > + ActionIcon, Button, Column, Container, Fa, Row, > + data_table::{DataTable, DataTableColumn, DataTableHeader}, > + form::{ > + Field, InputType, IntoSubmitValidateFn, ManagedField, ManagedFieldContext, > + ManagedFieldMaster, ManagedFieldScopeExt, ManagedFieldState, SubmitValidateFn, > + }, > + }, > +}; > +use pwt_macros::{builder, widget}; > + > +#[widget(pwt = crate, 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< > + T: 'static > + + Clone > + + Debug > + + Default > + + DeserializeOwned > + + Display > + + FromStr > + + PartialEq > + + Serialize, does the type really need all of these traits? usually traits are only defined in the implementation blocks where this is needed, not as fixed types on the struct itself also e.g it's usually expected that Display and FromStr should be compatible, so that to_string().parse().unwrap() should work. so having them need to serialize/deserialize could be dropped here and parse/to_string is used? Or we just use 'Value' instead of making it generic? > +> { > + #[builder] > + #[prop_or_default] > + /// Initial value pairs to display. > + pub value: Vec<(String, T)>, > + > + #[builder] > + #[prop_or(tr!("Name"))] > + /// Label for the key column, defaults to "Name". > + pub key_label: String, > + > + #[builder] > + #[prop_or_default] > + /// Placeholder to display in the key columns fields, default is no placeholder. > + pub key_placeholder: String, > + > + #[builder] > + #[prop_or(tr!("Value"))] > + /// Label for the value column. > + pub value_label: String, > + > + #[builder] > + #[prop_or_default] > + /// Placeholder to display in the value columns fields, default is no placeholder. > + pub value_placeholder: String, I know we are a bit careless with it in the codebase, but it's generally recommended to use 'AttrValue' instead of String as properties. Reason is that AttrValue can be cheaply cloned if the value is just a &'static str > + > + #[builder] > + #[prop_or_default] > + /// Input type to set on the value columns fields, default is text. > + pub value_input_type: InputType, here i noticed that you use a fixed 'Field' for the value column. Maybe it would better to just use a text field and optionally let the user set a fieldrenderer callback? imho just setting the type is too restricting, e.g. for numbers i can't set the minimum/maximum/step size etc. (and you use it only once AFAICS) also what is the relationship between this field and the actual value type? if i set "number" here, but T is e.g. String this does not make much sense? > + > + #[builder_cb(IntoSubmitValidateFn, into_submit_validate_fn, Vec<(String, T)>)] > + #[prop_or_default] > + /// Callback to run on submit on the data in the table. > + pub submit_validate: Option>>, > +} > + > +impl KeyValueList > +where > + T: 'static > + + Clone > + + Debug > + + Default > + + DeserializeOwned > + + Display > + + FromStr > + + PartialEq > + + Serialize, > +{ > + pub fn new() -> Self { > + yew::props!(Self {}) > + } > +} > + > +#[derive(Clone, Debug, PartialEq)] > +struct Entry { > + index: usize, you use this as key, but does it really make sense to have multiple entries with the same 'key' field? if we could restrict ourselves to only having unique 'key' fields this field could be dropped, and the adding/removing logic could get simpler. alternatively, if we don't need the key at all (only as a marker for removal/adding), we could keep this, but only ever increase the count, so we don't have to change all indices when an entry gets removed (it's what extjs does too when it autogenerates keys, it never reuses one, always increases the counter) > + key: String, > + value: T, > +} > + > +pub struct KeyValueListField > +where > + T: 'static > + + Clone > + + Debug > + + Default > + + DeserializeOwned > + + Display > + + FromStr > + + PartialEq > + + Serialize, > +{ > + state: ManagedFieldState, > + store: Store>, > +} > + > +impl Deref for KeyValueListField > +where > + T: 'static > + + Clone > + + Debug > + + Default > + + DeserializeOwned > + + Display > + + FromStr > + + PartialEq > + + Serialize, > +{ > + type Target = ManagedFieldState; > + > + fn deref(&self) -> &Self::Target { > + &self.state > + } > +} > + > +impl DerefMut for KeyValueListField > +where > + T: 'static > + + Clone > + + Debug > + + Default > + + DeserializeOwned > + + Display > + + FromStr > + + PartialEq > + + Serialize, > +{ > + fn deref_mut(&mut self) -> &mut Self::Target { > + &mut self.state > + } > +} > + > +pub enum Message { > + DataChange, > + UpdateKey(usize, String), > + UpdateValue(usize, String), > + RemoveEntry(usize), > +} > + > +impl KeyValueListField > +where > + T: 'static > + + Clone > + + Debug > + + Default > + + DeserializeOwned > + + for<'d> Deserialize<'d> > + + Display > + + FromStr > + + PartialEq > + + Serialize, > +{ > + fn set_data(&mut self, data: Vec<(String, T)>) { > + self.store.set_data( > + data.into_iter() > + .enumerate() > + .map(|(index, (key, value))| Entry { index, key, value }) > + .collect(), > + ); > + } > + > + pub fn sync_from_value(&mut self, value: Value) { > + match serde_json::from_value::>(value) { > + Ok(items) => self.set_data(items), > + Err(_err) => { > + // unable to parse list, likely caused by the user editing items. > + // simply ignore errors > + } > + } > + } > + > + fn columns( > + ctx: &ManagedFieldContext>, > + ) -> Rc>>> { > + let props = ctx.props().clone(); > + let link = ctx.link().clone(); > + > + Rc::new(vec![ > + DataTableColumn::new(props.key_label) > + .flex(1) > + .render({ > + let link = link.clone(); > + move |item: &Entry| { > + let index = item.index; > + Field::new() > + .on_change(link.callback(move |value| Message::UpdateKey(index, 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) > + .flex(1) > + .render({ > + let link = link.clone(); > + move |item: &Entry| { > + let index = item.index; > + let value = &item.value; > + Field::new() > + .input_type(props.value_input_type) > + .on_change( > + link.callback(move |value| Message::UpdateValue(index, value)), > + ) > + .disabled(props.input_props.disabled) > + .placeholder(props.value_placeholder.clone()) > + .value(value.to_string()) > + .into() > + } > + }) > + .into(), > + DataTableColumn::new("") > + .width("50px") > + .render(move |item: &Entry| { > + let index = item.index; > + ActionIcon::new("fa fa-lg fa-trash-o") > + .tabindex(0) > + .on_activate(link.callback(move |_| Message::RemoveEntry(index))) > + .disabled(props.input_props.disabled) > + .into() > + }) > + .into(), > + ]) > + } > +} > + > +impl ManagedField for KeyValueListField > +where > + T: 'static > + + Clone > + + Debug > + + Default > + + DeserializeOwned > + + Display > + + FromStr > + + PartialEq > + + Serialize, > +{ > + 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)); > + > + let value = Value::Null; > + > + // 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, default), > + store, > + }; > + > + this.set_data(ctx.props().value.clone()); > + 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!("at least one entry required!") > + } > + > + if data.iter().any(|(k, _)| k.is_empty()) { > + bail!("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 default: Value = props > + .value > + .iter() > + .filter_map(|n| serde_json::to_value(n).ok()) > + .collect(); > + ctx.link().update_default(default.clone()); > + self.sync_from_value(default); > + } > + true > + } > + > + fn value_changed(&mut self, _ctx: &ManagedFieldContext) { > + match self.state.value { > + Value::Null => self.sync_from_value(self.state.default.clone()), > + _ => self.sync_from_value(self.state.value.clone()), > + } > + } > + > + fn update(&mut self, ctx: &ManagedFieldContext, msg: Self::Message) -> bool { > + match msg { > + Message::DataChange => { > + let list: Vec<(String, T)> = 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(index) => { > + let data: Vec<(String, T)> = self > + .store > + .read() > + .iter() > + .filter(move |item| item.index != index) > + .map(|Entry { key, value, .. }| (key.clone(), value.clone())) > + .collect(); > + self.set_data(data); > + true > + } > + Message::UpdateKey(index, key) => { > + let mut data = self.store.write(); > + if let Some(item) = data.get_mut(index) { > + item.key = key; > + } > + true > + } > + Message::UpdateValue(index, value) => { > + let mut data = self.store.write(); > + if let Some(item) = data.get_mut(index) { > + if let Ok(v) = T::from_str(&value) { > + item.value = v; > + } > + } > + true > + } > + } > + } > + > + fn view(&self, ctx: &ManagedFieldContext) -> Html { > + let table = DataTable::new(Self::columns(ctx), self.store.clone()) > + .border(true) > + .class(FlexFit); > + > + let button_row = Row::new() > + .with_child( > + Button::new(tr!("Add")) > + .class(ColorScheme::Primary) > + .icon_class("fa fa-plus-circle") > + .on_activate({ > + let store = self.store.clone(); > + move |_| { > + let mut data = store.write(); > + let index = data.len(); > + > + data.push(Entry { > + index, > + key: String::new(), > + value: T::default(), > + }) > + } > + }), this button is not properly disabled when the whole field is disabled so it's always active, also it would be better if this was also an update message, like the removal, then the adding/removing/updating code is all together > + ) > + .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() > + } > +} > diff --git a/src/widget/mod.rs b/src/widget/mod.rs > index 0df2cbf..a6f2836 100644 > --- a/src/widget/mod.rs > +++ b/src/widget/mod.rs > @@ -189,6 +189,9 @@ pub use tooltip::Tooltip; > mod visibility_observer; > pub use visibility_observer::VisibilityObserver; > > +mod key_value_list; > +pub use key_value_list::KeyValueList; > + > use std::sync::atomic::{AtomicUsize, Ordering}; > > static UNIQUE_ELEMENT_ID: AtomicUsize = AtomicUsize::new(0);