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

[Dominik Csapak]

On 8/20/25 12:33, Dominik Csapak wrote:
> 
> 
> On 8/20/25 12:29, Stefan Hanreich wrote:
>>
>>
>> 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`
> 
> if you have an Option<&str> you can omit the .as_deref()
> ```
> Some("foo").unwrap_or("")
> ```
> 
> works
> 
>>
>> 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.


i mean if we have 1 or two lines per column for that is not that much,
since i hope we don't have many trees with > 10 columns?

but maybe you can share a concrete code example before and after?
that would make the decision a bit easier


an additional reason for why we don't want to overuse these helpers
is that they create an Rc from the callback, which will, when done
from e.g. the render method, change everytime. Since the render and
sorter are properties of the column, they will be rerendered even if
the callback or the captured data does not change

Often we create the columns once in an rc and save and use that,
but that is also not pretty and nobody is forcing one to do that,
so accidental re-renders can happen very easily

>>
>>> 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 at 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)
> 
> yes it does, VNode::from for T which impl ToString (implicitly by 
> Display) uses to_string too:
> 
> https://docs.rs/yew/latest/src/yew/virtual_dom/vnode.rs.html#164-166


sorry i misinterpreted your answer.. can you point me to the difference
of VList vs VText rendering... i can't find it atm>
>>
>>>> +        )
>>>> +    }
>>>> +
>>>>        /// 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 at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel