Re: [pdm-devel] [PATCH proxmox-yew-widget-toolkit 1/1] data table: add get_property helper for displaying optional values

[Stefan Hanreich <s.hanreich@proxmox.com>]

On 8/20/25 11:12 AM, Dominik Csapak wrote:
> wouldn't it also be possible to simply unwrap the option on the source
> side?
> 
> 
> e.g. for a string
> ...
> .get_property(|rec: &Record| rec.option.as_deref().unwrap_or(""))
> ```

This doesn't work for &str because the size is not known at compilation
time, at least this produces an error for me (am I holding it wrong?):

  Some("qwe").as_deref().unwrap_or("")

error[E0277]: the size for values of type `str` cannot be known at
compilation time
   --> src/sdn/evpn/remote_tree.rs:276:18
    |
276 |                 .get_property(|item: &RemoteTreeEntry| {
    |                  ^^^^^^^^^^^^ doesn't have a size known at
compile-time
    |
    = help: the trait `Sized` is not implemented for `str`
note: required by an implicit `Sized` bound in
`DataTableColumn::<T>::get_property`
   -->
/usr/share/cargo/registry/pwt-0.6.4/src/widget/data_table/column.rs:303:25
    |
303 |     pub fn get_property<E: Ord + std::fmt::Display>(
    |                         ^ required by the implicit `Sized`
requirement on this type parameter in `DataTableColumn::<T>::get_property`

The only other option I see is using get_property_owned and cloning
every time:

  Some("qwe")
    .map(str::to_string)
    .unwrap_or_else(|| "".to_string())

But since sort calls the function multiple times we'd clone multiple
times per cell when sorting? Which could be avoided with native support
for Option<&E>.

Of course another alternative is to use Cow / IAttr and the likes
instead if we want to minimize the overhead of cloning but afaict we're
currently using String everywhere? Might be worthwhile to consider
though imo.

> of course you can always implement the renderer and sorter yourself
> (which should only be a single line each if the value has Display + Ord)

which becomes verbose quite quickly for sparsely populated trees - why
not introduce a simple helper to avoid such repetitive code? It avoids
unnecessary cloning (or at least, is one way to do that) and makes the
code more succinct at the same time.

> in general I'm not sure we want to show None values as nothing, this
> largely depends on the data (e.g. valid things to show/expect could be
> '-', 'N/A', etc.)

fair point, but that should be solvable by introducing a default
parameter / callback and using that instead for generating the default
value.

> Also one comment inline
> 
> On 8/19/25 15:25, Stefan Hanreich wrote:
>> In some cases, particularly for tree views, it might make sense to not
>> show values for some cells. In order to simplify handling optional
>> values, add a get_property helper that can work with optional values.
>> It renders the values if it exists and nothing otherwise. Since Option
>> implements Ord, the widget can use its Ord implementation for sorting.
>> The current helpers cannot be used, since Option does not implement
>> Display, so the trait bounds are not satisfied.
>>
>> Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
>> ---
>>   src/widget/data_table/column.rs | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/widget/data_table/column.rs b/src/widget/data_table/
>> column.rs
>> index 016be99..770d7c7 100644
>> --- a/src/widget/data_table/column.rs
>> +++ b/src/widget/data_table/column.rs
>> @@ -5,7 +5,7 @@ use derivative::Derivative;
>>   use yew::html::IntoPropValue;
>>   use yew::prelude::*;
>>   -use yew::virtual_dom::Key;
>> +use yew::virtual_dom::{Key, VNode};
>>     use crate::props::{CallbackMut, IntoEventCallbackMut,
>> IntoSorterFn, RenderFn, SorterFn};
>>   use crate::state::TreeStore;
>> @@ -328,6 +328,22 @@ impl<T: 'static> DataTableColumn<T> {
>>           .render(move |item: &T| html! {{get_property_fn(item)}})
>>       }
>>   +    /// Builder style method to set a get_property_fn for renderer
>> and sorter
>> +    /// the given fn must return the value as an Option
>> +    pub fn get_property_optional<E: Ord + std::fmt::Display>(
>> +        self,
>> +        get_property_fn: impl 'static + Fn(&T) -> Option<&E>,
>> +    ) -> Self {
>> +        let get_property_fn = Rc::new(get_property_fn);
>> +        self.sorter({
>> +            let get_property_fn = get_property_fn.clone();
>> +            move |itema: &T, itemb: &T|
>> get_property_fn(itema).cmp(&get_property_fn(itemb))
>> +        })
>> +        .render(
>> +            move |item: &T| html!
>> {{get_property_fn(item).map(VNode::from).unwrap_or_default()}},
> 
> mhmm since we require the Display trait as boundary, wouldn't a
> to_string() be better here? The default for VNode is a VList, but an
> empty string produces a VText

this allocates a string for every cell, whereas VList doesn't and both
are effectively the same (no HTML produced)

>> +        )
>> +    }
>> +
>>       /// Builder style method for [`Self::set_tree_column`]
>>       pub fn tree_column(mut self, store: impl
>> IntoPropValue<Option<TreeStore<T>>>) -> Self {
>>           self.set_tree_column(store);
> 


_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel