From c368d855c7886d9560ac4130cbdf99b87b8f1714 Mon Sep 17 00:00:00 2001 From: Richard Shiue <71320345+richardshiue@users.noreply.github.com> Date: Fri, 21 Jul 2023 15:51:20 +0800 Subject: [PATCH] feat: enable sort by date + sort fix (#2953) --- .../integration_test/database_sort_test.dart | 105 +++++++++--------- .../application/field/field_controller.dart | 1 + .../checkbox_type_option.rs | 4 + .../checklist_type_option/checklist.rs | 4 + .../date_type_option/date_type_option.rs | 4 + .../number_type_option/number_type_option.rs | 4 + .../multi_select_type_option.rs | 4 + .../single_select_type_option.rs | 4 + .../text_type_option/text_type_option.rs | 4 + .../field/type_options/type_option.rs | 2 + .../field/type_options/type_option_cell.rs | 32 +++++- .../url_type_option/url_type_option.rs | 5 + .../src/services/sort/controller.rs | 73 +++++++----- .../sort_test/checkbox_and_text_test.rs | 9 +- .../database/sort_test/multi_sort_test.rs | 12 +- .../database/sort_test/single_sort_test.rs | 12 +- 16 files changed, 178 insertions(+), 101 deletions(-) diff --git a/frontend/appflowy_flutter/integration_test/database_sort_test.dart b/frontend/appflowy_flutter/integration_test/database_sort_test.dart index 6f7f0273db..628763cfa3 100644 --- a/frontend/appflowy_flutter/integration_test/database_sort_test.dart +++ b/frontend/appflowy_flutter/integration_test/database_sort_test.dart @@ -10,22 +10,22 @@ void main() { group('grid', () { testWidgets('add text sort', (tester) async { await tester.openV020database(); - // create a filter + // create a sort await tester.tapDatabaseSortButton(); await tester.tapCreateSortByFieldType(FieldType.RichText, 'Name'); // check the text cell order final textCells = [ - '', - '', - '', - '', - '', 'A', 'B', 'C', 'D', 'E', + '', + '', + '', + '', + '', ]; for (final (index, content) in textCells.indexed) { await tester.assertCellContent( @@ -142,7 +142,6 @@ void main() { // check the number cell order for (final (index, content) in [ - '', '-2', '-1', '0.1', @@ -152,6 +151,7 @@ void main() { '10', '11', '12', + '', ].indexed) { await tester.assertCellContent( rowIndex: index, @@ -186,53 +186,74 @@ void main() { await tester.pumpAndSettle(); }); - testWidgets('add number and text sort', (tester) async { + testWidgets('add checkbox and number sort', (tester) async { await tester.openV020database(); // create a filter await tester.tapDatabaseSortButton(); - await tester.tapCreateSortByFieldType(FieldType.Number, 'number'); + await tester.tapCreateSortByFieldType(FieldType.Checkbox, 'Done'); - // open the sort menu and select number order by descending + // open the sort menu and sort checkbox by descending await tester.tapSortMenuInSettingBar(); - await tester.tapSortButtonByName('number'); + await tester.tapSortButtonByName('Done'); await tester.tapSortByDescending(); - for (final (index, content) in [ - '12', - '11', - '10', - '2', - '1', - '0.2', - '0.1', - '-1', - '-2', - '', + for (final (index, content) in [ + true, + true, + true, + true, + true, + false, + false, + false, + false, + false, ].indexed) { - await tester.assertCellContent( + await tester.assertCheckboxCell( rowIndex: index, - fieldType: FieldType.Number, - content: content, + isSelected: content, ); } + // add another sort, this time by number descending await tester.tapSortMenuInSettingBar(); await tester.tapCreateSortByFieldTypeInSortMenu( - FieldType.RichText, - 'Name', + FieldType.Number, + 'number', ); + await tester.tapSortButtonByName('number'); + await tester.tapSortByDescending(); + + // check checkbox cell order + for (final (index, content) in [ + true, + true, + true, + true, + true, + false, + false, + false, + false, + false, + ].indexed) { + await tester.assertCheckboxCell( + rowIndex: index, + isSelected: content, + ); + } // check number cell order for (final (index, content) in [ + '1', + '0.2', + '0.1', + '-1', + '-2', '12', '11', '10', '2', '', - '-1', - '-2', - '0.1', - '0.2', - '1', ].indexed) { await tester.assertCellContent( rowIndex: index, @@ -241,26 +262,6 @@ void main() { ); } - // check text cell order - for (final (index, content) in [ - '', - '', - '', - '', - '', - 'A', - 'B', - 'C', - 'D', - 'E', - ].indexed) { - await tester.assertCellContent( - rowIndex: index, - fieldType: FieldType.RichText, - content: content, - ); - } - await tester.pumpAndSettle(); }); }); diff --git a/frontend/appflowy_flutter/lib/plugins/database_view/application/field/field_controller.dart b/frontend/appflowy_flutter/lib/plugins/database_view/application/field/field_controller.dart index ed77f9943d..f6b74624d0 100644 --- a/frontend/appflowy_flutter/lib/plugins/database_view/application/field/field_controller.dart +++ b/frontend/appflowy_flutter/lib/plugins/database_view/application/field/field_controller.dart @@ -790,6 +790,7 @@ class FieldInfo { case FieldType.RichText: case FieldType.Checkbox: case FieldType.Number: + case FieldType.DateTime: return true; default: return false; diff --git a/frontend/rust-lib/flowy-database2/src/services/field/type_options/checkbox_type_option/checkbox_type_option.rs b/frontend/rust-lib/flowy-database2/src/services/field/type_options/checkbox_type_option/checkbox_type_option.rs index 8c0c1603bf..35a482a653 100644 --- a/frontend/rust-lib/flowy-database2/src/services/field/type_options/checkbox_type_option/checkbox_type_option.rs +++ b/frontend/rust-lib/flowy-database2/src/services/field/type_options/checkbox_type_option/checkbox_type_option.rs @@ -143,4 +143,8 @@ impl TypeOptionCellDataCompare for CheckboxTypeOption { (false, false) => default_order(), } } + + fn exempt_from_cmp(&self, _: &::CellData) -> bool { + false + } } diff --git a/frontend/rust-lib/flowy-database2/src/services/field/type_options/checklist_type_option/checklist.rs b/frontend/rust-lib/flowy-database2/src/services/field/type_options/checklist_type_option/checklist.rs index b99ff9c323..0b2f919380 100644 --- a/frontend/rust-lib/flowy-database2/src/services/field/type_options/checklist_type_option/checklist.rs +++ b/frontend/rust-lib/flowy-database2/src/services/field/type_options/checklist_type_option/checklist.rs @@ -202,6 +202,10 @@ impl TypeOptionCellDataCompare for ChecklistTypeOption { Ordering::Equal } } + + fn exempt_from_cmp(&self, cell_data: &::CellData) -> bool { + cell_data.selected_option_ids.is_empty() + } } impl TypeOptionTransform for ChecklistTypeOption {} diff --git a/frontend/rust-lib/flowy-database2/src/services/field/type_options/date_type_option/date_type_option.rs b/frontend/rust-lib/flowy-database2/src/services/field/type_options/date_type_option/date_type_option.rs index a750b8033f..01e3c8f158 100644 --- a/frontend/rust-lib/flowy-database2/src/services/field/type_options/date_type_option/date_type_option.rs +++ b/frontend/rust-lib/flowy-database2/src/services/field/type_options/date_type_option/date_type_option.rs @@ -302,4 +302,8 @@ impl TypeOptionCellDataCompare for DateTypeOption { (None, None) => default_order(), } } + + fn exempt_from_cmp(&self, cell_data: &::CellData) -> bool { + cell_data.timestamp.is_none() + } } diff --git a/frontend/rust-lib/flowy-database2/src/services/field/type_options/number_type_option/number_type_option.rs b/frontend/rust-lib/flowy-database2/src/services/field/type_options/number_type_option/number_type_option.rs index 594ec0a62b..9407935487 100644 --- a/frontend/rust-lib/flowy-database2/src/services/field/type_options/number_type_option/number_type_option.rs +++ b/frontend/rust-lib/flowy-database2/src/services/field/type_options/number_type_option/number_type_option.rs @@ -260,6 +260,10 @@ impl TypeOptionCellDataCompare for NumberTypeOption { (Err(_), Err(_)) => Ordering::Equal, } } + + fn exempt_from_cmp(&self, cell_data: &::CellData) -> bool { + cell_data.0.is_empty() + } } impl std::default::Default for NumberTypeOption { fn default() -> Self { diff --git a/frontend/rust-lib/flowy-database2/src/services/field/type_options/selection_type_option/multi_select_type_option.rs b/frontend/rust-lib/flowy-database2/src/services/field/type_options/selection_type_option/multi_select_type_option.rs index a67f9b222c..797a108264 100644 --- a/frontend/rust-lib/flowy-database2/src/services/field/type_options/selection_type_option/multi_select_type_option.rs +++ b/frontend/rust-lib/flowy-database2/src/services/field/type_options/selection_type_option/multi_select_type_option.rs @@ -162,6 +162,10 @@ impl TypeOptionCellDataCompare for MultiSelectTypeOption { } default_order() } + + fn exempt_from_cmp(&self, cell_data: &::CellData) -> bool { + cell_data.is_empty() + } } #[cfg(test)] diff --git a/frontend/rust-lib/flowy-database2/src/services/field/type_options/selection_type_option/single_select_type_option.rs b/frontend/rust-lib/flowy-database2/src/services/field/type_options/selection_type_option/single_select_type_option.rs index 760e0c0f82..9b87d76ca2 100644 --- a/frontend/rust-lib/flowy-database2/src/services/field/type_options/selection_type_option/single_select_type_option.rs +++ b/frontend/rust-lib/flowy-database2/src/services/field/type_options/selection_type_option/single_select_type_option.rs @@ -146,6 +146,10 @@ impl TypeOptionCellDataCompare for SingleSelectTypeOption { (None, None) => default_order(), } } + + fn exempt_from_cmp(&self, cell_data: &::CellData) -> bool { + cell_data.is_empty() + } } #[cfg(test)] diff --git a/frontend/rust-lib/flowy-database2/src/services/field/type_options/text_type_option/text_type_option.rs b/frontend/rust-lib/flowy-database2/src/services/field/type_options/text_type_option/text_type_option.rs index 35dd038480..9f8dc5b8c3 100644 --- a/frontend/rust-lib/flowy-database2/src/services/field/type_options/text_type_option/text_type_option.rs +++ b/frontend/rust-lib/flowy-database2/src/services/field/type_options/text_type_option/text_type_option.rs @@ -155,6 +155,10 @@ impl TypeOptionCellDataCompare for RichTextTypeOption { ) -> Ordering { cell_data.0.cmp(&other_cell_data.0) } + + fn exempt_from_cmp(&self, cell_data: &::CellData) -> bool { + cell_data.0.trim().is_empty() + } } #[derive(Clone)] diff --git a/frontend/rust-lib/flowy-database2/src/services/field/type_options/type_option.rs b/frontend/rust-lib/flowy-database2/src/services/field/type_options/type_option.rs index 81abc469a6..823d1f0854 100644 --- a/frontend/rust-lib/flowy-database2/src/services/field/type_options/type_option.rs +++ b/frontend/rust-lib/flowy-database2/src/services/field/type_options/type_option.rs @@ -132,6 +132,8 @@ pub trait TypeOptionCellDataCompare: TypeOption { cell_data: &::CellData, other_cell_data: &::CellData, ) -> Ordering; + + fn exempt_from_cmp(&self, cell_data: &::CellData) -> bool; } pub fn type_option_data_from_pb_or_default>( diff --git a/frontend/rust-lib/flowy-database2/src/services/field/type_options/type_option_cell.rs b/frontend/rust-lib/flowy-database2/src/services/field/type_options/type_option_cell.rs index 97eb2459a3..c668befec4 100644 --- a/frontend/rust-lib/flowy-database2/src/services/field/type_options/type_option_cell.rs +++ b/frontend/rust-lib/flowy-database2/src/services/field/type_options/type_option_cell.rs @@ -19,6 +19,7 @@ use crate::services::field::{ SingleSelectTypeOption, TypeOption, TypeOptionCellData, TypeOptionCellDataCompare, TypeOptionCellDataFilter, TypeOptionTransform, URLTypeOption, }; +use crate::services::sort::SortCondition; pub const CELL_DATA: &str = "data"; @@ -45,7 +46,13 @@ pub trait TypeOptionCellDataHandler: Send + Sync + 'static { field: &Field, ) -> FlowyResult; - fn handle_cell_compare(&self, left_cell: &Cell, right_cell: &Cell, field: &Field) -> Ordering; + fn handle_cell_compare( + &self, + left_cell: &Cell, + right_cell: &Cell, + field: &Field, + sort_condition: SortCondition, + ) -> Ordering; fn handle_cell_filter(&self, field_type: &FieldType, field: &Field, cell: &Cell) -> bool; @@ -234,7 +241,13 @@ where Ok(cell) } - fn handle_cell_compare(&self, left_cell: &Cell, right_cell: &Cell, field: &Field) -> Ordering { + fn handle_cell_compare( + &self, + left_cell: &Cell, + right_cell: &Cell, + field: &Field, + sort_condition: SortCondition, + ) -> Ordering { let field_type = FieldType::from(field.field_type); let left = self .get_decoded_cell_data(left_cell, &field_type, field) @@ -242,7 +255,20 @@ where let right = self .get_decoded_cell_data(right_cell, &field_type, field) .unwrap_or_default(); - self.apply_cmp(&left, &right) + + match (self.exempt_from_cmp(&left), self.exempt_from_cmp(&right)) { + (true, true) => Ordering::Equal, + (true, false) => Ordering::Greater, + (false, true) => Ordering::Less, + (false, false) => { + let order = self.apply_cmp(&left, &right); + // The order is calculated by Ascending. So reverse the order if the SortCondition is descending. + match sort_condition { + SortCondition::Ascending => order, + SortCondition::Descending => order.reverse(), + } + }, + } } fn handle_cell_filter(&self, field_type: &FieldType, field: &Field, cell: &Cell) -> bool { diff --git a/frontend/rust-lib/flowy-database2/src/services/field/type_options/url_type_option/url_type_option.rs b/frontend/rust-lib/flowy-database2/src/services/field/type_options/url_type_option/url_type_option.rs index b665a666c1..7880bb0e82 100644 --- a/frontend/rust-lib/flowy-database2/src/services/field/type_options/url_type_option/url_type_option.rs +++ b/frontend/rust-lib/flowy-database2/src/services/field/type_options/url_type_option/url_type_option.rs @@ -126,7 +126,12 @@ impl TypeOptionCellDataCompare for URLTypeOption { ) -> Ordering { cell_data.data.cmp(&other_cell_data.data) } + + fn exempt_from_cmp(&self, cell_data: &::CellData) -> bool { + cell_data.data.is_empty() + } } + fn auto_append_scheme(s: &str) -> String { // Only support https scheme by now match url::Url::parse(s) { diff --git a/frontend/rust-lib/flowy-database2/src/services/sort/controller.rs b/frontend/rust-lib/flowy-database2/src/services/sort/controller.rs index 12b78c6bee..77aa1df472 100644 --- a/frontend/rust-lib/flowy-database2/src/services/sort/controller.rs +++ b/frontend/rust-lib/flowy-database2/src/services/sort/controller.rs @@ -156,7 +156,7 @@ impl SortController { } let fields = self.delegate.get_fields(&self.view_id, None).await; - for sort in self.sorts.iter() { + for sort in self.sorts.iter().rev() { rows .par_sort_by(|left, right| cmp_row(&left.row, &right.row, sort, &fields, &self.cell_cache)); } @@ -240,35 +240,46 @@ fn cmp_row( fields: &[Arc], cell_data_cache: &CellCache, ) -> Ordering { - let order = match ( - left.cells.get(&sort.field_id), - right.cells.get(&sort.field_id), - ) { - (Some(left_cell), Some(right_cell)) => { - let field_type = sort.field_type.clone(); - match fields - .iter() - .find(|field_rev| field_rev.id == sort.field_id) - { - None => default_order(), - Some(field_rev) => cmp_cell( - left_cell, - right_cell, - field_rev, - field_type, - cell_data_cache, - ), - } + let field_type = sort.field_type.clone(); + match fields + .iter() + .find(|field_rev| field_rev.id == sort.field_id) + { + None => default_order(), + Some(field_rev) => match ( + left.cells.get(&sort.field_id), + right.cells.get(&sort.field_id), + ) { + (Some(left_cell), Some(right_cell)) => cmp_cell( + left_cell, + right_cell, + field_rev, + field_type, + cell_data_cache, + sort.condition, + ), + (Some(_), None) => { + if field_type.is_checkbox() { + match sort.condition { + SortCondition::Ascending => Ordering::Greater, + SortCondition::Descending => Ordering::Less, + } + } else { + Ordering::Less + } + }, + (None, Some(_)) => { + if field_type.is_checkbox() { + match sort.condition { + SortCondition::Ascending => Ordering::Less, + SortCondition::Descending => Ordering::Greater, + } + } else { + Ordering::Greater + } + }, + _ => default_order(), }, - (Some(_), None) => Ordering::Greater, - (None, Some(_)) => Ordering::Less, - _ => default_order(), - }; - - // The order is calculated by Ascending. So reverse the order if the SortCondition is descending. - match sort.condition { - SortCondition::Ascending => order, - SortCondition::Descending => order.reverse(), } } @@ -278,6 +289,7 @@ fn cmp_cell( field: &Arc, field_type: FieldType, cell_data_cache: &CellCache, + sort_condition: SortCondition, ) -> Ordering { match TypeOptionCellExt::new_with_cell_data_cache(field.as_ref(), Some(cell_data_cache.clone())) .get_type_option_cell_data_handler(&field_type) @@ -285,7 +297,8 @@ fn cmp_cell( None => default_order(), Some(handler) => { let cal_order = || { - let order = handler.handle_cell_compare(left_cell, right_cell, field.as_ref()); + let order = + handler.handle_cell_compare(left_cell, right_cell, field.as_ref(), sort_condition); Option::::Some(order) }; diff --git a/frontend/rust-lib/flowy-database2/tests/database/sort_test/checkbox_and_text_test.rs b/frontend/rust-lib/flowy-database2/tests/database/sort_test/checkbox_and_text_test.rs index 9a2294792a..c68d44e6b2 100644 --- a/frontend/rust-lib/flowy-database2/tests/database/sort_test/checkbox_and_text_test.rs +++ b/frontend/rust-lib/flowy-database2/tests/database/sort_test/checkbox_and_text_test.rs @@ -18,7 +18,7 @@ async fn sort_checkbox_and_then_text_by_descending_test() { field_id: text_field.id.clone(), orders: vec!["A", "", "C", "DA", "AE", "AE"], }, - // // Insert checkbox sort + // Insert checkbox sort InsertSort { field: checkbox_field.clone(), condition: SortCondition::Descending, @@ -31,10 +31,7 @@ async fn sort_checkbox_and_then_text_by_descending_test() { field_id: text_field.id.clone(), orders: vec!["A", "", "AE", "C", "DA", "AE"], }, - // Insert text sort. After inserting the text sort, the order of the rows - // will be changed. - // before: ["A", "", "AE", "C", "DA", "AE"] - // after: ["", "A", "AE", "AE", "C", "DA"] + // Insert text sort InsertSort { field: text_field.clone(), condition: SortCondition::Ascending, @@ -45,7 +42,7 @@ async fn sort_checkbox_and_then_text_by_descending_test() { }, AssertCellContentOrder { field_id: text_field.id.clone(), - orders: vec!["", "A", "AE", "AE", "C", "DA"], + orders: vec!["A", "AE", "", "AE", "C", "DA"], }, ]; test.run_scripts(scripts).await; diff --git a/frontend/rust-lib/flowy-database2/tests/database/sort_test/multi_sort_test.rs b/frontend/rust-lib/flowy-database2/tests/database/sort_test/multi_sort_test.rs index 8977aa5829..3451b112ea 100644 --- a/frontend/rust-lib/flowy-database2/tests/database/sort_test/multi_sort_test.rs +++ b/frontend/rust-lib/flowy-database2/tests/database/sort_test/multi_sort_test.rs @@ -16,7 +16,7 @@ async fn sort_text_with_checkbox_by_ascending_test() { }, AssertCellContentOrder { field_id: checkbox_field.id.clone(), - orders: vec!["Yes", "Yes", "No", "No", "No"], + orders: vec!["Yes", "Yes", "No", "No", "No", "Yes"], }, InsertSort { field: text_field.clone(), @@ -24,7 +24,11 @@ async fn sort_text_with_checkbox_by_ascending_test() { }, AssertCellContentOrder { field_id: text_field.id.clone(), - orders: vec!["", "A", "AE", "AE", "C", "DA"], + orders: vec!["A", "AE", "AE", "C", "DA", ""], + }, + AssertCellContentOrder { + field_id: checkbox_field.id.clone(), + orders: vec!["Yes", "No", "Yes", "No", "No", "Yes"], }, ]; test.run_scripts(scripts).await; @@ -36,11 +40,11 @@ async fn sort_text_with_checkbox_by_ascending_test() { }, AssertCellContentOrder { field_id: text_field.id.clone(), - orders: vec!["", "A", "AE", "AE", "C", "DA"], + orders: vec!["A", "AE", "AE", "C", "DA", ""], }, AssertCellContentOrder { field_id: checkbox_field.id.clone(), - orders: vec!["Yes", "Yes", "Yes", "No", "No"], + orders: vec!["Yes", "Yes", "No", "No", "No", "Yes"], }, ]; test.run_scripts(scripts).await; diff --git a/frontend/rust-lib/flowy-database2/tests/database/sort_test/single_sort_test.rs b/frontend/rust-lib/flowy-database2/tests/database/sort_test/single_sort_test.rs index 09022e8169..39d1f0cf4b 100644 --- a/frontend/rust-lib/flowy-database2/tests/database/sort_test/single_sort_test.rs +++ b/frontend/rust-lib/flowy-database2/tests/database/sort_test/single_sort_test.rs @@ -18,7 +18,7 @@ async fn sort_text_by_ascending_test() { }, AssertCellContentOrder { field_id: text_field.id.clone(), - orders: vec!["", "A", "AE", "AE", "C", "DA"], + orders: vec!["A", "AE", "AE", "C", "DA", ""], }, ]; test.run_scripts(scripts).await; @@ -35,7 +35,7 @@ async fn sort_change_notification_by_update_text_test() { }, AssertCellContentOrder { field_id: text_field.id.clone(), - orders: vec!["", "A", "AE", "AE", "C", "DA"], + orders: vec!["A", "AE", "AE", "C", "DA", ""], }, // Wait the insert task to finish. The cost of time should be less than 200 milliseconds. Wait { millis: 200 }, @@ -45,12 +45,12 @@ async fn sort_change_notification_by_update_text_test() { let row_details = test.get_rows().await; let scripts = vec![ UpdateTextCell { - row_id: row_details[2].row.id.clone(), + row_id: row_details[1].row.id.clone(), text: "E".to_string(), }, AssertSortChanged { - old_row_orders: vec!["", "A", "E", "AE", "C", "DA"], - new_row_orders: vec!["", "A", "AE", "C", "DA", "E"], + old_row_orders: vec!["A", "E", "AE", "C", "DA", ""], + new_row_orders: vec!["A", "AE", "C", "DA", "E", ""], }, ]; test.run_scripts(scripts).await; @@ -263,7 +263,7 @@ async fn sort_multi_select_by_ascending_test() { }, AssertCellContentOrder { field_id: multi_select.id.clone(), - orders: vec!["", "", "", "Facebook", "Google,Facebook", "Google,Twitter"], + orders: vec!["Facebook", "Google,Facebook", "Google,Twitter", "", "", ""], }, ]; test.run_scripts(scripts).await;