From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 507E41FF13C for ; Thu, 16 Apr 2026 16:19:29 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 205577B14; Thu, 16 Apr 2026 16:19:29 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 16 Apr 2026 16:18:54 +0200 Message-Id: Subject: Re: [PATCH yew-widget-toolkit v3 13/38] widget: kvlist: add widget for user-modifiable data tables From: "Christoph Heiss" To: "Dominik Csapak" X-Mailer: aerc 0.21.0 References: <20260403165437.2166551-1-c.heiss@proxmox.com> <20260403165437.2166551-14-c.heiss@proxmox.com> <3d4e0b0e-cd51-4041-87f9-9782be422357@proxmox.com> In-Reply-To: <3d4e0b0e-cd51-4041-87f9-9782be422357@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1776349055457 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.069 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 Message-ID-Hash: LHZKG77SUF6X3554OME6MPBFALEIABAM X-Message-ID-Hash: LHZKG77SUF6X3554OME6MPBFALEIABAM X-MailFrom: c.heiss@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 CC: pdm-devel@lists.proxmox.com 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: 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 =3D crate, comp =3D ManagedFieldMaster>, @input)] >> +#[derive(Clone, PartialEq, Properties)] >> +#[builder] >> +/// A [`DataTable`]-based grid to hold a list of user-enterable key-val= ue pairs. >> +/// >> +/// Displays a [`DataTable`] with three columns; key, value and a delet= e 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 typ= e? > > 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<(St= ring, 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? 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=3DS1ATN* ID_SERIAL=3DSSDSC2* 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) -> Html { >> + let table =3D DataTable::new(Self::columns(ctx), self.store.clo= ne()) >> + .border(true) >> + .class(FlexFit); >> + >> + let button_row =3D Row::new() >> + .with_child( >> + Button::new(tr!("Add")) >> + .class(ColorScheme::Primary) >> + .icon_class("fa fa-plus-circle") >> + .on_activate({ >> + let store =3D self.store.clone(); >> + move |_| { >> + let mut data =3D store.write(); >> + let index =3D 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.