flutter-quill icon indicating copy to clipboard operation
flutter-quill copied to clipboard

feat(quill_native_bridge): Add default native implementation of `ClipboardService` for Android, Linux, macOS, iOS, Windows and Web

Open EchoEllet opened this issue 1 year ago • 25 comments

Description

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

The rich text pasting feature is now working without super_clipboard using only flutter_quill without any external dependencies, no need for installing Rust or anything extra, bridges that could make the app a bit slower to start initially.

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

https://github.com/user-attachments/assets/1cfd86b5-d74e-439f-9f84-716803475ae9

https://github.com/user-attachments/assets/5cc4ccbb-2f12-49be-80bd-16c85c84bb53

https://github.com/user-attachments/assets/dd178190-d5f3-47e5-8105-26c42426aaaa

Manually tested on both real device and emulator/simulator.

The android directory has been added using flutter create -t plugin --platforms android --org dev.flutterquill . for quill_native_bridge for the Android implementation, however, this command also make/revert some other changes to the ios directory which are not related, I removed them previously.

Same thing for macOS.

Related Issues

Type of Change

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

TODOS

  • [x] ~~Cleanup the code for native platforms~~ Done only for Kotlin and Swift implementations, web, Linux, and Windows still need more cleanup.
  • [x] 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.
  • [x] Retrieving HTML from the Clipboard on Android, iOS, and macOS.
  • [x] Copying an image to the Clipboard on Android, iOS, and macOS.
  • [x] Retrieving images from the Clipboard on Android, iOS, and macOS.
  • [x] ~~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.
  • [x] ~~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 #1788.~~ Should be separated, gif is not supported on desktop and web.
  • [x] ~~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.
  • [x] Configure the flutter_quill example AndroidManifest.xml to support copying images to the clipboard.
  • [x] ~~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.
  • [x] ~~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.
  • [x] 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
  • [x] ~~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.
  • [x] ~~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.
  • [x] Simplify ClipboardService
  • [ ] Fix issue #2220 for web
  • [ ] ~~Might update #1722 only to fix the bug for Safari instead, discuss the change first.~~ Seems to be unrelated.
  • [x] ~~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 #1998
  • [x] ~~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.
  • [x] ~~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
  • [x] ~~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.
  • [x] ~~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.
  • [x] 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.
  • [x] ~~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 take this into consideration 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.
  • [x] Separate the plugin platform interface from quill_native_bridge
  • [x] 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.
  • [x] 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.
  • [x] ~~Separate Android and Apple (iOS, macOS) implementation into their own package.~~ At first decided to have macOS and iOS in one package quill_native_bridge_apple.
  • [x] Use pigeon for Kotlin and Swift.
  • [x] ~~Update flutter_lints to latest version 5.0.0 and might update the lints for quill_native_bridge~~ Addressed in #2292
  • [ ] Test copyImageToClipboard and getClipboardImage with common image formats on all platforms.
  • [ ] Once getClipboardFiles supports web, avoid using dart:io in DefaultClipboardService for getHtmlFile and getMarkdownFile methods (solve the TODO, currently return null for web).
  • [ ] Solve TODOs in the ClipboardService of flutter_quill

Support for Windows is very limited and highly experimental, likely to have issues. Support for Linux is still under consideration and might be in a separate PR once most of the known issues are solved.

EchoEllet avatar Sep 15 '24 14:09 EchoEllet

While trying to add support for onGifPaste (introduced in #1788) and it seems to not be a feature that's supported natively on macOS. Copying GIF image using Google Chrome on macOS and then printing the available pasteboard types:

[__C.NSPasteboardType(_rawValue: public.png), __C.NSPasteboardType(_rawValue: Apple PNG pasteboard type), __C.NSPasteboardType(_rawValue: public.html), __C.NSPasteboardType(_rawValue: Apple HTML pasteboard type), __C.NSPasteboardType(_rawValue: org.chromium.source-url), __C.NSPasteboardType(_rawValue: public.tiff), __C.NSPasteboardType(_rawValue: NeXT TIFF v4.0 pasteboard type)

image

Doesn't seem to be supported by super_clipboard either.

EchoEllet avatar Sep 19 '24 11:09 EchoEllet

I will probably separate the implementation into different packages for a more scalable solution, I did want to have things as simple as possible (not necessarily simple, only as simple as possible and as complex as needed).

The Windows implementation will probably require conditional imports (win32, ffi) and probably will be in Dart code. For the web, there is a lint warning that need to be ignored (either from analysis_options.yaml or ignore: avoid_web_libraries_in_flutter for each file), for Android impl there are warning and error handling that are only Android side and will not be used on the other platforms (will not separate the Android impl into quill_native_bridge_android due to this reason, will keep it in quill_native_bridge). If someone wants to provide their own implementation then we also need to provide them quill_native_bridge_platform_interface and will be required for quill_native_bridge_web and quill_native_bridge_web.

This will complicate the usage for the users since they can still use quill_native_bridge directly.

Update: I decided not to, since the CI, the version system, and the scripts are not ready to handle too many packages while remaining maintainable.

EchoEllet avatar Sep 19 '24 14:09 EchoEllet

Renaming the plugin from quill_native_bridge to quill_native_integration can take some time and is error-prone process, so decided to delay it later, as for the web package, instead of creating quill_native_bridge_platform_interface I decided to only have quill_native_bridge_web, quill_native_bridge_windows for now. For Linux still haven't decided if the implementation will be in Dart or platform native code.

Those packages will depend on quill_native_bridge.

EchoEllet avatar Sep 21 '24 17:09 EchoEllet

I decided to move quill_native_bridge outside this repo for the following reasons:

  • The version is now separated, when a breaking change is introduced in flutter quill it doesn't affect quill_native_bridge and vice versa. Also, we may want to publish a pre-release of one package and not the other.
  • Integration tests are time-consuming and expensive since they will run in the example on a real device, I only wrote a few simple tests for quill_native_bridge, since this is a plugin, it will run on all platforms in CI, including older versions of Android to ensure compatibility as some of the logic work slightly different based on the Android API. The rest doesn't require this but still needs to be tested on various versions or at least the latest version. Most of the time we don't work on quill_native_bridge so running each commit will run the check.
  • Separate the issues, we currently have a high amount of issues, I have closed around 50-100 issues in 2023 only because they are outdated (which shouldn't be done) and we still have a lot of issues, even if not reported, not all users do report them. So we don't want to have more issue reports directly in here.
  • A separate example and it has its own packages and level of complexity that we don't need here. We only need to know that QuillNativeBridge.getClipboardHTML returns HTML from the clipboard when available here. We do need to test the changes on the example we have for Flutter Quill but if an issue happened and didn't in that example, it's a usage or Flutter Quill issue and should be fixed separately.
  • Conflicting with CI, quill_native_bridge has different requirements, and for unknown reasons, the workflow used the latest version of quill_native_bridge (that's published and hosted on pub.dev only for a reason) when the latest version is 10.5.4 and is used in pubspec.yaml. Changes of this branch are still not merged so the example don't know about them, I'm still not sure why however it's better if we move it completely out of this repo to ensure such issues don't exist. See #2264 where the issue occurred. I wasn't able to have this issue previously when I first introduced a related change.

I didn't want to create a new repo in my GitHub account for the following reasons:

  • I do change my username from time to time
  • At some point, the new repo will be controlled by someone else, having it in my own personal account is not future-proof.
  • We're creating different repositories now, having all related repositories in one place is useful for organization.
  • It also can give the user a better experience.

@CatHood0 @AtlasAutocode

I sent you invites to @FlutterQuill in case you're interested. We have not decided if we will move this repo to the new organization. I wanted to create an organization though the FlutterQuill was already used, asked @singerdmx and it seems it's already created so decided to stick to it instead of choosing a different name. We might have more than 1 or 2 repositories in this organization on the long term but this is not a priority at the moment.

EchoEllet avatar Sep 22 '24 18:09 EchoEllet

While writing some basic integration tests, the tests were failing when I added copyHTMLToClipbaord, it seems I accidentally (using something with the IDE) changed something while adding this feature causing a minor bug on all platforms, was able to catch this bug while writing the test itself which prove we really need some tests at some point.

Writing tests for Flutter Quill is not as simple as here since this is a new feature designed and meant to be tested that I'm familiar with, making it much easier to write. More discussion regarding the test situation will be in a separate issue in the future.

EchoEllet avatar Sep 22 '24 20:09 EchoEllet

While introducing the Windows implementation of the plugin, I separated it from quill_native_bridge (quill_native_bridge_windows) and decided to implement it as a Dart plugin (in dart code) with ffi and win32. I attempted to use the win32_clipboard package for high-level clipboard access. However, I couldn't retrieve the HTML since it always cast the data to UTF-16 (see this line) and HTML on windows is always using UTF-8 encoding which result in a strange text that look like Chinese (doesn't have any known language). Refer to HTML Clipboard Format Description for more details.

As a result, I opted to work directly with the Win32 API for a low-level implementation, I'm unfamiliar with Windows Win32 and this is my first time using their API, tested it on Windows, and it seems to work fine.

I noticed the clipboard content is not available even after copying HTML when setting the clipboard mode to Bidirectional in Virtual Box, There may be additional steps needed to reproduce this issue on Windows, so this is a highly experimental implementation. I'm uncertain if opening the clipboard by a program prevent others from accessing the clipboard on Windows and is expected behavior.

@AtlasAutocode I recall you mentioned you have used Win32 before. If you have time, could you review the quill_native_bridge_windows implementation, focusing on the Win32-specific code? To check for stability and quality, and suggest any improvements. There’s no rush—if you need more time, we can separate the Windows support into a different PR or release it as highly experimental, without making it the default package in quill_native_bridge. We can also delay merging this PR and focus on other tasks.

EchoEllet avatar Sep 23 '24 14:09 EchoEllet

I looked through the win32 code for accessing the clipboard and did not see any obvious problems. Your code matches what I use in C++/win32 apps.

I am not as familiar with the dart interface but it looks like you are correctly using calloc.free to recover stack allocated memory allocations that could result in memory leaks. And I don't see any places where lock/unlock are mismatched or missing.

Converting between UTF8 and UTF16 will need testing but that should not be difficult if we create an HTML document with a couple of Unicode characters and make sure they come in correctly. Unit tests should be able to do this.

CloseClipboard is important to allow other windows/apps to access the clipboard. I've never found this necessary, but you could enclose in try-finally to ensure the clipboard is released even if an exception is thrown.

I might be concerned if the clipboard html did not include the <html> tag but it looks like your code would not have a problem. My guess is that there might be other (undocumented?) meta tags before <html> but I don't know how to handle that if you also have to cope with possible missing <html> tag. Perhaps you could scan the lines, stop at the <html> and then delete all preceding lines or copy all following lines? I don't know how the clipboard (or us?) decides that the input data is html - if the <html> tag is not found then would it be processed as html?

Do you have to cope with <HTML> (wrong case)? According to the HTML5 specs, html is not case sensitive and it is legal to use tags with different upper/lower case. I wish dart had a case insensitivity setting but I understand this is a problem for non-English languages.

Minor comments: RegisterClipboardFormat for HTML can be const/static. It is one of the small number of formats that are pre-defined in windows and does not ever change. As such, it should never be zero.

Other comments: EnumClipboardFormats and GetClipboardFormatName might be useful. I've not used them since mostly I am exporting data to the clipboard to be used in other apps.

AtlasAutocode avatar Sep 23 '24 16:09 AtlasAutocode

could enclose in try-finally to ensure the clipboard is released even if an exception is thrown.

I considered this but it seems it doesn't throw any exception, I checked it and it seems that the error will be handled using GetLastError() or a similar function, of course, the current code is not scalable enough to have more features, and should be more organized which is I put 4 TODOs to confirm this, to make sure we don't forgot about the issue and then merge directly while the issue is here somewhere in GitHub.

My guess is that there might be other

There is a comment but it's valid and should be ignored, I already pasted the HTML in Quill JS Playground, and seems correct, though haven't in the Flutter Quill example since I'm using VirtualBox in Linux distro and I'm having a build failure related to Rust and Cargo, it seems that installing Rust on windows sometimes can't be automated, haven't tried further since I already have others tasks.

stop at the and then delete all preceding lines or copy all following lines

The code currently will remove all those lines related to description headers if they exist, once we reach the ` tag or there are no longer description headers it will break the loop and then return the HTML directly. I have added some minor tests and it seems that some HTML parsers do throw errors when there are such comments that are not valid HTML, and some do try to parse it anyway, causing unexpected behavior or adding those comments somewhere inside the HTML.

Do you have to cope with <HTML> (wrong case)? According to the HTML5 specs, HTML is not case-sensitive and it is legal to

If you mean this part:

 bool isHTML(String str) {
          final htmlRegExp =
              RegExp('<[^>]*>', multiLine: true, caseSensitive: false);
          return htmlRegExp.hasMatch(str) && str.startsWith('<');
        }

        expect(isHTML(clipboardHtml), true);
        expect(isHTML('Invalid<html></html>'), false);

Then it's already not case sensitive, it will run as an integration test to ensure Windows and all other platforms implementations do return HTML that can be parsed and doesn't include non-HTML content/comment as leading. I didn't see a need for this test until I saw this.

RegisterClipboardFormat for HTML can be const/static

I'm not sure what you meant, do you mean the format ID itself can be static/const:

extension type const ClipboardFormatExt(int formatId) implements CLIPBOARD_FORMAT {
  static const CF_HTML = 49320;
}

Or:

static const kHtmlFormatId = 49320;

If that's the case, then why RegisterClipboardFormat is needed to be called? Does it only return the ID or register that format and then return the ID? The win32 package docs:

/// Registers a new clipboard format. This format can then be used as a
/// valid clipboard format.
///
/// ```c
/// UINT RegisterClipboardFormatW(
///   LPCWSTR lpszFormat
/// );
/// ```
/// {@category user32}
int RegisterClipboardFormat(Pointer<Utf16> lpszFormat) =>
    _RegisterClipboardFormat(lpszFormat);

final _RegisterClipboardFormat = _user32.lookupFunction<
    Uint32 Function(Pointer<Utf16> lpszFormat),
    int Function(Pointer<Utf16> lpszFormat)>('RegisterClipboardFormatW');

EnumClipboardFormats and GetClipboardFormatName might be useful.

Thank you for mentioning them, I will see where they might be used.

In general, I'm okay with implementing the core clipboard operations that we need in quill_native_bridge_windows though I will most likely need your review and help on any related changes, prefer to not release something that I just used as stable to the public. This is my very first time using Win32 and I don't remember the last time I used C++. It doesn't have to be ASAP, the one feature that I do plan at the moment other than clipboard is the spell checking which is supported only on Android, iOS, and macOS which require less effort to implement.

EchoEllet avatar Sep 23 '24 17:09 EchoEllet

Do you have to cope with (wrong case)? According to the HTML5 specs, HTML is not case-sensitive

I was referring to the code that uses <html> lower case:

for (final line in lines) {
    // Stop processing when reaching the start of actual HTML content
    if (line.startsWith('<html>')) {
      break;
    }

why RegisterClipboardFormat is needed to be called?

It does need to be registered but registration only needs to be done once when the format is first needed. No need to call it every time you access the clipboard. From the docs, once a format is registered with the computer, any subsequent access from this or any other app will return the same number. This is so that different apps can exchange data in the specific format. When you restart your app, you again need to register the format to be able to access that format on the clipboard. When windows restarts, its current table of registered formats is cleared. Trivia: for HTML the returned registration ID is the same for all computers. For other formats the number can/will be different on different computers.

I use a global (late static final?): extern "C" UINT CF_REG_HTML = ::RegisterClipboardFormat ( _T("HTML Format") ); and then just use CF_REG_HTML whenever needed. OpenClipboard is not needed to register, and I have never seen it fail for HTML.

AtlasAutocode avatar Sep 23 '24 19:09 AtlasAutocode

I was referring to the code that uses lower case:

Good catch, should not be case sensitive. If the input was <HTML>, then this loop will continue until the end of the HTML. Will add one more test to cover this case.

late static final

I will register it when first calling this method instead of everytime starting the app every time since the user might never use this feature.

EchoEllet avatar Sep 23 '24 20:09 EchoEllet

Currently, the experimental Linux implementation is using xclip which is designed for x11. Will separate them first then will support Wayland with wl-clipboard.

Was not able to support macOS since not natively supported same as iOS.

Support for Gif on Linux

image/gif doesn't seem to be natively supported on Linux clipboard either:


$ xclip -o -t TARGETS
TIMESTAMP
TARGETS
MULTIPLE
text/html
text/_moz_htmlinfo
text/_moz_htmlcontext
image/png
image/jpeg
image/bmp
image/x-bmp
image/x-MS-bmp
image/x-icon
image/x-ico
image/x-win-bitmap
image/vnd.microsoft.icon
application/ico
image/ico
image/icon
text/ico
image/tiff

There might be something I'm not aware of, a workaround or a slightly different way to retrieve GIF images from the clipboard on Linux and macOS.

However, it seems to be an issue on other apps as well, copying an image on Android/iOS from the browser and pasting it into an app (e.g. Slack, Telegram) will paste it as a GIF image, macOS/Linux will paste it as image/png:

image

image

Similar issue on the web:

image

@AtlasAutocode If you can confirm this issue on Windows, then we might consider documenting the onGifPaste to indicate it's supported only on mobile, we want to fix clipboard, and pasting issues completely and allow the users to customize anything they want. So will not introduce any change until now other than updating to docs of onGifPaste. We have slightly discussed this in #2270.

As far as I tested, I can confirm this is an issue on the Web, Linux, and macOS regardless of the application. It's also not supported on other clipboard plugins nor on native apps, see this comment.

If the input was <HTML>, then this loop will continue until the end of the HTML.

Will fix this issue soon.

Was already able to catch some bugs with integration tests while implementing QuillNativeBridgeLinux.

Update: I introduced a regression that broke the Linux implementation completely while testing even before publishing the package as experimental, the integration tests really helped fix this bug quickly.

EchoEllet avatar Sep 24 '24 12:09 EchoEllet

Just as a comment:

Support for Gif on Linux

Gif is not common on Windows. Windows clipboard primarily uses BMP and vector metafiles (wmf, emf). PNG, TIFF or JPEG for files.

Using the win32 dart package would enable access to the windows clipboard formats but then we would need a way of viewing or converting.

I think clipboard image support is going to be very platform dependent. My own interest is in cross platform support where the same shared document file can be viewed on different platforms. Images cannot be saved as temporary files.

AtlasAutocode avatar Sep 24 '24 14:09 AtlasAutocode

@CatHood0 If you have the time, can you test the example of quill_native_bridge or flutter_quill on your Linux distro to see if it works properly? It should work on most major Linux distros except those who use Wayland. Currently, the implementation only supports X11, and Wayland's support will be soon.

git fetch origin
git checkout feat/default-clipboard-native-impl
git pull origin feat/default-clipboard-native-impl --rebase

Run the quill_native_bridge example using:

(cd quill_native_bridge/quill_native_bridge/example/ && flutter run -d linux)

Or flutter_quill example:

(cd example/ && flutter run -d linux)

If we can complete the support for Windows (even if highly experimental), we can remove super_clipboard without worrying about having to move it into a separate package or publish a major release due to breaking changes in flutter_quill_extensions.

As for now, we will need to improve the code quality and readability before adding more features.

EchoEllet avatar Sep 24 '24 15:09 EchoEllet

@AtlasAutocode

When you have the time, can you review copyHtmlToClipboard()? I'm unfamiliar with Win32 and this is actually the first time most of the things I done here either which is why I shifted Windows support to be the last, I have used Windows Docs and win32_clipboard as a reference and I can see issues, especially quality issues, with this implementation, it could really use improvements, ignore the usage of assert as I will throw exceptions though that will be later once address some things.

The naming could really use improvements, following naming conventions to be more clear

I'm not sure about the conventions when using Dart FFI with Win32, since this Dart project should at least make it easier for developers who are not familiar with other languages or make it clearer, see this example in Kotlin with Android (quill_native_bridge_android):

@ConfigurePigeon(PigeonOptions(
  dartOut: 'lib/src/messages.g.dart',
  // Using `GeneratedMessages.kt` instead of `Messages.g.kt` to follow
  // Kotlin conventions: https://kotlinlang.org/docs/coding-conventions.html#source-file-names
  kotlinOut:
      'android/src/main/kotlin/dev/flutterquill/quill_native_bridge/generated/GeneratedMessages.kt',
  dartPackageName: 'quill_native_bridge_android',
))

You can see this commit. This is not final yet though I'm looking forward to your feedback as you're already familiar with Win32.

Update: According to Win32 Docs SetClipboardData(), it expects a handle and not a direct pointer returned by GlobalLock. Updated though I still expect more issues.

copyHtmlToClipboard() is currently not used in flutter_quill and we have no plans of using it until now however I have found it useful in integration tests and that's the main reason I'm implementing it, I don't plan on implementing anything related to windows other than copyImageToClipboard and getClipboardImage so once things are more stable and organized, we won't introduce many changes as we don't need to (at least not for Windows). Those are already used in integration tests and I depend on tests (in addition to manual tests) to ensure to not introduce regressions. Also, it can be considered a breaking change if we release a new version that uses quill_native_bridge that lacks Windows support when it was supported using super_clipboard which I prefer not to introduce a new major release to remove super_clipboard and ask users to add a new package for super_clipboard to migrate as we have done many breaking changes.

EchoEllet avatar Sep 28 '24 22:09 EchoEllet

All locked memory allocations must be unlocked before their data handle is used.

I highly doubted there was an issue since I didn't clearly read the docs so left a TODO with a reference, ~~is what they meant that we should not call GlobalFree before or after SetClipboardData since the system will take ownership of the memory as it expects the GLOBAL_ALLOC_FLAGS.GMEM_MOVEABLE?~~

fails, then call GlobalFree to release the allocated memory. If it succeeds, then the clipboard owns the data and it should not be freed.

Thank you for clarifying, this answer to my previous question. Can you review this commit addressing those issues with minor change? I'm uncertain if we should GlobalUnlock the clipboardMemoryHandle or lockedMemoryPointer. I have looked into some Flutter plugins that use the win32 package with ffi and I have seen similar issues even though the code works without any errors, runtime, or compilation issues which is common in low-level languages and unexpected in languages like Dart.

EchoEllet avatar Sep 29 '24 14:09 EchoEllet

According to GlobalUnlock function Windows docs:

Note The global functions have greater overhead and provide fewer features than other memory management functions. New applications should use the heap functions unless documentation states that a global function should be used. For more information, see Global and Local Functions.

Should we use those new heap functions instead? Since we don't have complex requirements that need low-level access, in general, I'm looking to use the most up-to-date methods since Flutter desktop supports Windows 10 and newer versions, you might noticed that I used version 1.0 instead of 0.9 for the Windows HTML Clipboard Description headers in constructWindowsHtmlDescriptionHeaders()

EchoEllet avatar Sep 29 '24 14:09 EchoEllet

Updated in this commit. Are there any changes or improvements specific to accessing the Win32 API or related memory management issues?

The pointer is for transient use while locked.

I was thinking about having an internal utility function for locking the handle and getting access to the pointer inside a function so that we can't accidentally access the pointer and pass it to GlobalLock.

EchoEllet avatar Sep 29 '24 14:09 EchoEllet

The global functions have greater overhead

Yes, that is true but not relevant to your use case.

You MUST use global functions since you are transferring data from your app to the global windows environment (aka the clipboard). This also ensures that the data is available even after you close the source app.

For apps that use data internally, you would not use global functions. Local or heap-based allocations are faster but only accessible from within the app.

used version 1.0 instead of 0.9 for the Windows HTML Clipboard Description headers

You might want to be careful here. It sounds like a good idea but might have serious unintended consequences. There is a lot of antique inherited code dependencies. win32 is still a main windows interface for desktop. But Microsoft has been trying to eliminate it with .Net and UWP (not successfully!). The advantage of win32 (notice there is no win64!) is that it provides low level access to all windows operating system functions and is not going away for many years. FYI: Microsoft is trying to transition all users to cloud. You will be able to run windows apps from any computer (mac, linux, mobile) by running it on a cloud-based interface.

AtlasAutocode avatar Sep 29 '24 15:09 AtlasAutocode

internal utility function

I think this is more likely to cause a failure to unlock, but this is your call. I generally try to keep it simple:

processingFunction ( GlobalLock(handle) );
GlobalUnlock(handle);

.net programming has a very useful function

using ( var pointer = accessFn() ) {
  do whatever with pointer.
}

Within the block you can use the pointer and when the block ends it automatically releases/disposes of what was done in the using clause.

AtlasAutocode avatar Sep 29 '24 15:09 AtlasAutocode

might have serious unintended consequences.

Sure though this is usually when having compatibility issues with older versions, most apps already support version 1.0 and the Flutter team has already dropped the support for Windows 7. So there is probably not much to lose or have when using new version, IMO using new methods should not be on the account of having a less backward compatible product, this is an example on Android though this Windows case is different.

Microsoft has been trying to

If you want my personal opinion, Windows itself as an operating system is no longer as functional as it's used to be in Windows 7 and older versions, full of bugs and issues, and not as performant as before in even if you have the best hardware. Those new apps such as the Microsoft Store are commonly known to be buggy, though this is another topic that's not quite related to our discussion.

I think this is more likely to cause a failure to unlock,

This is why I avoided it, it needs more testing and has less control, there are probably other issues that I'm not aware of.

it automatically releases/disposes of what was done in the using clause.

Similar to the use block in Kotlin which is also compatible with JVM.

EchoEllet avatar Sep 29 '24 15:09 EchoEllet

Similar to HTML, we can't directly copy the input to Windows clipboard, which will cause the image to be invalid when pasting into other apps, not being pasted or missing. It seems that we need to use either either CF_DIBV5 (extended version of CF_DIB) or CF_DIB. Supporting those formats require more testing, conversation, and search about this, assuming that the image we're copying is PNG, then we need to decode it and encode it to Windows BMP, which is what I did and it still not copying properly, and it also require image as a dependency, adding new dependencies to the Flutter Quill project is something I highly prefer not to unless it's needed like win32.

According to the Win32 (which I need to read more about it), it looks like we need some additional work, maybe some headers similar to HTML, I have looked into BITMAPINFOHEADER and BITMAPV5HEADER and it seems that currently only definition of BITMAPINFOHEADER is available in win32.

It should be possible to define `BITMAPV5HEADER`

/// From [Win32 BITMAPV5HEADER docs](https://learn.microsoft.com/en-us/windows/win32/api/wingdi/ns-wingdi-bitmapv5header#syntax)
/// Since it's not declared in [win32](https://pub.dev/packages/win32)
base class BITMAPV5HEADER extends Struct {
  @Uint32()
  external int bV5Size;
  @Int32()
  external int bV5Width;
  @Int32()
  external int bV5Height;
  @Uint16()
  external int bV5Planes;
  @Uint16()
  external int bV5BitCount;
  @Uint32()
  external int bV5Compression;
  @Uint32()
  external int bV5SizeImage;
  @Int32()
  external int bV5XPelsPerMeter;
  @Int32()
  external int bV5YPelsPerMeter;
  @Uint32()
  external int bV5ClrUsed;
  @Uint32()
  external int bV5ClrImportant;
  @Uint32()
  external int bV5RedMask;
  @Uint32()
  external int bV5GreenMask;
  @Uint32()
  external int bV5BlueMask;
  @Uint32()
  external int bV5AlphaMask;
  @Uint32()
  external int bV5CSType;
  @Uint32()
  external int bV5Endpoints;
  @Uint32()
  external int bV5GammaRed;
  @Uint32()
  external int bV5GammaGreen;
  @Uint32()
  external int bV5GammaBlue;
  @Uint32()
  external int bV5Intent;
  @Uint32()
  external int bV5ProfileData;
  @Uint32()
  external int bV5ProfileSize;
  @Uint32()
  external int bV5Reserved;
}

See the Windows Standard Clipboard Formats.

As I'm unfamiliar with those and new to Win32 (once again, this is my first time to clarify), I don't want to introduce a feature that I just implemented for the first time for Windows as a stable feature, and I'm unable to support it due to the lack of time, also users still having build failure issues with super_clipboard, introducing a breaking change (not supporting those anymore when it was stable with super_clipboard) is still not an option yet so I have to use Process and copy the image using Windows Powershell, what we will use should be available in Windows 7 and newer version, shouldn't cause compatibility issues, not really the most efficient way though it should work for now.

EchoEllet avatar Sep 30 '24 13:09 EchoEllet

Doesn't seem to be a good solution:

Future<void> copyImageToClipboard(Uint8List imageBytes) async {
    final tempClipboardImageFileName =
        'tempClipboardImage-${DateTime.now().millisecondsSinceEpoch}.png';
    final tempImageFile =
        await File(generateTempFilePath(tempClipboardImageFileName))
            .create(recursive: true);
    try {
      await tempImageFile.writeAsBytes(imageBytes);
      final powerShellScript = '''
Add-Type -AssemblyName System.Windows.Forms
Add-Type -AssemblyName System.Drawing

\$image = [System.Drawing.Image]::FromFile("${tempImageFile.path}")

[System.Windows.Forms.Clipboard]::SetImage(\$image)

\$image.Dispose()
''';
      final result = await Process.run(
        'powershell.exe',
        ['-Command', powerShellScript],
      );
      final exitCode = result.exitCode;
      if (exitCode != 0) {
        final errorOutput = result.stderr;
        // Handle output error
      }
    } on ProcessException catch (e) {
      // Handle process error
    } finally {
      await tempImageFile.delete();
    }
  }

It seem that it load System.Windows.Forms and System.Drawing which might not meant for fast execution or use in application.

Running PowerShell commands and scripts seems to be noticeably slower, it can take up to 0.5 to 3 seconds to copy the image, which feels like sending a network request rather than copying the small and simple image, I'm running Windows as Virtual Machine though in general PowerShell is slower to handle those operations in an application regardless of how I'm running Windows or how running this process can be improved. I tried running the PowerShell script directly using Windows, and I haven't noticed much of a difference.

Maybe there is another way.

Update: ~~Managed to get it working with Win32 by reading a BMP file, still far from efficient.~~ It only copy and paste properly when that BMP file is downloaded somewhere or is actually a valid bitmap file, converting PNG/jpeg and other formats to BMP using image won't solve the issue, I assume Windows expect some headers or info attached with that image similar to HTML.

EchoEllet avatar Sep 30 '24 15:09 EchoEllet

I'm thinking about splitting the features of the quill_native_bridge package into different packages, so having:

  • quill_clipboard and related packages such as quill_clipboard_interface, quill_clipboard_android and the others.
  • quill_spell_checker and related packages such as quill_spell_checker_platform_interface, quill_spell_checker_android and the others.
  • other packages such as keyboard events (not available yet)

Each one will be in its own repo, this will make, in general, the project more scalable since we no longer need the clipboard word in the context of QuillClipboard (getHtmlText directly instead of getClipboardHtmlText), the error handling will be different and easier, and the platform check will also be different allowing us to have more control and being more specific about each feature, better compatibility, though do we want that level of complexity? Currently, we have 8 packages just for one package that contains native features. I will keep it simple for now and later we might want to more consideration this, since this package is for internal use only specifically for flutter_quill, this will make it easier for us to handle breaking changes.

EchoEllet avatar Oct 01 '24 14:10 EchoEllet

I'm trying to provide the option to customize the clipboard paste behavior, we have pasteText() in QuillRawEditorState and clipboardPaste() in QuillController.

Where should the option to customize the paste completely be provided?

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.

We need to know why those are separated into two different places.

We need a better place for providing an option for customizing the paste. We should choose either the QuillToolbar or QuillEditor. Also, we need to choose how we will add properties since we're adding things randomly (due to the lack of docs and code reviews), will they be in the configuration class or directly in the constructor? We can't fix them without breaking changes or many changes, only new properties will be added properly in an organized way for now, and then we will start to fix the previous parameters and move them to where they should be later in either breaking change or backward compatible, first, we need to agree on something, we can't keep moving things here and there randomly each time.

This is something that blocks providing the option to disable some of the features of this PR and I won't be able to merge without addressing this issue.

@singerdmx @AtlasAutocode @CatHood0 What do you think?

EchoEllet avatar Oct 01 '24 15:10 EchoEllet

I didn't mean to ask for a review too many times (I haven't noticed that). I will ask for a review less often.

I thought we agreed to not add new features.

This is a bug fix by introducing our own native clipboard API instead of using other plugins to solve common issues.

I was also trying to solve a related issue in the same PR.

I will split this package into its own repo (due to the requirement of running integration tests and other reasons) so minimal changes will be made on this repo. It will be moved soon to FlutterQuill/quill-native-bridge.

EchoEllet avatar Oct 01 '24 18:10 EchoEllet

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 avatar Oct 16 '24 14:10 EchoEllet

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

EchoEllet avatar Oct 16 '24 23:10 EchoEllet

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)?

EchoEllet avatar Oct 17 '24 10:10 EchoEllet

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.

AtlasAutocode avatar Oct 17 '24 18:10 AtlasAutocode

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 avatar Oct 18 '24 06:10 EchoEllet