From c500cd12878ce09eb7d83bf50fd2f363ea820193 Mon Sep 17 00:00:00 2001 From: Richard Shiue <71320345+richardshiue@users.noreply.github.com> Date: Thu, 22 Feb 2024 07:09:12 +0800 Subject: [PATCH] refactor: improve type option filter logic (#4652) --- .../entities/filter_entities/date_filter.rs | 3 +- .../entities/filter_entities/number_filter.rs | 3 +- .../date_type_option/date_filter.rs | 183 ++++++++++++------ .../date_type_option/date_type_option.rs | 6 +- .../number_type_option/number_filter.rs | 90 ++++++--- .../number_type_option/number_type_option.rs | 7 +- .../timestamp_type_option.rs | 7 +- 7 files changed, 189 insertions(+), 110 deletions(-) diff --git a/frontend/rust-lib/flowy-database2/src/entities/filter_entities/date_filter.rs b/frontend/rust-lib/flowy-database2/src/entities/filter_entities/date_filter.rs index 3c94efd701..75891cea6f 100644 --- a/frontend/rust-lib/flowy-database2/src/entities/filter_entities/date_filter.rs +++ b/frontend/rust-lib/flowy-database2/src/entities/filter_entities/date_filter.rs @@ -43,9 +43,8 @@ impl FromStr for DateFilterContentPB { } } -#[derive(Debug, Clone, PartialEq, Eq, ProtoBuf_Enum)] +#[derive(Debug, Clone, Default, PartialEq, Eq, ProtoBuf_Enum)] #[repr(u8)] -#[derive(Default)] pub enum DateFilterConditionPB { #[default] DateIs = 0, diff --git a/frontend/rust-lib/flowy-database2/src/entities/filter_entities/number_filter.rs b/frontend/rust-lib/flowy-database2/src/entities/filter_entities/number_filter.rs index d51d0e4726..17f43c8640 100644 --- a/frontend/rust-lib/flowy-database2/src/entities/filter_entities/number_filter.rs +++ b/frontend/rust-lib/flowy-database2/src/entities/filter_entities/number_filter.rs @@ -12,9 +12,8 @@ pub struct NumberFilterPB { pub content: String, } -#[derive(Debug, Clone, PartialEq, Eq, ProtoBuf_Enum)] +#[derive(Debug, Clone, Default, PartialEq, Eq, ProtoBuf_Enum)] #[repr(u8)] -#[derive(Default)] pub enum NumberFilterConditionPB { #[default] Equal = 0, diff --git a/frontend/rust-lib/flowy-database2/src/services/field/type_options/date_type_option/date_filter.rs b/frontend/rust-lib/flowy-database2/src/services/field/type_options/date_type_option/date_filter.rs index 35f30b388e..1eed418c86 100644 --- a/frontend/rust-lib/flowy-database2/src/services/field/type_options/date_type_option/date_filter.rs +++ b/frontend/rust-lib/flowy-database2/src/services/field/type_options/date_type_option/date_filter.rs @@ -1,65 +1,108 @@ use crate::entities::{DateFilterConditionPB, DateFilterPB}; -use chrono::NaiveDateTime; + +use chrono::{NaiveDate, NaiveDateTime}; + +use super::DateCellData; impl DateFilterPB { - pub fn is_visible>>(&self, cell_timestamp: T) -> bool { - match cell_timestamp.into() { - None => DateFilterConditionPB::DateIsEmpty == self.condition, - Some(timestamp) => { - match self.condition { - DateFilterConditionPB::DateIsNotEmpty => { - return true; - }, - DateFilterConditionPB::DateIsEmpty => { - return false; - }, - _ => {}, - } - - let cell_time = NaiveDateTime::from_timestamp_opt(timestamp, 0); - let cell_date = cell_time.map(|time| time.date()); - match self.timestamp { - None => { - if self.start.is_none() { - return true; - } - - if self.end.is_none() { - return true; - } - - let start_time = NaiveDateTime::from_timestamp_opt(*self.start.as_ref().unwrap(), 0); - let start_date = start_time.map(|time| time.date()); - - let end_time = NaiveDateTime::from_timestamp_opt(*self.end.as_ref().unwrap(), 0); - let end_date = end_time.map(|time| time.date()); - - cell_date >= start_date && cell_date <= end_date - }, - Some(timestamp) => { - let expected_timestamp = NaiveDateTime::from_timestamp_opt(timestamp, 0); - let expected_date = expected_timestamp.map(|time| time.date()); - - // We assume that the cell_timestamp doesn't contain hours, just day. - match self.condition { - DateFilterConditionPB::DateIs => cell_date == expected_date, - DateFilterConditionPB::DateBefore => cell_date < expected_date, - DateFilterConditionPB::DateAfter => cell_date > expected_date, - DateFilterConditionPB::DateOnOrBefore => cell_date <= expected_date, - DateFilterConditionPB::DateOnOrAfter => cell_date >= expected_date, - _ => true, - } - }, - } + /// Returns `None` if the DateFilterPB doesn't have the necessary data for + /// the condition. For example, `start` and `end` timestamps for + /// `DateFilterConditionPB::DateWithin`. + pub fn is_visible(&self, cell_data: &DateCellData) -> Option { + let strategy = match self.condition { + DateFilterConditionPB::DateIs => DateFilterStrategy::On(self.timestamp?), + DateFilterConditionPB::DateBefore => DateFilterStrategy::Before(self.timestamp?), + DateFilterConditionPB::DateAfter => DateFilterStrategy::After(self.timestamp?), + DateFilterConditionPB::DateOnOrBefore => DateFilterStrategy::OnOrBefore(self.timestamp?), + DateFilterConditionPB::DateOnOrAfter => DateFilterStrategy::OnOrAfter(self.timestamp?), + DateFilterConditionPB::DateWithIn => DateFilterStrategy::DateWithin { + start: self.start?, + end: self.end?, }, + DateFilterConditionPB::DateIsEmpty => DateFilterStrategy::Empty, + DateFilterConditionPB::DateIsNotEmpty => DateFilterStrategy::NotEmpty, + }; + + Some(strategy.filter(cell_data)) + } +} + +#[inline] +fn naive_date_from_timestamp(timestamp: i64) -> Option { + NaiveDateTime::from_timestamp_opt(timestamp, 0).map(|date_time: NaiveDateTime| date_time.date()) +} + +enum DateFilterStrategy { + On(i64), + Before(i64), + After(i64), + OnOrBefore(i64), + OnOrAfter(i64), + DateWithin { start: i64, end: i64 }, + Empty, + NotEmpty, +} + +impl DateFilterStrategy { + fn filter(self, cell_data: &DateCellData) -> bool { + match self { + DateFilterStrategy::On(expected_timestamp) => cell_data.timestamp.is_some_and(|timestamp| { + let cell_date = naive_date_from_timestamp(timestamp); + let expected_date = naive_date_from_timestamp(expected_timestamp); + cell_date == expected_date + }), + DateFilterStrategy::Before(expected_timestamp) => { + cell_data.timestamp.is_some_and(|timestamp| { + let cell_date = naive_date_from_timestamp(timestamp); + let expected_date = naive_date_from_timestamp(expected_timestamp); + cell_date < expected_date + }) + }, + DateFilterStrategy::After(expected_timestamp) => { + cell_data.timestamp.is_some_and(|timestamp| { + let cell_date = naive_date_from_timestamp(timestamp); + let expected_date = naive_date_from_timestamp(expected_timestamp); + cell_date > expected_date + }) + }, + DateFilterStrategy::OnOrBefore(expected_timestamp) => { + cell_data.timestamp.is_some_and(|timestamp| { + let cell_date = naive_date_from_timestamp(timestamp); + let expected_date = naive_date_from_timestamp(expected_timestamp); + cell_date <= expected_date + }) + }, + DateFilterStrategy::OnOrAfter(expected_timestamp) => { + cell_data.timestamp.is_some_and(|timestamp| { + let cell_date = naive_date_from_timestamp(timestamp); + let expected_date = naive_date_from_timestamp(expected_timestamp); + cell_date >= expected_date + }) + }, + DateFilterStrategy::DateWithin { start, end } => { + cell_data.timestamp.is_some_and(|timestamp| { + let cell_date = naive_date_from_timestamp(timestamp); + let expected_start_date = naive_date_from_timestamp(start); + let expected_end_date = naive_date_from_timestamp(end); + cell_date >= expected_start_date && cell_date <= expected_end_date + }) + }, + DateFilterStrategy::Empty => { + cell_data.timestamp.is_none() && cell_data.end_timestamp.is_none() + }, + DateFilterStrategy::NotEmpty => cell_data.timestamp.is_some(), } } } #[cfg(test)] mod tests { - #![allow(clippy::all)] use crate::entities::{DateFilterConditionPB, DateFilterPB}; + use crate::services::field::DateCellData; + + fn to_cell_data(timestamp: i32) -> DateCellData { + DateCellData::new(timestamp as i64, false, false, "".to_string()) + } #[test] fn date_filter_is_test() { @@ -70,10 +113,11 @@ mod tests { start: None, }; - for (val, visible) in vec![(1668387885, true), (1647251762, false)] { - assert_eq!(filter.is_visible(val as i64), visible); + for (val, visible) in [(1668387885, true), (1647251762, false)] { + assert_eq!(filter.is_visible(&to_cell_data(val)).unwrap(), visible); } } + #[test] fn date_filter_before_test() { let filter = DateFilterPB { @@ -83,8 +127,13 @@ mod tests { end: None, }; - for (val, visible, msg) in vec![(1668387884, false, "1"), (1647251762, true, "2")] { - assert_eq!(filter.is_visible(val as i64), visible, "{}", msg); + for (val, visible, msg) in [(1668387884, false, "1"), (1647251762, true, "2")] { + assert_eq!( + filter.is_visible(&to_cell_data(val)).unwrap(), + visible, + "{}", + msg + ); } } @@ -97,8 +146,8 @@ mod tests { end: None, }; - for (val, visible) in vec![(1668387884, true), (1668387885, true)] { - assert_eq!(filter.is_visible(val as i64), visible); + for (val, visible) in [(1668387884, true), (1668387885, true)] { + assert_eq!(filter.is_visible(&to_cell_data(val)).unwrap(), visible); } } #[test] @@ -110,8 +159,8 @@ mod tests { end: None, }; - for (val, visible) in vec![(1668387888, false), (1668531885, true), (0, false)] { - assert_eq!(filter.is_visible(val as i64), visible); + for (val, visible) in [(1668387888, false), (1668531885, true), (0, false)] { + assert_eq!(filter.is_visible(&to_cell_data(val)).unwrap(), visible); } } @@ -124,12 +173,12 @@ mod tests { timestamp: None, }; - for (val, visible, _msg) in vec![ + for (val, visible, _msg) in [ (1668272685, true, "11/13"), (1668359085, true, "11/14"), (1668704685, false, "11/18"), ] { - assert_eq!(filter.is_visible(val as i64), visible); + assert_eq!(filter.is_visible(&to_cell_data(val)).unwrap(), visible); } } @@ -142,8 +191,16 @@ mod tests { timestamp: None, }; - for (val, visible) in vec![(None, true), (Some(123), false)] { - assert_eq!(filter.is_visible(val), visible); + for (val, visible) in [(None, true), (Some(123), false)] { + assert_eq!( + filter + .is_visible(&DateCellData { + timestamp: val, + ..Default::default() + }) + .unwrap(), + visible + ); } } } 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 e80348c41d..34badaa58f 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 @@ -19,9 +19,6 @@ use crate::services::field::{ }; use crate::services::sort::SortCondition; -/// The [DateTypeOption] is used by [FieldType::Date], [FieldType::LastEditedTime], and [FieldType::CreatedTime]. -/// So, storing the field type is necessary to distinguish the field type. -/// Most of the cases, each [FieldType] has its own [TypeOption] implementation. #[derive(Clone, Debug, Serialize, Deserialize, Default)] pub struct DateTypeOption { pub date_format: DateFormat, @@ -351,8 +348,7 @@ impl TypeOptionCellDataFilter for DateTypeOption { if !field_type.is_date() { return true; } - - filter.is_visible(cell_data.timestamp) + filter.is_visible(cell_data).unwrap_or(true) } } diff --git a/frontend/rust-lib/flowy-database2/src/services/field/type_options/number_type_option/number_filter.rs b/frontend/rust-lib/flowy-database2/src/services/field/type_options/number_type_option/number_filter.rs index db6dc71a83..9832d05009 100644 --- a/frontend/rust-lib/flowy-database2/src/services/field/type_options/number_type_option/number_filter.rs +++ b/frontend/rust-lib/flowy-database2/src/services/field/type_options/number_type_option/number_filter.rs @@ -5,32 +5,62 @@ use rust_decimal::Decimal; use std::str::FromStr; impl NumberFilterPB { - pub fn is_visible(&self, num_cell_data: &NumberCellFormat) -> bool { - if self.content.is_empty() { - match self.condition { - NumberFilterConditionPB::NumberIsEmpty => { - return num_cell_data.is_empty(); - }, - NumberFilterConditionPB::NumberIsNotEmpty => { - return !num_cell_data.is_empty(); - }, - _ => {}, - } - } - match num_cell_data.decimal().as_ref() { - None => false, - Some(cell_decimal) => { - let decimal = Decimal::from_str(&self.content).unwrap_or_else(|_| Decimal::zero()); - match self.condition { - NumberFilterConditionPB::Equal => cell_decimal == &decimal, - NumberFilterConditionPB::NotEqual => cell_decimal != &decimal, - NumberFilterConditionPB::GreaterThan => cell_decimal > &decimal, - NumberFilterConditionPB::LessThan => cell_decimal < &decimal, - NumberFilterConditionPB::GreaterThanOrEqualTo => cell_decimal >= &decimal, - NumberFilterConditionPB::LessThanOrEqualTo => cell_decimal <= &decimal, - _ => true, - } + pub fn is_visible(&self, cell_data: &NumberCellFormat) -> Option { + let expected_decimal = Decimal::from_str(&self.content).unwrap_or_else(|_| Decimal::zero()); + + let strategy = match self.condition { + NumberFilterConditionPB::Equal => NumberFilterStrategy::Equal(expected_decimal), + NumberFilterConditionPB::NotEqual => NumberFilterStrategy::NotEqual(expected_decimal), + NumberFilterConditionPB::GreaterThan => NumberFilterStrategy::GreaterThan(expected_decimal), + NumberFilterConditionPB::LessThan => NumberFilterStrategy::LessThan(expected_decimal), + NumberFilterConditionPB::GreaterThanOrEqualTo => { + NumberFilterStrategy::GreaterThanOrEqualTo(expected_decimal) }, + NumberFilterConditionPB::LessThanOrEqualTo => { + NumberFilterStrategy::LessThanOrEqualTo(expected_decimal) + }, + NumberFilterConditionPB::NumberIsEmpty => NumberFilterStrategy::Empty, + NumberFilterConditionPB::NumberIsNotEmpty => NumberFilterStrategy::NotEmpty, + }; + + Some(strategy.filter(cell_data)) + } +} + +enum NumberFilterStrategy { + Equal(Decimal), + NotEqual(Decimal), + GreaterThan(Decimal), + LessThan(Decimal), + GreaterThanOrEqualTo(Decimal), + LessThanOrEqualTo(Decimal), + Empty, + NotEmpty, +} + +impl NumberFilterStrategy { + fn filter(self, cell_data: &NumberCellFormat) -> bool { + match self { + NumberFilterStrategy::Equal(expected_value) => cell_data + .decimal() + .is_some_and(|decimal| decimal == expected_value), + NumberFilterStrategy::NotEqual(expected_value) => cell_data + .decimal() + .is_some_and(|decimal| decimal != expected_value), + NumberFilterStrategy::GreaterThan(expected_value) => cell_data + .decimal() + .is_some_and(|decimal| decimal > expected_value), + NumberFilterStrategy::LessThan(expected_value) => cell_data + .decimal() + .is_some_and(|decimal| decimal < expected_value), + NumberFilterStrategy::GreaterThanOrEqualTo(expected_value) => cell_data + .decimal() + .is_some_and(|decimal| decimal >= expected_value), + NumberFilterStrategy::LessThanOrEqualTo(expected_value) => cell_data + .decimal() + .is_some_and(|decimal| decimal <= expected_value), + NumberFilterStrategy::Empty => cell_data.is_empty(), + NumberFilterStrategy::NotEmpty => !cell_data.is_empty(), } } } @@ -39,6 +69,7 @@ impl NumberFilterPB { mod tests { use crate::entities::{NumberFilterConditionPB, NumberFilterPB}; use crate::services::field::{NumberCellFormat, NumberFormat}; + #[test] fn number_filter_equal_test() { let number_filter = NumberFilterPB { @@ -48,15 +79,16 @@ mod tests { for (num_str, visible) in [("123", true), ("1234", false), ("", false)] { let data = NumberCellFormat::from_format_str(num_str, &NumberFormat::Num).unwrap_or_default(); - assert_eq!(number_filter.is_visible(&data), visible); + assert_eq!(number_filter.is_visible(&data), Some(visible)); } let format = NumberFormat::USD; for (num_str, visible) in [("$123", true), ("1234", false), ("", false)] { let data = NumberCellFormat::from_format_str(num_str, &format).unwrap(); - assert_eq!(number_filter.is_visible(&data), visible); + assert_eq!(number_filter.is_visible(&data), Some(visible)); } } + #[test] fn number_filter_greater_than_test() { let number_filter = NumberFilterPB { @@ -65,7 +97,7 @@ mod tests { }; for (num_str, visible) in [("123", true), ("10", false), ("30", true), ("", false)] { let data = NumberCellFormat::from_format_str(num_str, &NumberFormat::Num).unwrap_or_default(); - assert_eq!(number_filter.is_visible(&data), visible); + assert_eq!(number_filter.is_visible(&data), Some(visible)); } } @@ -77,7 +109,7 @@ mod tests { }; for (num_str, visible) in [("12", true), ("1234", false), ("30", true), ("", false)] { let data = NumberCellFormat::from_format_str(num_str, &NumberFormat::Num).unwrap_or_default(); - assert_eq!(number_filter.is_visible(&data), visible); + assert_eq!(number_filter.is_visible(&data), Some(visible)); } } } 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 2241a4c3bb..94da6b0c0b 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 @@ -120,10 +120,7 @@ impl NumberTypeOption { Self::default() } - pub(crate) fn format_cell_data( - &self, - num_cell_data: &NumberCellData, - ) -> FlowyResult { + fn format_cell_data(&self, num_cell_data: &NumberCellData) -> FlowyResult { match self.format { NumberFormat::Num => { if SCIENTIFIC_NOTATION_REGEX @@ -252,7 +249,7 @@ impl TypeOptionCellDataFilter for NumberTypeOption { return true; } match self.format_cell_data(cell_data) { - Ok(cell_data) => filter.is_visible(&cell_data), + Ok(cell_data) => filter.is_visible(&cell_data).unwrap_or(true), Err(_) => true, } } diff --git a/frontend/rust-lib/flowy-database2/src/services/field/type_options/timestamp_type_option/timestamp_type_option.rs b/frontend/rust-lib/flowy-database2/src/services/field/type_options/timestamp_type_option/timestamp_type_option.rs index ada47d3864..a5b78a6d89 100644 --- a/frontend/rust-lib/flowy-database2/src/services/field/type_options/timestamp_type_option/timestamp_type_option.rs +++ b/frontend/rust-lib/flowy-database2/src/services/field/type_options/timestamp_type_option/timestamp_type_option.rs @@ -174,15 +174,14 @@ impl CellDataChangeset for TimestampTypeOption { impl TypeOptionCellDataFilter for TimestampTypeOption { fn apply_filter( &self, - filter: &::CellFilter, + _filter: &::CellFilter, field_type: &FieldType, - cell_data: &::CellData, + _cell_data: &::CellData, ) -> bool { if !field_type.is_last_edited_time() && !field_type.is_created_time() { return true; } - - filter.is_visible(cell_data.timestamp) + false } }