search

singerdmx / flutter-quill

Rich text editor for Flutter
https://pub.dev/packages/flutter_quill
MIT License
2.6k stars 840 forks source link

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

Closed EchoEllet closed 1 month ago

EchoEllet commented 2 months ago

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](https://github.com/user-attachments/assets/adf52ac9-6118-43ce-9602-3bf083c22415)

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.

Related Issues

Type of Change

TODOS

Support for Windows is very limited and highly experimental.

EchoEllet commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

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

@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 commented 2 months ago

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 commented 2 months ago

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.

AtlasAutocode commented 2 months ago

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 \ tag but it looks like your code would not have a problem. My guess is that there might be other (undocumented?) meta tags before \ but I don't know how to handle that if you also have to cope with possible missing \ tag. Perhaps you could scan the lines, stop at the \ 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 \ tag is not found then would it be processed as html?

Do you have to cope with \ (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.

EchoEllet commented 2 months ago

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 (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.

AtlasAutocode commented 2 months ago

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 \ 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.

EchoEllet commented 2 months ago

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 commented 2 months ago

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: ```shell $ 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](https://github.com/user-attachments/assets/2f200fa7-d629-41fa-aaba-c61f9de2d52d) ![image](https://github.com/user-attachments/assets/84b9a16b-7c21-4327-84e2-59c5481f124a) Similar issue on the web: ![image](https://github.com/user-attachments/assets/5b750960-46d9-4e60-8443-11e607d06274) @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](https://github.com/singerdmx/flutter-quill/discussions/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](https://github.com/singerdmx/flutter-quill/pull/2230#issuecomment-2360727337).

If the input was , 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.

AtlasAutocode commented 2 months ago

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.

EchoEllet commented 2 months ago

@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 commented 1 month ago

@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`): ```dart @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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

AtlasAutocode commented 1 month ago

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 commented 1 month ago

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.

EchoEllet commented 1 month ago

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 commented 1 month ago

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`

```dart /// 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 commented 1 month ago

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 commented 1 month ago

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

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 commented 1 month ago

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:

The pasteText():

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

EchoEllet commented 1 month ago

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:

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.