Re: [PATCH proxmox-yew-widget-toolkit 3/5] widget: grid picker: keep selected records hidden by the filter

[Dominik Csapak <d.csapak@proxmox.com>]

comments inline

On 10/28/26 4:15 PM, Nicolas Frey wrote:
> the data table drops selected records that do not match the store
> filter. in a grid picker, the filter is only a search aid, so typing a
> query deselected all records that did not match it.
> 
> add an opt-in flag to only drop records that are no longer in the
> store, using a new function `DataStore::contains_key`, and enable it in
> the grid picker.
> 
> Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
> ---
>   src/state/data_store.rs             |  3 ++
>   src/state/store.rs                  |  4 +++
>   src/state/tree_store/mod.rs         |  4 +++
>   src/widget/data_table/data_table.rs | 51 ++++++++++++++++++++++++-----
>   src/widget/grid_picker.rs           |  2 ++
>   5 files changed, 56 insertions(+), 8 deletions(-)
> 
> diff --git a/src/state/data_store.rs b/src/state/data_store.rs
> index 90460c2..16b59ea 100644
> --- a/src/state/data_store.rs
> +++ b/src/state/data_store.rs
> @@ -64,6 +64,9 @@ pub trait DataStore: Clone + PartialEq {
>       fn filtered_record_pos(&self, key: &Key) -> Option<usize>;
>       fn filtered_data_len(&self) -> usize;
>   
> +    /// Returns whether the store contains a record with this key, regardless of the filter.
> +    fn contains_key(&self, key: &Key) -> bool;
> +

This is a bit too narrow for my taste for such an important part of
the interface. I'd probably like more 'unfiltered' methods
that mirror the 'filtered' ones. with that, your contains_key
is just a 'unfiltered_record_pos(_).is_some()'

Also even when adding only the 'contains' method, i'd add the
'unfiltered' part, otherwise this can be misunderstood without
looking at the docs.

>       #[allow(clippy::type_complexity)]
>       fn filtered_data<'a>(
>           &'a self,
> diff --git a/src/state/store.rs b/src/state/store.rs
> index 1afe0a4..89a1ad4 100644
> --- a/src/state/store.rs
> +++ b/src/state/store.rs
> @@ -429,6 +429,10 @@ impl<T: Clone + PartialEq + 'static> DataStore for Store<T> {
>           self.filtered_data_len()
>       }
>   
> +    fn contains_key(&self, key: &Key) -> bool {
> +        self.read().record_pos(key).is_some()
> +    }
> +
>       fn filtered_data<'a>(
>           &'a self,
>       ) -> Box<dyn Iterator<Item = (usize, Box<dyn DataNode<T> + 'a>)> + 'a> {
> diff --git a/src/state/tree_store/mod.rs b/src/state/tree_store/mod.rs
> index 5e84b8f..67f0d76 100644
> --- a/src/state/tree_store/mod.rs
> +++ b/src/state/tree_store/mod.rs
> @@ -288,6 +288,10 @@ impl<T: Clone + PartialEq + 'static> DataStore for TreeStore<T> {
>           self.filtered_data_len()
>       }
>   
> +    fn contains_key(&self, key: &Key) -> bool {
> +        self.read().lookup_node(key).is_some()
> +    }
> +
>       fn filtered_data<'a>(
>           &'a self,
>       ) -> Box<dyn Iterator<Item = (usize, Box<dyn DataNode<T> + 'a>)> + 'a> {
> diff --git a/src/widget/data_table/data_table.rs b/src/widget/data_table/data_table.rs
> index 90f37ea..2ee8ed2 100644
> --- a/src/widget/data_table/data_table.rs
> +++ b/src/widget/data_table/data_table.rs
> @@ -237,6 +237,14 @@ pub struct DataTable<S: DataStore> {
>       #[prop_or_default]
>       pub multiselect_mode: MultiSelectMode,
>   
> +    /// In multiselect mode, keep selected records that are hidden by the store filter.
> +    ///
> +    /// By default, the selection is reduced to the records that match the filter, so that
> +    /// actions on the selection only affect visible rows. Enable this if the filter is only a
> +    /// search aid, and must not change the selection.
> +    #[prop_or_default]
> +    pub keep_filtered_selection: bool,
> +


With this i asked myself: 'what is a filtered selection' ?

I think something like 'keep_selection_when_filtered' would be more
descriptive (even though it's longer).


>       /// Table scroll callback
>       #[prop_or_default]
>       pub on_table_scroll: Option<Callback<Event>>,
> @@ -395,6 +403,17 @@ impl<S: DataStore> DataTable<S> {
>           self.allow_deselect = allow_deselect.into_prop_value();
>       }
>   
> +    /// Builder style method to set the keep_filtered_selection flag.
> +    pub fn keep_filtered_selection(mut self, keep: impl IntoPropValue<bool>) -> Self {
> +        self.set_keep_filtered_selection(keep);
> +        self
> +    }
> +
> +    /// Method to set the keep_filtered_selection flag.
> +    pub fn set_keep_filtered_selection(&mut self, keep: impl IntoPropValue<bool>) {
> +        self.keep_filtered_selection = keep.into_prop_value();
> +    }
> +

these could be automatically implemented by using the #[builder] macro

>       /// Builder style method to set the show_header flag.
>       pub fn show_header(mut self, show_header: impl IntoPropValue<bool>) -> Self {
>           self.set_show_header(show_header);
> @@ -765,6 +784,9 @@ impl<S: DataStore> PwtDataTable<S> {
>           }
>   
>           let mut keys: HashSet<Key> = HashSet::new();
> +        if props.keep_filtered_selection {
> +            keys.extend(selection.selected_keys());
> +        }
>           for pos in 0..record_count {
>               if let Some(key) = props.store.lookup_filtered_record_key(pos) {
>                   keys.insert(key);
> @@ -797,7 +819,11 @@ impl<S: DataStore> PwtDataTable<S> {
>               return;
>           }
>   
> -        let selection_len = selection.len();
> +        // only count visible rows, the selection may contain records hidden by the filter
> +        let selection_len = (0..record_count)
> +            .filter_map(|pos| props.store.lookup_filtered_record_key(pos))
> +            .filter(|key| selection.contains(key))
> +            .count();

since the selection is basically a hashset, we could think
of exposing a the usual set operations too (intersect, difference, etc)
and offer a 'filtered_get_keys' that returns a hashset of the existing
(visible) keys

nothing wrong with this code here, per se but exposing more of the
selection might benefit us in the future too...

>           if record_count == selection_len {
>               self.selection_status = RowSelectionStatus::All;
>           } else if selection_len > 0 {
> @@ -812,13 +838,22 @@ impl<S: DataStore> PwtDataTable<S> {
>           if let Some(selection) = &props.selection {
>               let mut selection = selection.write();
>               if selection.is_multiselect() {
> -                let mut keys: HashSet<Key> = HashSet::new();
> -                for (_pos, node) in props.store.filtered_data() {
> -                    let key = props.store.extract_key(&node.record());
> -                    if selection.contains(&key) {
> -                        keys.insert(key);
> -                    }
> -                }
> +                let keys = if props.keep_filtered_selection {
> +                    selection
> +                        .selected_keys()
> +                        .into_iter()
> +                        .filter(|key| props.store.contains_key(key))
> +                        .collect()
> +                } else {
> +                    props
> +                        .store
> +                        .filtered_data()
> +                        .filter_map(|(_pos, node)| {
> +                            let key = props.store.extract_key(&node.record());
> +                            selection.contains(&key).then_some(key)
> +                        })
> +                        .collect()
> +                };
>                   selection.bulk_select(keys);
>               } else if let Some(key) = selection.selected_key() {
>                   if props.store.filtered_record_pos(&key).is_none() {
> diff --git a/src/widget/grid_picker.rs b/src/widget/grid_picker.rs
> index b40ac92..d3ac5ae 100644
> --- a/src/widget/grid_picker.rs
> +++ b/src/widget/grid_picker.rs
> @@ -185,6 +185,8 @@ impl<S: DataStore + 'static> Component for PwtGridPicker<S> {
>               .clone()
>               .key(Key::from("picker-table"))
>               .autoselect(false)
> +            // the filter is only a search aid and must not change the selection
> +            .keep_filtered_selection(true)
>               .cell_configuration(CellConfiguration::new().class("pwt-datatable-cell pwt-pointer"))
>               .hover(true)
>               .header_focusable(false)