From bd9be437fa3ce90c1b4e2df694ff8eebe01eafa8 Mon Sep 17 00:00:00 2001 From: Abdul Ahad Date: Thu, 17 Oct 2024 19:10:34 +0530 Subject: [PATCH 1/5] remove unecessary images from localstorage --- .../header/document_cover_widget.dart | 18 +++++++++++++++--- .../editor_plugins/image/image_util.dart | 4 ++++ 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/document_cover_widget.dart b/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/document_cover_widget.dart index 60dce56e99b99..cb350198d6496 100644 --- a/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/document_cover_widget.dart +++ b/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/document_cover_widget.dart @@ -707,12 +707,24 @@ class DocumentCoverState extends State { } Future onCoverChanged(CoverType type, String? details) async { - if (type == CoverType.file && details != null && !isURL(details)) { + bool isFileType(CoverType type, String? details) => + type == CoverType.file && details != null && !isURL(details); + + if (isFileType(type, details)) { if (_isLocalMode()) { - details = await saveImageToLocalStorage(details); + final previousType = + widget.node.attributes[DocumentHeaderBlockKeys.coverType]; + final previousDetails = + widget.node.attributes[DocumentHeaderBlockKeys.coverDetails]; + + if (isFileType(previousType, previousDetails)) { + await deleteImageFromLocalStorage(previousDetails); + } + + details = await saveImageToLocalStorage(details!); } else { // else we should save the image to cloud storage - (details, _) = await saveImageToCloudStorage(details, widget.view.id); + (details, _) = await saveImageToCloudStorage(details!, widget.view.id); } } widget.onChangeCover(type, details); diff --git a/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/image/image_util.dart b/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/image/image_util.dart index efa5721382fc3..c807a70baca2c 100644 --- a/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/image/image_util.dart +++ b/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/image/image_util.dart @@ -117,3 +117,7 @@ Future> extractAndUploadImages( return images; } + +Future deleteImageFromLocalStorage(String localImagePath) async { + await File(localImagePath).delete(); +} From 5c6b18b0c65f4d606cf0b1f28d68b8dfc0bbb8e6 Mon Sep 17 00:00:00 2001 From: Abdul Ahad Date: Mon, 21 Oct 2024 21:41:55 +0530 Subject: [PATCH 2/5] feat: Add handler for deleting previous cover image on cover image change --- .../header/document_cover_widget.dart | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/document_cover_widget.dart b/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/document_cover_widget.dart index 50f3635b0c0ce..a6a5d7b0ba9f4 100644 --- a/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/document_cover_widget.dart +++ b/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/document_cover_widget.dart @@ -763,29 +763,32 @@ class DocumentCoverState extends State { } Future onCoverChanged(CoverType type, String? details) async { - bool isFileType(CoverType type, String? details) => - type == CoverType.file && details != null && !isURL(details); - - if (isFileType(type, details)) { + await deletePreviousImageIfNecessary(); + if (type == CoverType.file && details != null && !isURL(details)) { if (_isLocalMode()) { - final previousType = - widget.node.attributes[DocumentHeaderBlockKeys.coverType]; - final previousDetails = - widget.node.attributes[DocumentHeaderBlockKeys.coverDetails]; - - if (isFileType(previousType, previousDetails)) { - await deleteImageFromLocalStorage(previousDetails); - } - - details = await saveImageToLocalStorage(details!); + details = await saveImageToLocalStorage(details); } else { // else we should save the image to cloud storage - (details, _) = await saveImageToCloudStorage(details!, widget.view.id); + (details, _) = await saveImageToCloudStorage(details, widget.view.id); } } widget.onChangeCover(type, details); } + Future deletePreviousImageIfNecessary() async { + final previousType = CoverType.fromString( + widget.node.attributes[DocumentHeaderBlockKeys.coverType]); + final previousDetails = + widget.node.attributes[DocumentHeaderBlockKeys.coverDetails]; + + bool isFileType(CoverType type, String? details) => + type == CoverType.file && details != null && !isURL(details); + + if (isFileType(previousType, previousDetails) && _isLocalMode()) { + await deleteImageFromLocalStorage(previousDetails); + } + } + void setOverlayButtonsHidden(bool value) { if (isOverlayButtonsHidden == value) return; setState(() { From bf6b3ef4101ebd791aba312265001dddc5ac89bb Mon Sep 17 00:00:00 2001 From: Abdul Ahad Date: Mon, 21 Oct 2024 22:12:17 +0530 Subject: [PATCH 3/5] fix: add local image case for versions after 0.5.5 --- .../editor_plugins/migration/editor_migration.dart | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/migration/editor_migration.dart b/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/migration/editor_migration.dart index 8463030667f40..dcda2941ffe88 100644 --- a/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/migration/editor_migration.dart +++ b/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/migration/editor_migration.dart @@ -226,6 +226,14 @@ class EditorMigration { }, }; } + } else { + extra = { + ViewExtKeys.coverKey: { + ViewExtKeys.coverTypeKey: + PageStyleCoverImageType.localImage.toString(), + ViewExtKeys.coverValueKey: coverDetails, + }, + }; } break; default: From e66835297a2dc84febf8d4f1517aa64252fc4624 Mon Sep 17 00:00:00 2001 From: Abdul Ahad Date: Tue, 22 Oct 2024 19:04:30 +0530 Subject: [PATCH 4/5] fix: add try catch block and delete action to bottom of function --- .../header/document_cover_widget.dart | 27 +++++++++---------- .../editor_plugins/image/image_util.dart | 6 ++++- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/document_cover_widget.dart b/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/document_cover_widget.dart index a6a5d7b0ba9f4..f5781bd9e714b 100644 --- a/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/document_cover_widget.dart +++ b/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/header/document_cover_widget.dart @@ -763,27 +763,26 @@ class DocumentCoverState extends State { } Future onCoverChanged(CoverType type, String? details) async { - await deletePreviousImageIfNecessary(); - if (type == CoverType.file && details != null && !isURL(details)) { - if (_isLocalMode()) { - details = await saveImageToLocalStorage(details); - } else { - // else we should save the image to cloud storage - (details, _) = await saveImageToCloudStorage(details, widget.view.id); - } - } - widget.onChangeCover(type, details); - } - - Future deletePreviousImageIfNecessary() async { final previousType = CoverType.fromString( - widget.node.attributes[DocumentHeaderBlockKeys.coverType]); + widget.node.attributes[DocumentHeaderBlockKeys.coverType], + ); final previousDetails = widget.node.attributes[DocumentHeaderBlockKeys.coverDetails]; bool isFileType(CoverType type, String? details) => type == CoverType.file && details != null && !isURL(details); + if (isFileType(type, details)) { + if (_isLocalMode()) { + details = await saveImageToLocalStorage(details!); + } else { + // else we should save the image to cloud storage + (details, _) = await saveImageToCloudStorage(details!, widget.view.id); + } + } + widget.onChangeCover(type, details); + + // After cover change,delete from localstorage if previous cover was image type if (isFileType(previousType, previousDetails) && _isLocalMode()) { await deleteImageFromLocalStorage(previousDetails); } diff --git a/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/image/image_util.dart b/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/image/image_util.dart index c807a70baca2c..47c4274135939 100644 --- a/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/image/image_util.dart +++ b/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/image/image_util.dart @@ -119,5 +119,9 @@ Future> extractAndUploadImages( } Future deleteImageFromLocalStorage(String localImagePath) async { - await File(localImagePath).delete(); + try { + await File(localImagePath).delete(); + } catch (e) { + Log.error('cannot delete image file', e); + } } From 86ffa676f2f315f17a2cae8fb8b74648dd7596ee Mon Sep 17 00:00:00 2001 From: Abdul Ahad Date: Thu, 31 Oct 2024 20:17:08 +0530 Subject: [PATCH 5/5] chore: add test case for uploading and deleting image in localmode --- .../document_with_cover_image_test.dart | 61 +++++++++++++++++++ .../editor_plugins/image/image_util.dart | 7 ++- 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/frontend/appflowy_flutter/integration_test/desktop/document/document_with_cover_image_test.dart b/frontend/appflowy_flutter/integration_test/desktop/document/document_with_cover_image_test.dart index c365c1bad52fc..bcc995682c588 100644 --- a/frontend/appflowy_flutter/integration_test/desktop/document/document_with_cover_image_test.dart +++ b/frontend/appflowy_flutter/integration_test/desktop/document/document_with_cover_image_test.dart @@ -1,13 +1,21 @@ +import 'dart:io'; + import 'package:appflowy/generated/locale_keys.g.dart'; import 'package:appflowy/plugins/document/presentation/editor_plugins/header/document_cover_widget.dart'; +import 'package:appflowy/plugins/document/presentation/editor_plugins/image/image_util.dart'; import 'package:appflowy_backend/protobuf/flowy-folder/view.pb.dart'; import 'package:easy_localization/easy_localization.dart'; +import 'package:flutter/gestures.dart'; import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; import 'package:flutter_emoji_mart/flutter_emoji_mart.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:integration_test/integration_test.dart'; +import 'package:path_provider/path_provider.dart'; +import 'package:path/path.dart' as p; import '../../shared/emoji.dart'; +import '../../shared/mock/mock_file_picker.dart'; import '../../shared/util.dart'; void main() { @@ -50,6 +58,59 @@ void main() { await tester.editor.tapOnRemoveCover(); tester.expectToSeeNoDocumentCover(); }); + + testWidgets('document cover local image tests', (tester) async { + await tester.initializeAppFlowy(); + await tester.tapAnonymousSignInButton(); + + tester.expectToSeeNoDocumentCover(); + + // Hover over cover toolbar to show 'Add Cover' and 'Add Icon' buttons + await tester.editor.hoverOnCoverToolbar(); + + // Insert a document cover + await tester.editor.tapOnAddCover(); + tester.expectToSeeDocumentCover(CoverType.asset); + + // Hover over the cover to show the 'Change Cover' and delete buttons + await tester.editor.hoverOnCover(); + tester.expectChangeCoverAndDeleteButton(); + + // Change cover to a local image image + final imagePath = await rootBundle.load('assets/test/images/sample.jpeg'); + final tempDirectory = await getTemporaryDirectory(); + final localImagePath = p.join(tempDirectory.path, 'sample.jpeg'); + final imageFile = File(localImagePath) + ..writeAsBytesSync(imagePath.buffer.asUint8List()); + + await tester.editor.hoverOnCover(); + await tester.editor.tapOnChangeCover(); + + final uploadButton = find.findTextInFlowyText( + LocaleKeys.document_imageBlock_upload_label.tr(), + ); + await tester.tapButton(uploadButton); + + mockPickFilePaths(paths: [localImagePath]); + await tester.tapButtonWithName( + LocaleKeys.document_imageBlock_upload_placeholder.tr(), + ); + + await tester.pumpAndSettle(); + tester.expectToSeeDocumentCover(CoverType.file); + + // Remove the cover + await tester.editor.hoverOnCover(); + await tester.editor.tapOnRemoveCover(); + tester.expectToSeeNoDocumentCover(); + + // Test if deleteImageFromLocalStorage(localImagePath) function is called once + await tester.pump(kDoubleTapTimeout); + expect(deleteImageTestCounter, 1); + + // delete temp files + await imageFile.delete(); + }); testWidgets('document icon tests', (tester) async { await tester.initializeAppFlowy(); diff --git a/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/image/image_util.dart b/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/image/image_util.dart index 47c4274135939..74d195531230d 100644 --- a/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/image/image_util.dart +++ b/frontend/appflowy_flutter/lib/plugins/document/presentation/editor_plugins/image/image_util.dart @@ -118,9 +118,14 @@ Future> extractAndUploadImages( return images; } +@visibleForTesting +int deleteImageTestCounter = 0; + Future deleteImageFromLocalStorage(String localImagePath) async { try { - await File(localImagePath).delete(); + await File(localImagePath) + .delete() + .whenComplete(() => deleteImageTestCounter++); } catch (e) { Log.error('cannot delete image file', e); }