Re: [PATCH yew-widget-toolkit v3 13/38] widget: kvlist: add widget for user-modifiable data tables

Dominik Csapak
2026-04-16T12:23:54Z

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 <c.heiss@proxmox.com>
> ---
> 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<KeyValueListField<T>>, @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<SubmitValidateFn<Vec<(String, T)>>>,
> +}
> +
> +impl<T> KeyValueList<T>
> +where
> + T: 'static
> + + Clone
> + + Debug
> + + Default
> + + DeserializeOwned
> + + Display
> + + FromStr
> + + PartialEq
> + + Serialize,
> +{
> + pub fn new() -> Self {
> + yew::props!(Self {})
> + }
> +}
> +
> +#[derive(Clone, Debug, PartialEq)]
> +struct Entry<T: Clone + Debug + PartialEq> {
> + 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<T>
> +where
> + T: 'static
> + + Clone
> + + Debug
> + + Default
> + + DeserializeOwned
> + + Display
> + + FromStr
> + + PartialEq
> + + Serialize,
> +{
> + state: ManagedFieldState,
> + store: Store<Entry<T>>,
> +}
> +
> +impl<T> Deref for KeyValueListField<T>
> +where
> + T: 'static
> + + Clone
> + + Debug
> + + Default
> + + DeserializeOwned
> + + Display
> + + FromStr
> + + PartialEq
> + + Serialize,
> +{
> + type Target = ManagedFieldState;
> +
> + fn deref(&self) -> &Self::Target {
> + &self.state
> + }
> +}
> +
> +impl<T> DerefMut for KeyValueListField<T>
> +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<T> KeyValueListField<T>
> +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::<Vec<(String, T)>>(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<KeyValueListField<T>>,
> + ) -> Rc<Vec<DataTableHeader<Entry<T>>>> {
> + 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<T>| {
> + 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<T>, b: &Entry<T>| a.key.cmp(&b.key))
> + .into(),
> + DataTableColumn::new(props.value_label)
> + .flex(1)
> + .render({
> + let link = link.clone();
> + move |item: &Entry<T>| {
> + 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<T>| {
> + 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<T> ManagedField for KeyValueListField<T>
> +where
> + T: 'static
> + + Clone
> + + Debug
> + + Default
> + + DeserializeOwned
> + + Display
> + + FromStr
> + + PartialEq
> + + Serialize,
> +{
> + type Message = Message;
> + type Properties = KeyValueList<T>;
> + type ValidateClosure = (bool, Option<SubmitValidateFn<Vec<(String, T)>>>);
> +
> + fn create(ctx: &ManagedFieldContext<Self>) -> Self {
> + let store = Store::with_extract_key(|entry: &Entry<T>| 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<Value, Error> {
> + let data = serde_json::from_value::<Vec<(String, T)>>(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<Self>, 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<Self>) {
> + 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<Self>, 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<Self>) -> 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);