From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id D4C3A1FF0DF for ; Thu, 05 Nov 2026 11:17:13 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id A793B21413; Thu, 05 Nov 2026 11:17:13 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 05 Nov 2026 11:17:08 +0100 Message-Id: Subject: Re: [PATCH proxmox-yew-widget-toolkit 3/5] widget: grid picker: keep selected records hidden by the filter From: "Nicolas Frey" To: "Dominik Csapak" , X-Mailer: aerc 0.20.0 References: <20261028151517.518742-1-n.frey@proxmox.com> <20261028151517.518742-4-n.frey@proxmox.com> <53d706ba-5154-4369-8d67-ccb009adb23a@proxmox.com> In-Reply-To: <53d706ba-5154-4369-8d67-ccb009adb23a@proxmox.com> X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793873828715 X-SPAM-LEVEL: Spam detection results: 0 AWL 1.189 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust 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: ABDROPVOD5C4GB7LX5XVBZPXSULEFS4Z X-Message-ID-Hash: ABDROPVOD5C4GB7LX5XVBZPXSULEFS4Z X-MailFrom: n.frey@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 X-Mailman-Version: 3.3.10 Precedence: list List-Id: Yew framework devel list at Proxmox List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Mon Nov 2, 2026 at 3:12 PM CET, Dominik Csapak wrote: > 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 >> --- >> 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; >> fn filtered_data_len(&self) -> usize; >> >> + /// Returns whether the store contains a record with this key, rega= rdless 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. > makes sense, will adjust in a v2, thanks! >> #[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 DataStore for = Store { >> 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 + '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 DataStore for = TreeStore { >> 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 + '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 { >> #[prop_or_default] >> pub multiselect_mode: MultiSelectMode, >> >> + /// In multiselect mode, keep selected records that are hidden by t= he 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). > ack > >> /// Table scroll callback >> #[prop_or_default] >> pub on_table_scroll: Option>, >> @@ -395,6 +403,17 @@ impl DataTable { >> self.allow_deselect =3D 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) -> 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 IntoPropVa= lue) { >> + self.keep_filtered_selection =3D keep.into_prop_value(); >> + } >> + > > these could be automatically implemented by using the #[builder] macro > ah yeah forgot, as all the other ones are written out without the macro I just went with this. I'll update the others with #[builder] too in a separate commit >> /// Builder style method to set the show_header flag. >> pub fn show_header(mut self, show_header: impl IntoPropValue= ) -> Self { >> self.set_show_header(show_header); >> @@ -765,6 +784,9 @@ impl PwtDataTable { >> } >> >> let mut keys: HashSet =3D HashSet::new(); >> + if props.keep_filtered_selection { >> + keys.extend(selection.selected_keys()); >> + } >> for pos in 0..record_count { >> if let Some(key) =3D props.store.lookup_filtered_record_ke= y(pos) { >> keys.insert(key); >> @@ -797,7 +819,11 @@ impl PwtDataTable { >> return; >> } >> >> - let selection_len =3D selection.len(); >> + // only count visible rows, the selection may contain records h= idden by the filter >> + let selection_len =3D (0..record_count) >> + .filter_map(|pos| props.store.lookup_filtered_record_key(po= s)) >> + .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... > ack >> if record_count =3D=3D selection_len { >> self.selection_status =3D RowSelectionStatus::All; >> } else if selection_len > 0 { >> @@ -812,13 +838,22 @@ impl PwtDataTable { >> if let Some(selection) =3D &props.selection { >> let mut selection =3D selection.write(); >> if selection.is_multiselect() { >> - let mut keys: HashSet =3D HashSet::new(); >> - for (_pos, node) in props.store.filtered_data() { >> - let key =3D props.store.extract_key(&node.record())= ; >> - if selection.contains(&key) { >> - keys.insert(key); >> - } >> - } >> + let keys =3D 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 =3D props.store.extract_key(&node.r= ecord()); >> + selection.contains(&key).then_some(key) >> + }) >> + .collect() >> + }; >> selection.bulk_select(keys); >> } else if let Some(key) =3D 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 Component for PwtGridPi= cker { >> .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-da= tatable-cell pwt-pointer")) >> .hover(true) >> .header_focusable(false)