feat: soft keyboard events

[vishna]

Description

Provides space/character event emulation on platforms that are not equipped with hardware keyboard (e.g. Android/iOS).

Emulation is based on diff between delta, so it happens after the change is committed to the document rather than on key press.

Enable it by setting true to QuillEditorConfigurations.softKeyboardShortcutSupport (This disables hardware events)

There might be some issues I haven't foreseen/came across yet, but it seems to work fine with all the standard shortcuts.

Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2024-09-17.at.09.43.52.mp4

Related Issues

• Fix QuillEditorConfigurations.enableMarkdownStyleConversion not respected on mobile #2214

Type of Change

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

[CatHood0]

See the changes that need to be into the code to avoid unexpected errors and new issues created by users that may be confused with how works this new functionality.

Note

And probably we will need to add a doc comment that tells to the devs that this feature only works when platform is Ios or Android

[vishna]

@CatHood0 @EchoEllet thanks for the thorough review 🙇 applied all the remarks, let me know if I can make it any better

[EchoEllet]

What I was trying to suggest is to move the isAndroidApp and isIosApp into it's own internal member with explanation why it's for iOS and Android

[vishna]

@EchoEllet @CatHood0 I've started wondering about one thing in my PR:

flutter-quill/lib/src/editor/raw_editor/config/events/soft_keyboard_shortcut_support.dart

Lines 37 to 38 in 27343d5

	final diffDelta = lastDelta!.diff(currentDelta);
	final keyPressed = _lastSingleChar(diffDelta);

Does delta diff gets more costly the bigger the doc is? Is there maybe a better way to detect this key press than doing a diff on the delta? Obviously key press events are not available on mobile but is there maybe some API of intercepting delta inserts? Could this maybe be more efficient this way? 🤔

[vishna]

@EchoEllet @CatHood0 I've removed delta diffing from my PR, it should make things faster 🤞 Now to detect if we're getting a new character:

• added a flag to differentiate if the replaceText is called by an input client
  

  flutter-quill/lib/src/controller/quill_controller.dart

  Lines 311 to 319 in e27ba2a

  	void replaceText(
  	int index,
  	int len,
  	Object? data,
  	TextSelection? textSelection, {
  	bool ignoreFocus = false,
  	bool shouldNotifyListeners = true,
  	bool isInputClient = false,
  	}) {

• checking if the incoming delta is single character insert
  

  flutter-quill/lib/src/controller/quill_controller.dart

  Lines 331 to 333 in e27ba2a

  	if (isInputClient && len == 0 && data is String && data.length == 1) {
  	isNewCharDelta = true;
  	}

• passing relevant delta to the logic I've written so far at the end of this method
  

  flutter-quill/lib/src/controller/quill_controller.dart

  Lines 388 to 395 in e27ba2a

  	if (delta != null &&
  	isNewCharDelta &&
  	_softKeyboardShortcutSupport != null &&
  	editorConfigurations.softKeyboardShortcutSupport) {
  	assert(QuillSoftKeyboardShortcutSupport.isSupported,
  	QuillSoftKeyboardShortcutSupport.assertMessage);
  	_softKeyboardShortcutSupport!.onNewChar(delta);
  	}

I probably should be able to make _softKeyboardShortcutSupport!.onNewChar static and get rid of the _softKeyboardShortcutSupport instance from QuillController

[EchoEllet]

Same question as previous comment.

[vishna]

forgot to update this one, will do

[AtlasAutocode]

I do not like these types of markdown conversions and soft key expansions.
However, I rarely use mobile and will not use markdown conversion.

I agree with @EchoEllet that there are enough questions about this topic, and its impact on our users, that this should at least be marked as experimental until it can be established that there are no unwanted side effects.

[EchoEllet]

I do not like these types of markdown conversions and soft key expansions.\ However, I rarely use mobile and will not use markdown conversion.

I do agree with this, too. we should provide an API that abstracts these kind of features into their own package or class. For example, we don't have something like imageEmbedSupport, instead, we provide an interface that accepts any kind of widget to be embedded in the editor.

For this example, we can't add all kinds of widgets directly in the editor and support them. Different users have different requirements.

Generally, LGTM, as long as it stays experimental for quite long (until the project is ready), so we can remove them at any time once we discover issues, I'm hoping that at some point, no longer add new features until we clean up the issues we currently have.

Building a stable project is more like a Lego game. You can't keep adding things while bot having a solid foundation. It will break at some point, and once it does, it will be more difficult to maintain later.

Keep in mind that this is not an issue with this PR. It's a project issue, and we're working on it.

[singerdmx]

@EchoEllet I will leave this PR to you regarding whether to merge or when to merge

[EchoEllet]

Can you explain why this change was introduced? If it's specific to this feature then it probably should be in a separate file including the new variable isNewCharDelta.

This is an example:

/// Paste event for the web.
///
/// Will be `null` for non-web platforms.
///
/// See: https://developer.mozilla.org/en-US/docs/Web/API/Element/paste_event
StreamSubscription? _webPasteEventSubscription;

extension QuillControllerWeb on QuillController {
  void initializeWebPasteEvent() {
    _webPasteEventSubscription =
        EventStreamProviders.pasteEvent.forTarget(window.document).listen((e) {
      // TODO: See if we can support markdown paste
      final html = e.clipboardData?.getData('text/html');
      if (html == null) {
        return;
      }
      // TODO: Temporarily disable the rich text pasting feature as a workaround
      //    due to issue https://github.com/singerdmx/flutter-quill/issues/2220
      // pasteHTML(html: html);
    });
  }

  void closeWebPasteEvent() {
    _webPasteEventSubscription?.cancel();
    _webPasteEventSubscription = null;
  }
}

We have introduced _webPasteEventSubscription in a separate file to not quill_controller.dart harder to read and understand.

[EchoEllet]

Same here, should be in debug mode only (the check itself and not what is inside it).

If you have details about the tree shaking and whatever if it's going to remove the check completely if all code inside it is stripped out, I would be open for feedback, but if we don't know much about it, we will address it as the docs of kDebugMode suggest:

/// A constant that is true if the application was compiled in debug mode.
///
/// More specifically, this is a constant that is true if the application was
/// not compiled with '-Ddart.vm.product=true' and '-Ddart.vm.profile=true'.
///
/// Since this is a const value, it can be used to indicate to the compiler that
/// a particular block of code will not be executed in debug mode, and hence
/// can be removed.
///
/// An alternative strategy is to use asserts, as in:
///
/// ```dart
/// assert(() {
///   // ...debug-only code here...
///   return true;
/// }());
/// ```
///
/// See also:
///
///  * [kReleaseMode], which is true in release builds.
///  * [kProfileMode], which is true in profile builds.
const bool kDebugMode = !kReleaseMode && !kProfileMode;

Can you move the check into its own file with a name explaining the change?

[EchoEllet]

Same as before

[EchoEllet]

This code seems to be related to a specific feature, shouldn't be here, if we add all requested features into the same file for a few years then it will be much harder to read and maintain.

[vishna]

It's specific for the post-insertion detection of the shortcuts which have the caret at a different position. I don't think there will be much more features added here over years. If this line can't be here, I'll need to copy paste the whole set of shortcut classes to achieve desired effect ...which is fine, I just wanted to avoid doing this.

[EchoEllet]

more features added here over years

Until now we're having more changes to different files when adding more features.

If someone removed all those files of this feature they will have no idea this change made here is related so should be in its own file with a clear name and docs.

[EchoEllet]

What does the isInputClient do? Can you explain it further or maybe provide some docs about it? And why this change was introduced? Is there a way to make it easier to be more clear that it's specific to this feature, so if a user delete soft_keyboard_shortcut_support.dart and related files, they will be required to address this change and remove it as well instead of keeping it is as. Maybe create a static getter in the internal file or something.

[vishna]

I use isInputClient to differentiate between replaceText called by RawEditorStateTextInputClientMixin.updateEditingValue and other actors. This allows me to avoid triggering shortcut code if e.g. space is inserted as a result of some other operation.

Once I know the data is coming from the input client, I check what is inserted. If it's a single character, I mark the flag and at the end of replaceText method I can trigger the shortcut code (e.g. apply ordered list formatting)

This is more efficient than what I was doing initially (which was diffing every delta + controller listener - the bigger the delta, the longer it takes to calculate the diff).

Maybe there could be some callback API for this 🤔

void onReplacedText(Delta diff, CallSource source); // called at the end of replaceText

This alone would probably allow me to implement this feature without any other major modifications to the package core.

[EchoEllet]

I use isInputClient to differentiate between replaceText called by RawEditorStateTextInputClientMixin.updateEditingValue and other actors. This allows me to avoid triggering shortcut code if e.g. space is inserted as a result of some other operation.

Once I know the data is coming from the input client, I check what is inserted. If it's a single character, I mark the flag and at the end of replaceText method I can trigger the shortcut code (e.g. apply ordered list formatting)

This is more efficient than what I was doing initially (which was diffing every delta + controller listener - the bigger the delta, the longer it takes to calculate the diff).

The main issue is that we're adding features that are highly specific. For example, we can't provide toolbar buttons to close the app, switch to full-screen mode, or share the document. Instead, we offer the option to allow users to add their own toolbar buttons and even customize the toolbar completely. Other buttons, like bold, italic, and search, are more general and commonly requested features.

Same thing for embedding widgets into the editor.

High-level features should not be included in low-level classes. To be more specific, such features should not be a direct part of the core package (at least in my opinion and that of others). Separation of concerns is important.

I might be wrong, which is why I suggested to mark those new APIs as @experimental, so we can correct them at any time without breaking changes but still not how I prefer to do it.

Maybe there could be some callback API for this

If you can provide a code example of how this will work, it would help clarify the change.

[EchoEllet]

Apologize for requesting too many changes many times but we will take code reviews more seriously than before from now on (at least try to). If you want, you can leave it to me and I will address those changes but will probably take longer than expected and I might ask you a few questions.

Thank you for your efforts on this PR.

[vishna]

Apologize for requesting too many changes many times but we will take code reviews more seriously than before from now on (at least try to). If you want, you can leave it to me and I will address those changes but will probably take longer than expected and I might ask you a few questions.

Thank you for your efforts on this PR.

No worries, my knowledge of the library internals is limited so I learn as I poke things around. I appreciate all the feedback!

[vishna]

@EchoEllet

• refactored everything soft keyboard related to be in a separate folder -> soft_keyboard_shortcut_support (some unavoidable code duplication)
• added the aforementioned ReplacedTextCallback support to QuillController (this is minor breaking change, unsure if this can be done any smarter)
• adapted quill_screen.dart to the refactored code

Let me know what you think and where it would be fine to store these soft_keyboard_shortcut_support classes. I might be the only person needing these so might not make sense to keep them in this library 😅

[vishna]

I made my team aware of the problem - we decided we can live without this soft keyboard feature. It would have been a nice UX addition to our app but it doesn't make sense to just fork everything in order to have it. That said, I'm closing this PR since there is no point in keeping it open and distracting authors of this package from solving more important issues.

[EchoEllet]

I appreciate your understanding, it's unfortunate that we couldn't merge this feature built-in or at least provide a way for users to implement their own custom changes seamlessly, let's assume we have access to native mobile keyboard events, would it require fewer changes to implement?

Looking forward to your future contributions.