From e9f8796809eb2e9fa9596d7f83135dca1fb8c87e Mon Sep 17 00:00:00 2001 From: "Nathan.fooo" <86001920+appflowy@users.noreply.github.com> Date: Thu, 5 Jan 2023 20:29:19 +0800 Subject: [PATCH] fix: update the cell content if input is not valid data (#1652) Co-authored-by: nathan --- .../cell/cell_service/cell_controller.dart | 59 +++++++++++++++---- .../grid/application/cell/date_cal_bloc.dart | 2 +- .../application/cell/number_cell_bloc.dart | 58 ++++++++++-------- .../widgets/cell/number_cell.dart | 16 ++--- .../number_type_option_entities.rs | 5 +- .../field/type_options/type_option_cell.rs | 15 +++-- .../flowy-user/src/services/database.rs | 1 - 7 files changed, 98 insertions(+), 58 deletions(-) diff --git a/frontend/app_flowy/lib/plugins/grid/application/cell/cell_service/cell_controller.dart b/frontend/app_flowy/lib/plugins/grid/application/cell/cell_service/cell_controller.dart index a947ec327f..bb2dcc59d7 100644 --- a/frontend/app_flowy/lib/plugins/grid/application/cell/cell_service/cell_controller.dart +++ b/frontend/app_flowy/lib/plugins/grid/application/cell/cell_service/cell_controller.dart @@ -133,7 +133,7 @@ class IGridCellController extends Equatable { final IGridCellDataPersistence _cellDataPersistence; CellListener? _cellListener; - ValueNotifier? _cellDataNotifier; + CellDataNotifier? _cellDataNotifier; bool isListening = false; VoidCallback? _onFieldChangedFn; @@ -170,8 +170,20 @@ class IGridCellController extends Equatable { FieldType get fieldType => cellId.fieldInfo.fieldType; + /// Listen on the cell content or field changes + /// + /// An optional [listenWhenOnCellChanged] can be implemented for more + /// granular control over when [listener] is called. + /// [listenWhenOnCellChanged] will be invoked on each [onCellChanged] + /// get called. + /// [listenWhenOnCellChanged] takes the previous `value` and current + /// `value` and must return a [bool] which determines whether or not + /// the [onCellChanged] function will be invoked. + /// [onCellChanged] is optional and if omitted, it will default to `true`. + /// VoidCallback? startListening({ required void Function(T?) onCellChanged, + bool Function(T? oldValue, T? newValue)? listenWhenOnCellChanged, VoidCallback? onCellFieldChanged, }) { if (isListening) { @@ -180,7 +192,10 @@ class IGridCellController extends Equatable { } isListening = true; - _cellDataNotifier = ValueNotifier(_cellsCache.get(_cacheKey)); + _cellDataNotifier = CellDataNotifier( + value: _cellsCache.get(_cacheKey), + listenWhen: listenWhenOnCellChanged, + ); _cellListener = CellListener(rowId: cellId.rowId, fieldId: cellId.fieldInfo.id); @@ -255,24 +270,21 @@ class IGridCellController extends Equatable { /// You can set [deduplicate] to true (default is false) to reduce the save operation. /// It's useful when you call this method when user editing the [TextField]. /// The default debounce interval is 300 milliseconds. - void saveCellData(D data, - {bool deduplicate = false, - void Function(Option)? resultCallback}) async { + void saveCellData( + D data, { + bool deduplicate = false, + void Function(Option)? onFinish, + }) async { + _loadDataOperation?.cancel(); if (deduplicate) { - _loadDataOperation?.cancel(); - _saveDataOperation?.cancel(); _saveDataOperation = Timer(const Duration(milliseconds: 300), () async { final result = await _cellDataPersistence.save(data); - if (resultCallback != null) { - resultCallback(result); - } + onFinish?.call(result); }); } else { final result = await _cellDataPersistence.save(data); - if (resultCallback != null) { - resultCallback(result); - } + onFinish?.call(result); } } @@ -302,6 +314,7 @@ class IGridCellController extends Equatable { await _cellListener?.stop(); _loadDataOperation?.cancel(); _saveDataOperation?.cancel(); + _cellDataNotifier?.dispose(); _cellDataNotifier = null; if (_onFieldChangedFn != null) { @@ -343,3 +356,23 @@ class GridCellFieldNotifierImpl extends IGridCellFieldNotifier { ); } } + +class CellDataNotifier extends ChangeNotifier { + T _value; + bool Function(T? oldValue, T? newValue)? listenWhen; + CellDataNotifier({required T value, this.listenWhen}) : _value = value; + + set value(T newValue) { + if (listenWhen?.call(_value, newValue) ?? false) { + _value = newValue; + notifyListeners(); + } else { + if (_value != newValue) { + _value = newValue; + notifyListeners(); + } + } + } + + T get value => _value; +} diff --git a/frontend/app_flowy/lib/plugins/grid/application/cell/date_cal_bloc.dart b/frontend/app_flowy/lib/plugins/grid/application/cell/date_cal_bloc.dart index 82514da3d5..1d4cacb8dd 100644 --- a/frontend/app_flowy/lib/plugins/grid/application/cell/date_cal_bloc.dart +++ b/frontend/app_flowy/lib/plugins/grid/application/cell/date_cal_bloc.dart @@ -102,7 +102,7 @@ class DateCalBloc extends Bloc { } } - cellController.saveCellData(newCalData, resultCallback: (result) { + cellController.saveCellData(newCalData, onFinish: (result) { result.fold( () => updateCalData(Some(newCalData), none()), (err) { diff --git a/frontend/app_flowy/lib/plugins/grid/application/cell/number_cell_bloc.dart b/frontend/app_flowy/lib/plugins/grid/application/cell/number_cell_bloc.dart index 5a85f007ad..1b8d55cf94 100644 --- a/frontend/app_flowy/lib/plugins/grid/application/cell/number_cell_bloc.dart +++ b/frontend/app_flowy/lib/plugins/grid/application/cell/number_cell_bloc.dart @@ -1,8 +1,7 @@ -import 'package:flowy_sdk/protobuf/flowy-error/errors.pb.dart'; +import 'package:flowy_sdk/log.dart'; import 'package:flutter_bloc/flutter_bloc.dart'; import 'package:freezed_annotation/freezed_annotation.dart'; import 'dart:async'; -import 'package:dartz/dartz.dart'; import 'cell_service/cell_service.dart'; part 'number_cell_bloc.freezed.dart'; @@ -20,20 +19,19 @@ class NumberCellBloc extends Bloc { initial: () { _startListening(); }, - didReceiveCellUpdate: (content) { - emit(state.copyWith(content: content)); + didReceiveCellUpdate: (cellContent) { + emit(state.copyWith(cellContent: cellContent ?? "")); }, updateCell: (text) { - cellController.saveCellData(text, resultCallback: (result) { - result.fold( - () => null, - (err) { - if (!isClosed) { - add(NumberCellEvent.didReceiveCellUpdate(right(err))); - } - }, - ); - }); + if (state.cellContent != text) { + emit(state.copyWith(cellContent: text)); + cellController.saveCellData(text, onFinish: (result) { + result.fold( + () {}, + (err) => Log.error(err), + ); + }); + } }, ); }, @@ -51,13 +49,22 @@ class NumberCellBloc extends Bloc { } void _startListening() { - _onCellChangedFn = cellController.startListening( - onCellChanged: ((cellContent) { - if (!isClosed) { - add(NumberCellEvent.didReceiveCellUpdate(left(cellContent ?? ""))); - } - }), - ); + _onCellChangedFn = + cellController.startListening(onCellChanged: ((cellContent) { + if (!isClosed) { + add(NumberCellEvent.didReceiveCellUpdate(cellContent)); + } + }), listenWhenOnCellChanged: (oldValue, newValue) { + // If the new value is not the same as the content, which means the + // backend formatted the content that user enter. For example: + // + // state.cellContent: "abc" + // oldValue: "" + // newValue: "" + // The oldValue is the same as newValue. So the [onCellChanged] won't + // get called. So just return true to refresh the cell content + return true; + }); } } @@ -65,20 +72,19 @@ class NumberCellBloc extends Bloc { class NumberCellEvent with _$NumberCellEvent { const factory NumberCellEvent.initial() = _Initial; const factory NumberCellEvent.updateCell(String text) = _UpdateCell; - const factory NumberCellEvent.didReceiveCellUpdate( - Either cellContent) = _DidReceiveCellUpdate; + const factory NumberCellEvent.didReceiveCellUpdate(String? cellContent) = + _DidReceiveCellUpdate; } @freezed class NumberCellState with _$NumberCellState { const factory NumberCellState({ - required Either content, + required String cellContent, }) = _NumberCellState; factory NumberCellState.initial(GridCellController context) { - final cellContent = context.getCellData() ?? ""; return NumberCellState( - content: left(cellContent), + cellContent: context.getCellData() ?? "", ); } } diff --git a/frontend/app_flowy/lib/plugins/grid/presentation/widgets/cell/number_cell.dart b/frontend/app_flowy/lib/plugins/grid/presentation/widgets/cell/number_cell.dart index 40a735a5df..c7141a43b3 100644 --- a/frontend/app_flowy/lib/plugins/grid/presentation/widgets/cell/number_cell.dart +++ b/frontend/app_flowy/lib/plugins/grid/presentation/widgets/cell/number_cell.dart @@ -29,8 +29,7 @@ class _NumberCellState extends GridFocusNodeCellState { final cellController = widget.cellControllerBuilder.build(); _cellBloc = getIt(param1: cellController) ..add(const NumberCellEvent.initial()); - _controller = - TextEditingController(text: contentFromState(_cellBloc.state)); + _controller = TextEditingController(text: _cellBloc.state.cellContent); super.initState(); } @@ -41,9 +40,8 @@ class _NumberCellState extends GridFocusNodeCellState { child: MultiBlocListener( listeners: [ BlocListener( - listenWhen: (p, c) => p.content != c.content, - listener: (context, state) => - _controller.text = contentFromState(state), + listenWhen: (p, c) => p.cellContent != c.cellContent, + listener: (context, state) => _controller.text = state.cellContent, ), ], child: Padding( @@ -80,20 +78,16 @@ class _NumberCellState extends GridFocusNodeCellState { _delayOperation?.cancel(); _delayOperation = Timer(const Duration(milliseconds: 30), () { if (_cellBloc.isClosed == false && - _controller.text != contentFromState(_cellBloc.state)) { + _controller.text != _cellBloc.state.cellContent) { _cellBloc.add(NumberCellEvent.updateCell(_controller.text)); } }); } } - String contentFromState(NumberCellState state) { - return state.content.fold((l) => l, (r) => ""); - } - @override String? onCopy() { - return _cellBloc.state.content.fold((content) => content, (r) => null); + return _cellBloc.state.cellContent; } @override diff --git a/frontend/rust-lib/flowy-grid/src/services/field/type_options/number_type_option/number_type_option_entities.rs b/frontend/rust-lib/flowy-grid/src/services/field/type_options/number_type_option/number_type_option_entities.rs index 7dd3ee82b3..326eb3db30 100644 --- a/frontend/rust-lib/flowy-grid/src/services/field/type_options/number_type_option/number_type_option_entities.rs +++ b/frontend/rust-lib/flowy-grid/src/services/field/type_options/number_type_option/number_type_option_entities.rs @@ -2,7 +2,7 @@ use crate::services::cell::{CellBytesCustomParser, CellProtobufBlobParser, Decod use crate::services::field::number_currency::Currency; use crate::services::field::{strip_currency_symbol, NumberFormat, STRIP_SYMBOL}; use bytes::Bytes; -use flowy_error::{FlowyError, FlowyResult}; +use flowy_error::FlowyResult; use rust_decimal::Decimal; use rusty_money::Money; use std::str::FromStr; @@ -40,7 +40,8 @@ impl NumberCellData { if num_str.chars().all(char::is_numeric) { Self::from_format_str(&num_str, sign_positive, format) } else { - Err(FlowyError::invalid_data().context("Should only contain numbers")) + // returns empty string if it can be formatted + Ok(Self::default()) } } }, diff --git a/frontend/rust-lib/flowy-grid/src/services/field/type_options/type_option_cell.rs b/frontend/rust-lib/flowy-grid/src/services/field/type_options/type_option_cell.rs index 295a24833d..2a1308e0e1 100644 --- a/frontend/rust-lib/flowy-grid/src/services/field/type_options/type_option_cell.rs +++ b/frontend/rust-lib/flowy-grid/src/services/field/type_options/type_option_cell.rs @@ -60,7 +60,7 @@ impl CellDataCacheKey { } hasher.write(field_rev.id.as_bytes()); hasher.write_u8(decoded_field_type as u8); - hasher.write(cell_str.as_bytes()); + cell_str.hash(&mut hasher); Self(hasher.finish()) } } @@ -125,8 +125,14 @@ where } } - let cell_data = self.decode_cell_str(cell_str, decoded_field_type, field_rev)?; + let cell_data = self.decode_cell_str(cell_str.clone(), decoded_field_type, field_rev)?; if let Some(cell_data_cache) = self.cell_data_cache.as_ref() { + tracing::trace!( + "Cell cache update: field_type:{}, cell_str: {}, cell_data: {:?}", + decoded_field_type, + cell_str, + cell_data + ); cell_data_cache.write().insert(key.as_ref(), cell_data.clone()); } Ok(cell_data) @@ -140,12 +146,13 @@ where ) { if let Some(cell_data_cache) = self.cell_data_cache.as_ref() { let field_type: FieldType = field_rev.ty.into(); + let key = CellDataCacheKey::new(field_rev, field_type.clone(), cell_str); tracing::trace!( - "Update cell cache field_type: {}, cell_data: {:?}", + "Cell cache update: field_type:{}, cell_str: {}, cell_data: {:?}", field_type, + cell_str, cell_data ); - let key = CellDataCacheKey::new(field_rev, field_type, cell_str); cell_data_cache.write().insert(key.as_ref(), cell_data); } } diff --git a/frontend/rust-lib/flowy-user/src/services/database.rs b/frontend/rust-lib/flowy-user/src/services/database.rs index 1ec25514f5..eaed88141c 100644 --- a/frontend/rust-lib/flowy-user/src/services/database.rs +++ b/frontend/rust-lib/flowy-user/src/services/database.rs @@ -18,7 +18,6 @@ impl UserDB { } } - #[tracing::instrument(level = "trace", skip(self))] fn open_user_db_if_need(&self, user_id: &str) -> Result, FlowyError> { if user_id.is_empty() { return Err(ErrorCode::UserIdIsEmpty.into());