-
Notifications
You must be signed in to change notification settings - Fork 0
Feature undo delete in headlines archive #54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a65be92
52bb3a2
0b56d3f
4da0c34
dfc4263
193b152
78468ff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
import 'dart:async'; | ||
|
||
import 'package:bloc/bloc.dart'; | ||
import 'package:core/core.dart'; | ||
import 'package:data_repository/data_repository.dart'; | ||
|
@@ -15,9 +17,18 @@ class ArchivedHeadlinesBloc | |
on<LoadArchivedHeadlinesRequested>(_onLoadArchivedHeadlinesRequested); | ||
on<RestoreHeadlineRequested>(_onRestoreHeadlineRequested); | ||
on<DeleteHeadlineForeverRequested>(_onDeleteHeadlineForeverRequested); | ||
on<UndoDeleteHeadlineRequested>(_onUndoDeleteHeadlineRequested); | ||
on<_ConfirmDeleteHeadlineRequested>(_onConfirmDeleteHeadlineRequested); | ||
} | ||
|
||
final DataRepository<Headline> _headlinesRepository; | ||
Timer? _deleteTimer; | ||
|
||
@override | ||
Future<void> close() { | ||
_deleteTimer?.cancel(); | ||
return super.close(); | ||
} | ||
|
||
Future<void> _onLoadArchivedHeadlinesRequested( | ||
LoadArchivedHeadlinesRequested event, | ||
|
@@ -96,22 +107,83 @@ class ArchivedHeadlinesBloc | |
DeleteHeadlineForeverRequested event, | ||
Emitter<ArchivedHeadlinesState> emit, | ||
) async { | ||
final originalHeadlines = List<Headline>.from(state.headlines); | ||
final headlineIndex = originalHeadlines.indexWhere((h) => h.id == event.id); | ||
_deleteTimer?.cancel(); | ||
|
||
final headlineIndex = state.headlines.indexWhere((h) => h.id == event.id); | ||
if (headlineIndex == -1) return; | ||
|
||
final updatedHeadlines = originalHeadlines..removeAt(headlineIndex); | ||
emit(state.copyWith(headlines: updatedHeadlines)); | ||
final headlineToDelete = state.headlines[headlineIndex]; | ||
final updatedHeadlines = List<Headline>.from(state.headlines) | ||
..removeAt(headlineIndex); | ||
|
||
emit( | ||
state.copyWith( | ||
headlines: updatedHeadlines, | ||
lastDeletedHeadline: headlineToDelete, | ||
), | ||
); | ||
|
||
_deleteTimer = Timer( | ||
const Duration(seconds: 5), | ||
() => add(_ConfirmDeleteHeadlineRequested(event.id)), | ||
); | ||
} | ||
|
||
Future<void> _onConfirmDeleteHeadlineRequested( | ||
_ConfirmDeleteHeadlineRequested event, | ||
Emitter<ArchivedHeadlinesState> emit, | ||
) async { | ||
try { | ||
await _headlinesRepository.delete(id: event.id); | ||
emit(state.copyWith(lastDeletedHeadline: null)); | ||
} on HttpException catch (e) { | ||
emit(state.copyWith(headlines: originalHeadlines, exception: e)); | ||
// If deletion fails, restore the headline to the list | ||
final originalHeadlines = List<Headline>.from(state.headlines) | ||
..add(state.lastDeletedHeadline!); | ||
emit( | ||
state.copyWith( | ||
headlines: originalHeadlines, | ||
exception: e, | ||
lastDeletedHeadline: null, | ||
), | ||
); | ||
} catch (e) { | ||
final originalHeadlines = List<Headline>.from(state.headlines) | ||
..add(state.lastDeletedHeadline!); | ||
emit( | ||
state.copyWith( | ||
headlines: originalHeadlines, | ||
exception: UnknownException('An unexpected error occurred: $e'), | ||
lastDeletedHeadline: null, | ||
), | ||
); | ||
} | ||
} | ||
|
||
void _onUndoDeleteHeadlineRequested( | ||
UndoDeleteHeadlineRequested event, | ||
Emitter<ArchivedHeadlinesState> emit, | ||
) { | ||
_deleteTimer?.cancel(); | ||
if (state.lastDeletedHeadline != null) { | ||
final updatedHeadlines = List<Headline>.from(state.headlines) | ||
..insert( | ||
state.headlines.indexWhere( | ||
(h) => | ||
h.updatedAt.isBefore(state.lastDeletedHeadline!.updatedAt), | ||
) != | ||
-1 | ||
? state.headlines.indexWhere( | ||
(h) => | ||
h.updatedAt.isBefore(state.lastDeletedHeadline!.updatedAt), | ||
) | ||
: state.headlines.length, | ||
state.lastDeletedHeadline!, | ||
); | ||
Comment on lines
+169
to
+182
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logic to find the insertion index for the undone headline calls You can improve this by calling
|
||
emit( | ||
state.copyWith( | ||
headlines: updatedHeadlines, | ||
lastDeletedHeadline: null, | ||
), | ||
); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ class ArchivedHeadlinesState extends Equatable { | |
this.hasMore = false, | ||
this.exception, | ||
this.restoredHeadline, | ||
this.lastDeletedHeadline, | ||
}); | ||
|
||
final ArchivedHeadlinesStatus status; | ||
|
@@ -25,6 +26,7 @@ class ArchivedHeadlinesState extends Equatable { | |
final bool hasMore; | ||
final HttpException? exception; | ||
final Headline? restoredHeadline; | ||
final Headline? lastDeletedHeadline; | ||
|
||
ArchivedHeadlinesState copyWith({ | ||
ArchivedHeadlinesStatus? status, | ||
|
@@ -33,14 +35,16 @@ class ArchivedHeadlinesState extends Equatable { | |
bool? hasMore, | ||
HttpException? exception, | ||
Headline? restoredHeadline, | ||
Headline? lastDeletedHeadline, | ||
}) { | ||
return ArchivedHeadlinesState( | ||
status: status ?? this.status, | ||
headlines: headlines ?? this.headlines, | ||
cursor: cursor ?? this.cursor, | ||
hasMore: hasMore ?? this.hasMore, | ||
exception: exception ?? this.exception, | ||
exception: exception, | ||
restoredHeadline: restoredHeadline, | ||
lastDeletedHeadline: lastDeletedHeadline, | ||
Comment on lines
+45
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The implementation of This pattern is especially dangerous for A safer I suggest reverting to the safer
|
||
); | ||
} | ||
|
||
|
@@ -52,5 +56,6 @@ class ArchivedHeadlinesState extends Equatable { | |
hasMore, | ||
exception, | ||
restoredHeadline, | ||
lastDeletedHeadline, | ||
]; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,3 @@ | ||
export 'content_status_l10n.dart'; | ||
export 'source_type_l10n.dart'; | ||
export 'string_truncate.dart'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
extension StringTruncate on String { | ||
String truncate(int maxLength) { | ||
if (length <= maxLength) { | ||
return this; | ||
} | ||
return '${substring(0, maxLength)}...'; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling logic in the
on HttpException
andcatch (e)
blocks is duplicated. This can be simplified by using a singlecatch
block and checking the exception type within it. This will make the code more concise and easier to maintain.