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

["Christoph Heiss" <c.heiss@proxmox.com>]

Thanks for the review!

On Thu Apr 16, 2026 at 2:23 PM CEST, Dominik Csapak wrote:
> high level comment:
>
> i think this would better fit in the yew-comp crate, since it's not a
> general ui pattern.

Fine by me, I can move it. TBH I wasn't completely sure to begin with,
where exactly the line between the yew-widget-toolkit and yew-comp
crates is and put this.

>
> Not a big deal for me though.
>
> other comments inline
>
> On 4/3/26 6:55 PM, Christoph Heiss wrote:
[..]
>> +#[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?

Yes (unfortunately), due to implementing `ManagedField` below.

>
> 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?

That certainly would make a lot of things easier. If it may be even
preferred for yew stuff, I'll go this route.

>
>> +> {
>> +    #[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

Thanks for the hint, I'll change it!

>
>> +
>> +    #[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)

Didn't consider that, definitely makes sense!

>
> 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?

Yeah, that was left to the callee. But that will away with using `Value`
and a custom field renderer callback anyway.

>
>> +
>> +    #[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?

There certainly are usecases where this might be useful. E.g. the
auto-installer integration I'm working on, this component is used to be
able to define filters for udev properties of devices, where (depending
on the selected mode) either all must match or any.

For the latter, having e.g.

  ID_SERIAL=S1ATN*
  ID_SERIAL=SSDSC2*

can be useful, to match devices which match either serial number.

(Also, realised I actually broke this with v2 -> v3 of this series in
the PDM API .. so good time to revisit that as well, I guess.)

>
> 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)

I see, could make things simpler, I'll rework that too.

>
>> +    key: String,
>> +    value: T,
>> +}
>> +
[..]
>> +
>> +    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
>

Good catch, I'll fix both for v4.