Fix drag/drop behavior for duplicate group/split columns

finos · Apr 10, 2023 · ae28c9a · ae28c9a
1 parent 1b5be78
commit ae28c9a
Show file tree

Hide file tree

Showing 6 changed files with 75 additions and 28 deletions.
diff --git a/rust/perspective-viewer/src/rust/components/column_selector.rs b/rust/perspective-viewer/src/rust/components/column_selector.rs
@@ -233,7 +233,7 @@ impl Component for ColumnSelector {
                 + config.split_by.len()
                 + config.filter.len()
                 + config.sort.len()) as f64,
-            212.0,
+            224.0,
         );
 
         let config_selector = html_nested! {

diff --git a/rust/perspective-viewer/src/rust/components/column_selector/config_selector.rs b/rust/perspective-viewer/src/rust/components/column_selector/config_selector.rs
@@ -87,6 +87,10 @@ impl DragContext<ConfigSelectorMsg> for GroupByContext {
     fn create(col: InPlaceColumn) -> ConfigSelectorMsg {
         ConfigSelectorMsg::New(DragTarget::GroupBy, col)
     }
+
+    fn is_self_move(target: DragTarget) -> bool {
+        target == DragTarget::GroupBy
+    }
 }
 
 impl DragContext<ConfigSelectorMsg> for SplitByContext {
@@ -105,6 +109,10 @@ impl DragContext<ConfigSelectorMsg> for SplitByContext {
     fn create(col: InPlaceColumn) -> ConfigSelectorMsg {
         ConfigSelectorMsg::New(DragTarget::SplitBy, col)
     }
+
+    fn is_self_move(target: DragTarget) -> bool {
+        target == DragTarget::SplitBy
+    }
 }
 
 impl DragContext<ConfigSelectorMsg> for SortDragContext {
@@ -123,6 +131,10 @@ impl DragContext<ConfigSelectorMsg> for SortDragContext {
     fn create(col: InPlaceColumn) -> ConfigSelectorMsg {
         ConfigSelectorMsg::New(DragTarget::Sort, col)
     }
+
+    fn is_self_move(target: DragTarget) -> bool {
+        target == DragTarget::Sort
+    }
 }
 
 impl DragContext<ConfigSelectorMsg> for FilterDragContext {
@@ -141,6 +153,10 @@ impl DragContext<ConfigSelectorMsg> for FilterDragContext {
     fn create(col: InPlaceColumn) -> ConfigSelectorMsg {
         ConfigSelectorMsg::New(DragTarget::Filter, col)
     }
+
+    fn is_self_move(target: DragTarget) -> bool {
+        target == DragTarget::Filter
+    }
 }
 
 type GroupBySelector = DragDropList<ConfigSelector, PivotColumn, GroupByContext>;

diff --git a/rust/perspective-viewer/src/rust/components/containers/dragdrop_list.rs b/rust/perspective-viewer/src/rust/components/containers/dragdrop_list.rs
@@ -30,6 +30,7 @@ pub trait DragContext<T> {
     fn dragleave() -> T;
     fn dragenter(index: usize) -> T;
     fn create(col: InPlaceColumn) -> T;
+    fn is_self_move(effect: DragTarget) -> bool;
 }
 
 #[derive(Properties, Derivative)]
@@ -193,43 +194,54 @@ where
             }
         });
 
+        let invalid_drag: bool;
         let columns_html = {
             let mut columns = ctx
                 .props()
                 .children
                 .iter()
                 .map(|x| (true, Some(x)))
+                .enumerate()
                 .collect::<Vec<_>>();
 
-            if let Some((x, column)) = &ctx.props().is_dragover {
+            invalid_drag = if let Some((x, column)) = &ctx.props().is_dragover {
                 let index = *x;
-                let col_vchild = columns
+                let is_append = index == columns.len();
+                let is_self_move = ctx
+                    .props()
+                    .dragdrop
+                    .get_drag_target()
+                    .map(|x| V::is_self_move(x))
+                    .unwrap_or_default();
+
+                let is_duplicate = columns
                     .iter()
-                    .map(|z| z.1.as_ref().unwrap())
-                    .find(|x| x.props.get_item() == *column)
-                    .cloned();
+                    .position(|x| x.1 .1.as_ref().unwrap().props.get_item() == *column);
 
-                let changed = if !ctx.props().allow_duplicates {
-                    let old_size = columns.len();
-                    columns.retain(|x| x.1.as_ref().unwrap().props.get_item() != *column);
-                    columns.len() < old_size
-                } else {
-                    false
-                };
+                if let Some(duplicate) = is_duplicate && !ctx.props().allow_duplicates &&  !is_append {
+                    columns.remove(duplicate);
+                } else if ctx.props().allow_duplicates && !is_append && is_self_move {
+                    columns.remove(is_duplicate.unwrap());
+                }
 
                 // If inserting into the middle of the list, use
                 // the length of the existing element to prevent
                 // jitter as the underlying dragover zone moves.
                 if index < columns.len() {
-                    columns.insert(index, (false, col_vchild));
+                    columns.insert(index, (usize::MAX, (false, None)));
+                    false
+                } else if (!is_append && !ctx.props().allow_duplicates)
+                    || ((!is_append || !is_self_move)
+                        && (is_duplicate.is_none() || ctx.props().allow_duplicates))
+                {
+                    columns.push((usize::MAX, (false, None)));
+                    false
                 } else {
-                    columns.push((false, col_vchild));
-                }
-
-                if changed {
-                    columns.push((true, None));
+                    true
                 }
-            }
+            } else {
+                false
+            };
 
             columns
                 .into_iter()
@@ -246,23 +258,24 @@ where
                         }
                     });
 
-                    if let (true, Some(column)) = column {
+                    if let (key, (true, Some(column))) = column {
                         html! {
-                            <div class="pivot-column" ondragenter={ dragenter }>
+                            <div { key } class="pivot-column" ondragenter={ dragenter }>
                                 { Html::from(column) }
                                 <span class="row_close" onmousedown={ close }></span>
                             </div>
                         }
-                    } else if let (_, Some(column)) = column {
+                    } else if let (key, (_, Some(column))) = column {
                         html! {
-                            <div class="pivot-column" ondragenter={ dragenter }>
+                            <div { key } class="pivot-column" ondragenter={ dragenter }>
                                 { Html::from(column) }
                                 <span class="row_close" style="opacity:0.3"></span>
                             </div>
                         }
                     } else {
+                        let (key, _) = column;
                         html! {
-                            <div class="pivot-column" ondragenter={ dragenter }>
+                            <div { key }  class="pivot-column" ondragenter={ dragenter }>
                                 <div class="config-drop"></div>
                             </div>
                         }
@@ -287,7 +300,7 @@ where
                     <div class="psp-text-field">
                         <ul class="psp-text-field__input" for={ ctx.props().name }>
                             { columns_html }
-                            if ctx.props().is_dragover.is_none() {
+                            if ctx.props().is_dragover.is_none() || invalid_drag {
                                 <EmptyColumn
                                     { column_dropdown }
                                     { exclude }

diff --git a/rust/perspective-viewer/src/rust/dragdrop.rs b/rust/perspective-viewer/src/rust/dragdrop.rs
@@ -103,6 +103,23 @@ impl DragDrop {
         }
     }
 
+    pub fn get_drag_target(&self) -> Option<DragTarget> {
+        match *self.drag_state.borrow() {
+            DragState::DragInProgress(DragFrom {
+                effect: DragEffect::Move(target),
+                ..
+            })
+            | DragState::DragOverInProgress(
+                DragFrom {
+                    effect: DragEffect::Move(target),
+                    ..
+                },
+                _,
+            ) => Some(target),
+            _ => None,
+        }
+    }
+
     pub fn set_drag_image(&self, event: &DragEvent) -> ApiResult<()> {
         event.stop_propagation();
         if let Some(dt) = event.data_transfer() {

diff --git a/rust/perspective-viewer/src/rust/lib.rs b/rust/perspective-viewer/src/rust/lib.rs
@@ -10,6 +10,7 @@
 #![recursion_limit = "1024"]
 #![feature(const_type_name)]
 #![feature(macro_metavar_expr)]
+#![feature(let_chains)]
 #![feature(anonymous_lifetime_in_impl_trait)]
 #![warn(
     clippy::all,

diff --git a/rust/perspective-viewer/test/results/results.json b/rust/perspective-viewer/test/results/results.json
@@ -3,7 +3,7 @@
     "superstore.html/doesn't leak elements.": "d0fd18b3d4d7c183c5ed155b4bf37972",
     "superstore.html/doesn't leak views when setting group by.": "54daaa4bbbe59f6ed4acc301ba871bab",
     "superstore.html/doesn't leak views when setting filters.": "6dfc1e505f1428424c3265f0236f22fc",
-    "__GIT_COMMIT__": "e8058857eca31dae227def42d3bed8a49f2e2345",
+    "__GIT_COMMIT__": "78c0f588ba98a2aee406e6b3c3b594af6b4cb646",
     "blank.html/Handles reloading with a schema.": "e58c62f6e0ff16dc4d753f99e0fc39c3",
     "superstore_shows_a_grid_without_any_settings_applied_": "ae1c4690d978598ca14c8669244ce604",
     "superstore_Responsive_Layout_shows_horizontal_columns_on_small_vertical_viewports_": "57ba3ad341cf8a0e4df6ab96715ff2a0",
@@ -57,7 +57,7 @@
     "superstore_doesn_t_leak_elements": "8abeada4188bb6c316b8964861a60f43",
     "superstore_doesn_t_leak_views_when_setting_group_by": "fd75dd7101bf8501a3271a6c2e5b5032",
     "superstore_doesn_t_leak_views_when_setting_filters": "2f523e33ebbec489590323f36fd07dae",
-    "superstore_opens_settings_when_field_is_set_to_true": "f7fadaa4836d0106b346b44bc19e342f",
+    "superstore_opens_settings_when_field_is_set_to_true": "5c4daad07beddff125611b169474ad8e",
     "superstore_opens_settings_when_field_is_set_to_false": "80705a191c3675ed34d3058874f09a00",
     "superstore_save()_returns_the_current_config": "ded04b5d6cb96a3651578334f189b20e",
     "superstore_restore()_restores_a_config_from_save()": "ded04b5d6cb96a3651578334f189b20e",