singerdmx / flutter-quill

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

fix: replace flutter_keyboard_visibility with flutter_keyboard_visibility_temp_fork to support Flutter/Wasm target #2293

Closed EchoEllet closed 1 month ago

EchoEllet commented 1 month ago

Description

Replace flutter_keyboard_visibility with flutter_keyboard_visibility_temp_fork to support Flutter/Wasm target.

This is not a future-proof solution and we should replace the plugin, we might have our own solution in quill_native_bridge or develop a separate plugin.

Related Issues

Type of Change

EchoEllet commented 1 month ago

Was able to build the app for Flutter/WASM, however still having an issue running the example app:

Details

![image](https://github.com/user-attachments/assets/c4fbed14-cf41-4087-bcbf-4df2e7c5342a) Which seems to be related to [hydrated_bloc](https://pub.dev/packages/hydrated_bloc). See [bloc #4230](https://www.github.com/felangel/bloc/issues/4230) After removing `hydrated_bloc`: ![image](https://github.com/user-attachments/assets/5f648ffe-39be-4f28-8463-5909cc72e982) Seems like a simple issue that require minor changes.

Update:

image

Was able to fix those minor issues and seems to be running without any noticeable issues with Flutter/Wasm target on the web.

EchoEllet commented 1 month ago

It looks like kIsWeb is true on Flutter/Wasm, and this conditional import uses web/quill_controller_web_stub.dart:

import 'web/quill_controller_web_stub.dart'
    if (dart.library.html) 'web/quill_controller_web_real.dart';

So our code expects to use quill_controller_web_real.dart due to the kIsWeb check when the conditional import is invalid for Flutter/WASM which uses quill_controller_web_stub.dart that throws exception.

The alternative of dart.library.html is dart.library.js_interop. Since we don't need quill_controller_web_real.dart unless #2220 (see 1998) is fixed, commented the code and removed it completely from being used, left a TODO with reference so other developers know why this is still not removed, I will address as soon as I can. We might also consider moving this functionality to quill_native_bridge_web.

This comment will be updated soon.

EchoEllet commented 1 month ago

Is it possible to remove 'temp'?

flutter_keyboard_visibility_temp_fork is the name of the package and doesn't reflect the package itself, I could have named it flutter_keyboard_visibility_experimental though it doesn't necessarily mean it's experimental, the name is not a reflection of functionality. Chose to name it flutter_keyboard_visibility_temp_fork to make it clear enough to users that the support of this package will be dropped once we find a replacement in flutter_quill. If there is a replacement or update, we will deprecate the package and update flutter_quill.

Already updated the README.md of this fork with the details so you might want to see it. It doesn't have many changes except fixing those 2 critical issues.

I immediately worry that the main package is not stable, is going to have big breaking changes

This is why I suggested that we should be more cautious in the future when adding packages, especially plugins, the original package hasn't been updated in pub.dev for 2 years and on GitHub for 9 months, there were minor changes since those last 2 years, you will notice that they still didn't migrated to flutter_lints from pedantic which was last published before 3 years. See their commit history.

Why I'm not worried about this?

- I do have access to `flutter_keyboard_visibility_temp_fork` and should be able to publish any necessary changes - Since this functionality is usually only expected for mobile, it should be easier to implement since I'm already familiar with both Kotlin and Java (from Android, XML, and Java world), (not really with Swift though it's already straightforward). For the web, most of us are all familiar with Javascript/Typescript (though I haven't really used a production project using JS for the last few years). Should be easy enough to use [web](https://pub.dev/packages/web) - It's most likely that `flutter_keyboard_visibility` won't get updated anytime soon (even if it will, we should not expect it). Addressing those issues is critical since users are often worried when a new technology in the framework doesn't work with a dependency or package like Flutter Quill even if they don't use it, they will shift to other alternatives. I'm still in the middle of #2230 so I can't develop a replacement for `flutter_keyboard_visibility`, we haven't even discussed if this functionality should be in `quill_native_bridge` or in a separate package `quill_keyboard_visibility`.

but it sets up subliminal red flags.

I agree though I don't see any risk from this PR, we only use this plugin from one place in raw_editor_state.dart, and even if we decide to use another package or major breaking changes are introduced, updating the related code is quite minimal.

fdwl commented 1 month ago

The project failed to compile due to a conflict in dependencies. Here's the translated and elaborated error message:

What went wrong:
Execution failed for task ':app:mergeLibDexBerrytraceDebug'.
> A failure occurred while executing com.android.build.gradle.internal.tasks.DexMergingTaskDelegate
   > There was a failure while executing work items
      > A failure occurred while executing com.android.build.gradle.internal.tasks.DexMergingWorkAction
         > com.android.builder.dexing.DexArchiveMergerException: Error while merging dex archives: 
           Type com.jrai.flutter_keyboard_visibility.BuildConfig is defined multiple times: 
           /Volumes/noah/work/babify_app/build/flutter_keyboard_visibility/.transforms/80052641c9a712d2d3284697f65c1e3f/transformed/classes/classes.dex, 
           /Volumes/noah/work/babify_app/build/flutter_keyboard_visibility_temp_fork/.transforms/f307c2e859924a658e4ca8c7d61427e1/transformed/classes/classes.dex
           Learn how to resolve the issue at https://developer.android.com/studio/build/dependencies#duplicate_classes.

This error indicates that there are duplicate class definitions in your project. Specifically, the BuildConfig class from the flutter_keyboard_visibility package is defined multiple times. This is likely because you have two versions of the same package in your project:

  1. The original flutter_keyboard_visibility package

  2. A temporary fork of the same package, possibly named flutter_keyboard_visibility_temp_fork

radvansky-tomas commented 1 month ago

Have same issue with dex build, any update on this ?

effmuhammad commented 1 month ago

I also have same issue with dex build, any update?

EchoEllet commented 1 week ago

Have same issue with dex build, any update on this ? I also have same issue with dex build, any update? The project failed to compile due to a conflict in dependencies. Here's the translated and elaborated error message

Fixed in newer versions of flutter_keyboard_visibility_temp_fork (see #2346). Run flutter pub upgrade.