Skip to content
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

feat: Use quill_native_bridge as default impl in DefaultClipboardService, fix related bugs in the extensions package #2230

Merged
merged 102 commits into from
Oct 16, 2024

Conversation

EchoEllet
Copy link
Collaborator

@EchoEllet EchoEllet commented Sep 15, 2024

Description

Add default native implementation of ClipboardService for Android, macOS, iOS, Windows, and Web.

Note

The package quill_native_bridge has been moved into a separate repo (FlutterQuill/quill-native-bridge).

The rich text-pasting feature now works without any additional plugins, no need for bridges or installing Rust.

This feature is introduced to allow moving super_clipboard into quill_super_clipboard (super_clipboard will be no longer a dependency of flutter_quill or flutter_quill_extensions) without introducing a breaking change.

Feature iOS Android macOS Windows Linux Web
isIOSSimulator
getClipboardHtml
copyHtmlToClipboard
copyImageToClipboard
getClipboardImage
getClipboardGif
getClipboardFiles
Rich Text Paste Flutter Quill Example

Android Example

image

The example above doesn't call this method from flutter_quill_extensions:

FlutterQuillExtensions.useSuperClipboardPlugin()
Videos

QuillExampleAndroid.mov
QuillExampleIos.mov
QuillExampleMacOS.mov

Manually tested on both real device and emulator/simulator.

Related Issues

Type of Change

  • New feature: Adds new functionality without breaking existing features.
  • 🛠️ Bug fix: Resolves an issue without altering current behavior.
  • 🧹 Code refactor: Code restructuring that does not affect behavior.
  • Breaking change: Alters existing functionality and requires updates.
  • 🧪 Tests: Adds new tests or modifies existing tests.
  • 📝 Documentation: Updates or additions to documentation.
  • 🗑️ Chore: Routine tasks, or maintenance.
  • Build configuration change: Changes to build or deploy processes.

TODOS

  • Cleanup the code for native platforms Done only for Kotlin and Swift implementations, web, Linux, and Windows still need more cleanup.
  • Cleanup the dart method channel code (MethodChannelQuillNativeBridge and related classes)
  • Remove optional template assets and files for the example of quill_native_bridge Will leave them as they don't update often.
  • Retrieving HTML from the Clipboard on Android, iOS, and macOS.
  • Copying an image to the Clipboard on Android, iOS, and macOS.
  • Retrieving images from the Clipboard on Android, iOS, and macOS.
  • Retrieving gif images from the Clipboard on Android, iOS, and macOS. Was not able to support macOS since not natively supported same as iOS. Supported only on Android and iOS.
  • See and discuss if getClipboardImage should or should not return a Gif file, if not, should getClipboardImage fallback to returning the Gif image in case of unsupported operating systems? Related feat: add paste gif from clipboard #1788. Should be separated, gif is not supported on desktop and web.
  • See if there is a need for moving the web implementation into its own package (quill_native_bridge_web and quill_native_bridge_platform_interface), also we might consider throwing assert failure on all methods in case of web in MethodChannelQuillNativeBridge.dart since will use QuillNativeBridgeWeb for the web. Web implementation has been separated into its own package.
  • Configure the flutter_quill example AndroidManifest.xml to support copying images to the clipboard.
  • See if retrieving Markdown from the clipboard is a standard feature on all platforms or if there is a use-case to it. Doesn't seem to be a standard feature on most platforms.
  • See if retrieving HTML and Markdown files is a standard feature on all platforms or if it's a desktop-specific feature Seems to be a desktop-specific feature, AFAIK is not supported on Android desktop mode.
  • See if we need to separate retrieving PNG images from GIF images, currently, we have different methods getClipboardImage and getGifImage but they can be in the same function and will always return the image regardless of what it was. We probably do want to separate them so that users can have separate implementations for onImagePaste and onGifPaste
  • Review the Clipboard Impl for Android, and see where we would use coerceToHtmlText(), currently we're always retrieving the first item. The main point of retrieving HTML is to have rich text that can be converted and pasted into the editor, having plain text wrapped to HTML is not what we expect, this functionality is not available built-in on other platforms and we're trying to have those methods unified when possible. This feature of rich pasting will be only supported if the app you're copying content from supports it.
  • Might implement a new feature which is copying Delta as HTML to the system clipboard to paste it for other apps (might not do it for now or only for example). The copyHTMLToClipboard has been added for integration tests but is not used in ClipboardService.
  • Simplify ClipboardService
  • Fix issue Rich Text Paste Duplicates Content via HTML & Plain Text on Web #2220 for web This is a separate issue. Will focus on the current issue for now.
  • Might update Fix safari clipboard bug #1722 only to fix the bug for Safari instead, discuss the change first. Seems to be unrelated.
  • See which features can be implemented on the web and the restrictions, we will probably use the Clipboard API on all browsers that support it and fall back to events (such as paste event) for Firefox. Retrieving GIF images from the clipboard is not supported on the web. Was able to use the Clipboard API instead of fallback to Clipboard events on Firefox without any issues. See [Web] Pasting HTML Doesn't Work Propertly #1998
  • Update docs and references to super_clipbaord in flutter_quill and flutter_quill_extensions. Updated, still need minor adjustments once we make it possible to disable those experimental features.
  • Discuss if we need to introduce a breaking change for flutter_quill_extensions to move super_clipboard into its own package or provide an example code instead. Once Windows support is complete, removing super_clipboard shouldn't be a breaking change since the quill_native_bridge all currently used features by super_clipboard except getting Markdown and HTML files on the desktop.
  • Support Linux Wayland, currently only support x11 Moved into a separate issue in quill-native-bridge repo.
  • See Flutter #127396 and decide if we should use the base keyword (introduced in Dart 3) or stick to plugin_platform_interface for quill_native_bridge (will probably do) Since no decision has made by the Flutter team yet, and haven't discussed this change, decided to stick to plugin_platform_interface for now.
  • Update the minimum version requirement of quill_native_bridge and flutter_quill_extensions in flutter_quill once done before publishing a new release.
  • The ClipboardService and related classes should be experimental for now by adding a comment and experimental annotation. We may consider removing ClipboardServiceProvider and accessing the instance directly from ClipboardService if being experimental is the only issue.
  • Add the option to not use some of the features in this PR. This is a separate issue and might be solved in a separate PR, this PR focuses on replacing the implementation. See this comment, Feat: support for customize paste function #2156 and The option to disable the Rich clipboard feature or customize the clipboard behavior #2320.
  • Add basic integration tests for at least some of the functionality.
  • Check the format of Kotlin, and Swift in GitHub workflow. Will be in a separate GitHub workflow once we move the quill_native_bridge into its own package. Currently, Swift and Kotlin code is formatted. Moved into a separate issue in quill-native-bridge.
  • Currently on Android, will return the HTML/Image from the primary clip item (the last item has been copied), if the user copies HTML, then an image, and if getClipboardHTML was called after that then will be null since that's not the last/primary item (which is the image in this case). We should consider this and if it's the behavior that we expect. The current behavior is the expected, at least on most applications. However, we should confirm if the current solution is consistent across different platforms when possible.
  • Separate the plugin platform interface from quill_native_bridge
  • Provide a different way to check the platform support for a method so that implementations of QuillNativeBridgePlatform (outside of quill_native_bridge) can decide if its unsupported feature, if we update a package implementation (e.g. quill_native_bridge_windows) and add support for a method, updating quill_native_bridge or quill_native_bridge_platform_interface is not required. Allowing to check if Clipboard API is supported in QuillNativeBridgeWeb.
  • Improve the error handling in general, throwing exceptions instead of using asserts in quill_native_bridge_windows, and quill_native_bridge_linux. Also, Handle Process exception on Linux. Moved to a separate issue in quill-native-bridge repo
  • Error handling that's platform-specific should be separated from the common platform interface MethodChannelQuillNativeBridge (quill_native_bridge_platform_interface). We're handling Android-specific logic in the MethodChannelQuillNativeBridge.
  • Separate Android and Apple (iOS, macOS) implementation into their package. At first decided to have macOS and iOS in one package quill_native_bridge_apple.
  • Use pigeon for Kotlin and Swift.
  • Update flutter_lints to latest version 5.0.0 and might update the lints for quill_native_bridge Addressed in chore(deps): update dev dependency flutter_lints to 5.0.0, solve warnings #2292
  • Test copyImageToClipboard and getClipboardImage with common image formats on all platforms. Moved into a separate issue in quill-native-bridge repo.
  • Once getClipboardFiles supports web, avoid using dart:io in DefaultClipboardService for getHtmlFile and getMarkdownFile methods (solve the TODO, currently return null for web). Lack of support for getClipboardFiles on the web blocks the fix, Moved to a separate issue in quill-native-bridge repo.
  • Solve TODOs in the ClipboardService of flutter_quill

Support for Windows is very limited and highly experimental.

…dHTML() method channel for Android and iOS, example and Android platform directory
@EchoEllet EchoEllet self-assigned this Sep 15, 2024
@EchoEllet EchoEllet added the in progress This issue or feature is currently being worked on by someone. label Sep 15, 2024
@EchoEllet EchoEllet force-pushed the feat/default-clipboard-native-impl branch from e3cc4c5 to e6ff099 Compare September 15, 2024 21:21
@EchoEllet EchoEllet changed the title feat(quill_native_bridge): add initial implementation for getClipboardHTML() method channel for Android and iOS, example and Android platform directory feat(quill_native_bridge): add initial implementation for getClipboardHTML() method channel for Android, macOS and iOS, example Sep 15, 2024
@EchoEllet EchoEllet changed the title feat(quill_native_bridge): add initial implementation for getClipboardHTML() method channel for Android, macOS and iOS, example feat(quill_native_bridge): Add default native implementation of ClipboardService for Android, macOS, and iOS Sep 16, 2024
@EchoEllet EchoEllet force-pushed the feat/default-clipboard-native-impl branch from 7eb67dc to 9743d47 Compare September 16, 2024 19:53
@EchoEllet EchoEllet changed the title feat(quill_native_bridge): Add default native implementation of ClipboardService for Android, macOS, and iOS feat(quill_native_bridge): Add default native implementation of ClipboardService for Android, macOS, iOS and web Sep 16, 2024
EchoEllet added a commit to FlutterQuill/quill-super-clipboard that referenced this pull request Oct 15, 2024
EchoEllet added a commit to FlutterQuill/quill-super-clipboard that referenced this pull request Oct 15, 2024
EchoEllet added a commit to FlutterQuill/quill-super-clipboard that referenced this pull request Oct 15, 2024
@EchoEllet
Copy link
Collaborator Author

EchoEllet commented Oct 16, 2024

This PR took longer than expected due to adding a feature to customize clipboard behavior, allowing users to disable the rich clipboard feature (see this comment). This feature was unrelated to this PR and was a pre-existing issue.

I will finalize this PR for merging and move the related issues to the FlutterQuill/quill-native-bridge repo.

@CatHood0 @AtlasAutocode If you have time, can you review this PR? I have moved quill_native_bridge to FlutterQuill/quill-native-bridge to minimize changes in this repo..

@EchoEllet EchoEllet changed the title feat(quill_native_bridge): Add default native implementation of ClipboardService for Android, Linux, macOS, iOS, Windows and Web feat: Use quill_native_bridge as default impl in DefaultClipboardService, fix related bugs in the extensions package Oct 16, 2024
@singerdmx singerdmx marked this pull request as ready for review October 16, 2024 15:57
@EchoEllet EchoEllet merged commit a9859f9 into master Oct 16, 2024
2 checks passed
@EchoEllet EchoEllet deleted the feat/default-clipboard-native-impl branch October 16, 2024 23:46
@EchoEllet
Copy link
Collaborator Author

I usually prefer more code review though we need to prioritize other issues.

@EchoEllet
Copy link
Collaborator Author

EchoEllet commented Oct 17, 2024

Noticed that the clipboardPaste() handle:

  • Internal image paste
  • HTML and Markdown rich paste
  • Plain Text Paste
  • Provide support for the onClipboardPaste callback which will be used if none of those are available, we should instead provide something to completely override this behavior, we should expect either true or false in that callback and will be used at first, if true is used mean it's being handled as expected, if false then it means that the user wants to fallback to our default handling. It's similar to the , similar to customVideoBuilder which except a value and return null instead of false. Maybe we want a more advanced approach for further customization, we want to know if the user wants to fallback to our default handling, or use their own and see if the operation was done successfully or not, the user should be able to decide if they want to call bringIntoView(textEditingValue.selection.extent) on success or failure, so we might use enum or sealed class instead of true and false, which will also make it more readable and easier to understand.

The pasteText():

  • Will return if the editor is in read-only mode
  • Call clipboardPaste() and return if the paste operation is called successfully.
  • Attempt to use onImagePaste and onGifPaste, which won't call bringIntoView(textEditingValue.selection.extent) in case of success.

Our current design is inconsistent. There is also a bug (unrelated to this PR), Clipboard.getData(Clipboard.kTextPlain) doesn't work always as expected (see issue #2323). It will always try to return the clipboard item as plain text, the behavior is platform-dependent, on iOS if you copy an image from the browser and then paste, with our current code it will paste the image URL instead of the image even if it's available, that's because we currently prefer Clipboard.getData(Clipboard.kTextPlain).text over onImagePaste and onGifPaste.

image

The solution is to either:

  • Provide a new platform method getClipboardText() in quill_native_bridge and ensure to only return a String if there is a plain text clipboard item, not another item, and then cast it, super_clipboard also has this functionality.
  • Change the order of the handling

In either case, we will need to refactor some things in version 11.0.0 since some things are broken and we still need to provide a way to customize the clipboard behavior, it's a commonly requested feature and it will allow the user to override the default if there is a bug (which currently have some bugs) or if the user doesn't want the default.

@AtlasAutocode When you have the time, can you explain the design and why pasteText() and clipboardPaste() are different (as you separate them in #1843)?

@AtlasAutocode
Copy link
Collaborator

pasteText is in the editor and should only call the controller's clipboardPaste function. I see no reason for pasteText to call clipboard image functions directly. Presumably this is historical code left so as not to introduce a breaking change.

@EchoEllet
Copy link
Collaborator Author

EchoEllet commented Oct 18, 2024

Thanks for the reply. The image paste was supported initially with pasteboard, then I removed it and introduced super_clipboard. However, in 11.0.0, I'm still unsure about the new behavior.

@EchoEllet
Copy link
Collaborator Author

Presumably this is historical code left so as not to introduce a breaking change.

I'm still not sure how will we handle this in 11.0.0, passing the editor config to the controller and accessing it from there is something I want to avoid since we haven't understood the existing structure properly, how about we move the clipboard paste function back from the QuillController to where it was so we don't have to move onImagePaste and onGifPaste to the controller config?

This feature can still be implemented without having to provide the clipboardPaste() in the controller, instead, we can only provide the internal image and delta stored within the controller.

CatHood0 pushed a commit to CatHood0/flutter-quill that referenced this pull request Oct 28, 2024
…ice, fix related bugs in the extensions package (singerdmx#2230)

* feat(quill_native_bridge): add initial implementation for getClipboardHTML() method channel for Android and iOS, example and Android platform directory

* chore(example): use quill_native_bridge from path in dependency_overrides

* feat: use quill_native_bridge for HTML Clipboard in flutter_quill and the example (initial impl)

* feat(quill_native_bridge): extend the support for macOS.

* chore(example): clear todos in app module build.gradle

* chore(android): additional check before accessing the clip data item

* fix(flutter_quill_extensions): unrelated bug for copying an image (needs further fixes)

* feat: add copyImageToClipboard() method channel, an asset file in the example, and set supported platforms in pubspec.yaml, configure quill_native_bridge example

* docs(readme): add an optional step to configure copying images to the clipboard on Android for quill_native_bridge

* feat: use copyImageToClipboard() in flutter_quill_extensions, fix common issues in the copy image button

* chore: correct name of _loadImageBytesFromImageProvider()

* feat: add web support for copyImageToClipboard()

* feat: (WIP) retrieving image files from the clipboard, simplifying ClipboardService, slightly updating Android, disabling copyImageToClipboard() for web (temporarily). WIP

* chore(example): rename _flutterQuillAssetImage to _kFlutterQuillAssetImage

* chore(example): update error messages in quill_native_bridge example

* feat: add support for gif, fix unhandled exception on Android only when getting an image from the clipboard before closing the app on a new app start, clean up the platform support check

* chore(example): configure the Android example for copying images to the system clipboard, update README.md

* feat: support for web (WIP, will be updated soon)

* chore: annotate ClipboardService and related classes as experimental

* chore: temporarily add dependency_overrides to fix build failure - revert this change later

* feat: separate web implementation from quill_native_bridge into quill_native_bridge_web

* chore: update description of quill_native_bridge_web

* chore(web): avoid using jsify() when possible

* chore: publish and use quill_native_bridge_web in quill_native_bridge, update to the newly published quill_native_bridge in flutter_quill

* docs(readme): use pub.dev link of quill_native_bridge in quill_native_bridge_web

* chore: use .toJS in clipboardItem map (minor change)

* fix: use hosted quill_native_bridge in dependencies and override with the local path in dependency_overrides

* fix: update minimum required version of Flutter/Dart by quill_native_bridge

* test: write basic integration tests for getClipboardImage() and copyImageToClipboard()

* feat: add copyHTMLToClipboard(), add integration tests for copyHTMLToClipboard() and getClipboardHTML(), minor cleanup in the example

* chore: cleanup QuillNativeBridgeWeb, extract mime constants, organize the order of implemented methods

* fix(web): issue caused by previous commit

* docs(web): update methods of QuillNativeBridge to reflect the web support

* chore(android): cleanup the check for HTML from the clipboard in getClipboardHTML

* test: add a test for getClipboardHTML to ensure it's not null only when the HTML item is the last/primary

* ci: attempt to fix CI failure by updating the outdated path of quill_native_bridge after splitting the web implementation

* docs: document why platform implementations of QuillNativeBridgePlatform should extend it rather than implements

* chore(example): add Windows platform runner example using Flutter CLI (change is automated)

* feat: (WIP) add windows experimental support for getClipboardHTML(), new integration test to ensure the HTML is parsable

* fix(docs): update incorrect description of quill_native_bridge_windows in README

* refactor: move plugin platform interface to quill_native_bridge_platform_interface

* chore: (WIP): publish quill_native_bridge 10.7.5

* chore: (WIP) publish quill_native_bridge_web to use quill_native_bridge_platform_interface

* chore(example): add Linux platform runner example using Flutter CLI (change is automated)

* feat(linux): (WIP) highly experimental Linux support using xclip

* fix(windows): non case sensitive check in stripWin32HtmlDescription() suggested by @AtlasAutocode

* chore(example): unrelated change for windows in Flutter Quill example due to using flutter_inappwebview_windows without running the example on windows (outside of this PR)

* chore(android): cleanup onMethodCall() in QuillNativeBridgePlugin

* docs(readme): update docs to reflect the changes

* chore: use hasWebSupport instead of hardcoding the check directly in QuillNativeBridgePlatformFeature.isSupported

* feat: rename MethodChannelQuillNativeBridge.methodChannel to testMethodChannel, check in development mode to ensure that testMethodChannel can be only used in unit tests for non-web platforms

* refactor: move android implementation from quill_native_bridge to quill_native_bridge_android, use pigeon for Android

* chore: update QuillNativeBridgeAndroid.registerWith() assert message

* refactor(apple): move iOS and macOS implementation from quill_native_bridge to quill_native_bridge_ios and quill_native_bridge_macos, use pigeon for iOS and macOS, clean the code and update QuillNativeBridge to QuillNativeBridgeImpl on Android

* chore: add quill_native_bridge_platform_interface in pubspec_overides.yaml of quill_native_bridge

* refactor: rename copyHTMLToClipboard() and getClipboardHTML() to copyHtmlToClipboard() and getClipboardHtml()

* feat: more advanced method to check the platform support check allowing each platform to have its own check, publish a new experimental version of quill_native_version

* chore(analysis): remove unused imports in quill_native_bridge and quill_native_bridge_platform_interface

* fix(ci): temporarily pub get quill_native_bridge packages in main workflow

* chore: format dart pigeon generated files as a workaround to CI failure

* chore(deps): update flutter_lints to 5.0.0 in quill_native_bridge

* refactor(windows): move CloseClipboard() to finally block in getClipboardHtml()

* chore(windows): clear todo related to previous commit

* chore(readme): remove description from the platform support table, use emojis instead of text for readability

* chore(readme): use different emoji for platforms that don't support a feature or are inapplicable in the platform support table

* style(windows): use TEXT and free from win32 instead of toNativeUtf16() and calloc.free(pointer) from ffi

* refactor(windows): only register HTML format id once

* feat(windows): (WIP) highly experimental support for copyHtmlToClipboard(), minor cleanup (need more)

* refactor: extract supported platforms into Set for isSupported()

* chore(windows): add error code in errors using GetLastError()

* chore(windows): avoid passing the locked memory pointer to SetClipboardData() in copyHtmlToClipboard()

* docs(readme): document android platform configuration in quill_native_bridge

* fix(windows): (WIP) GlobalUnlock before SetClipboardData and GlobalFree on SetClipboardData failure

* docs(readme): slightly improve platform configuration docs in quill_native_bridge

* fix(windows): GlobalUnlock() the handle instead of pointer before calling GlobalUnlock()

* chore(android): add reference to Flutter #63533

* chore(android): define package in GeneratedMessages.kt, update related imports

* chore(android): ignore analysis warning for the generated code messages.g.dart

* feat: (WIP) add initial support for getClipboardFiles()

* chore(ci): format generated messages.g.dart file to bypass CI failure

* feat(linux): add support for getClipboardFiles()

* docs: update the old reference of QuillNativeBridgeFeature, add doc comment for the library

* chore(readme): add link for quill_native_bridge, correct minor typo

* chore(docs): minor changes to update the doc comment in QuillController

* chore: move quill_native_bridge to https://github.com/FlutterQuill/quill-native-bridge

* docs(readme): improve the 'Rich Text Paste' section, drop the TODO for providing a way to customize the paste behavior and customize the clipboard operations

* chore: update deprecation message of FlutterQuillExtensions.useSuperClipboardPlugin() to reflect the change

* chore(example): remove the comment of deprecated method FlutterQuillExtensions.useSuperClipboardPlugin()

* chore: rename ClipboardService.copyImageToClipboard() to ClipboardService.copyImage() (non-breaking change)

* chore: add a link to a TODO in DefaultClipboardService._getClipboardFile()
CatHood0 pushed a commit to CatHood0/flutter-quill that referenced this pull request Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android Issues or feature requests specific to the Android platform. in progress This issue or feature is currently being worked on by someone. iOS Issues or feature requests specific to the iOS platform. linux Issues or feature requests specific to the Linux platform. macOS Issues or feature requests specific to the macOS platform. web Issues or feature requests specific to the Web platform. windows Issues or feature requests specific to the Windows platform.
Projects
None yet
3 participants