Skip to content

Chore(breaking changes): Replace TextInputClient with DeltaTextInputClient #2510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

CatHood0
Copy link
Collaborator

@CatHood0 CatHood0 commented Mar 12, 2025

Description

The previous implementation of applying changes in the TextInputClient worked well and did the job that was expected of it, but it was not very easy to read (not everyone investigates what the Diff class is), and sometimes it was somewhat complicated to modify (at least in my case), since it depended a lot on what was obtained from the Diff object.

This change is actually quite simple.

Warning

I've marked this change as potentially breaking, as we'll likely need to modify the testing API to now use TextEditingDelta values ​​if this PR is to be merged.

Using the available TextEditingDelta classes

What we did was go from applying the changes directly to dividing the logic into different sections:

Was this change really necessary?

Yes, since this allows us to obtain more granular changes and avoids having to use a method like getDiff, which, in the long run (or for long documents), ruins the user experience quite a bit when having to compare large changes that occur in the editor.

We went from applying the Diff class directly to the replaceText, to instead obtaining specific classes for each use case/type of change made, which makes the logic for applying them more independent.

Some of the advantages of this change are:

  • Better code readability.
  • Better stability thanks of the granular information that TextEditingDelta gives to us.
  • It's more maintainable in the long term.
  • The logic for applying each change is now specific to each use case and is easier to modify.
  • Allows new contributors to enter the code and modify it without much fear of breaking something.

Related Issues

Type of Change

  • Feature: New functionality without breaking existing features.
  • 🛠️ Bug fix: Resolves an issue without altering current behavior.
  • 🧹 Refactor: Code reorganization, no behavior change.
  • Breaking: Alters existing functionality and requires updates.
  • 🧪 Tests: New or modified tests
  • 📝 Documentation: Updates or additions to documentation.
  • 🗑️ Chore: Routine tasks, or maintenance.
  • Build configuration change: Build/configuration changes.

TODO

  • check if works for all platforms (i'll need some help with apple OS)
  • fix buggy behavior on deletion operations in web browsers
  • fix buggy behavior in android (the caret is not synced with the TextEditingValue passed)

CatHood0 and others added 16 commits February 21, 2025 01:13
…ingerdmx#2483)

* docs: update controller length extension method deprecation message

* docs: update changelog
…ute (singerdmx#2438)

* feat: Enable BoxDecoration for DefaultTextBlockStyle of header Attribute

---------

Co-authored-by: Ellet <[email protected]>
… of page route changing when editor is set to readonly. (singerdmx#2488)

Co-authored-by: chaos <[email protected]>
@CatHood0 CatHood0 added enhancement New feature or request in progress This issue or feature is currently being worked on by someone. labels Mar 12, 2025
@CatHood0 CatHood0 self-assigned this Mar 12, 2025
@CatHood0 CatHood0 marked this pull request as draft March 12, 2025 00:42
@CatHood0 CatHood0 requested review from EchoEllet and singerdmx March 12, 2025 00:47
@CatHood0

This comment was marked as resolved.

@CatHood0

This comment was marked as off-topic.

@CatHood0
Copy link
Collaborator Author

CatHood0 commented Mar 12, 2025

Although it solves several problems for desktop and mobile devices, it seems to behave strangely on the web. I wonder what's causing it to behave this way, when in reality, we're doing exactly the same thing as before, but with a little extra code.

For example:

  • If we press shift+arrow up and then try to use the backspace key to remove the text in selection, it will only update the position (note that on the web, it's actually just sending a TextEditingDeltaNonTextUpdate instead of a TextEditingDeltaDeletion).
  • If I try to go right after performing the previous action, it will now completely ignore that event.
  • If I try to use the delete key specifically, instead of removing the text to the right, it will only delete it as if it were the backspace key.
issue_with-2025-03-11_22.42.09.mp4

Update: this part was solved by leaving the old implementation for the web (I'll need to make further changes to remove it and get the new one working properly). In any case, this is just a temporary solution.

@CatHood0

This comment was marked as resolved.

@CatHood0
Copy link
Collaborator Author

CatHood0 commented Mar 29, 2025

@EchoEllet What do you think about using DeltaTextInputClient instead of TextInputClient?

I commented this because it's outdated.
Although, considering plain text, it's certainly a disadvantage. But to solve it, we can implement TextInputClient and DeltaTextInputClient. This way, we'll keep the plain text value up to date and update the editor in a more granular way without having to search through long text strings.

No, we cannot have the TextInputClient and DeltaTextInputClient implemented in a same mixin, since flutter recommends that.

  /// Whether to enable that the engine sends text input updates to the
  /// framework as [TextEditingDelta]'s or as one [TextEditingValue].
  ///
  /// Enabling this flag results in granular text updates being received from the
  /// platform's text input control.
  ///
  /// When this is enabled:
  ///  * You must implement [DeltaTextInputClient] and not [TextInputClient] to
  ///    receive granular updates from the platform's text input.
  ///  * Platform text input updates will come through
  ///    [DeltaTextInputClient.updateEditingValueWithDeltas].
  ///  * If [TextInputClient] is implemented with this property enabled then
  ///    you will experience unexpected behavior as [TextInputClient] does not implement
  ///    a delta channel.
  ///
  /// When this is disabled:
  ///  * If [DeltaTextInputClient] is implemented then updates for the
  ///    editing state will continue to come through the
  ///    [DeltaTextInputClient.updateEditingValue] channel.
  ///  * If [TextInputClient] is implemented then updates for the editing
  ///    state will come through [TextInputClient.updateEditingValue].
  ///
  /// Defaults to false.
  final bool enableDeltaModel;

I'd like to hear your opinion.

Update

I'm currently testing whether this implementation is actually worthwhile. So far, I haven't encountered any issues, but I'm sure some tweaks will need to be made. In any case, I strongly believe we should use DeltaTextInputClient instead of TextInputClient.

The only thing that allows DeltaTextInputClient to work and be called is adding this parameter to TextInput.attach:

_textInputConnection = TextInput.attach(
 this,
 TextInputConfiguration(
    inputType: TextInputType.multiline,
    readOnly: widget.config.readOnly,
+   enableDeltaModel: true,
    inputAction: widget.config.textInputAction,
    enableSuggestions: !widget.config.readOnly,
    keyboardAppearance: widget.config.keyboardAppearance ??
      CupertinoTheme.maybeBrightnessOf(context) ??
      Theme.of(context).brightness,
    textCapitalization: widget.config.textCapitalization,
    allowedMimeTypes: widget.config.contentInsertionConfiguration == null
      ? const <String>[]
      : widget.config.contentInsertionConfiguration!.allowedMimeTypes,
),

@CatHood0 CatHood0 changed the title chore: improve changes application from TextInputClient Chore: Replace TextInputClient with DeltaTextInputClient Mar 29, 2025
@CatHood0
Copy link
Collaborator Author

CatHood0 commented Mar 29, 2025

@EchoEllet do you know what could be happening here?

I'm just using ctrl+v (paste text that was copied before from the editor) several times.

[ERROR:flutter/runtime/dart_vm_initializer.cc(40)] Unhandled Exception: PathNotFoundException: Cannot delete file, path = '/tmp/quill_native_bridge_linux/xclip' (OS
 Error: No existe el fichero o el directorio, errno = 2)
#0      _checkForErrorResponse (dart:io/common.dart:58:9)
#1      _File._delete.<anonymous closure> (dart:io/file_impl.dart:352:9)
<asynchronous suspension>
#2      QuillNativeBridgeLinux.getClipboardImage (package:quill_native_bridge_linux/quill_native_bridge_linux.dart:200:7)
<asynchronous suspension>
#3      DefaultClipboardService.getImageFile (package:flutter_quill/src/editor_toolbar_controller_shared/clipboard/default_clipboard_service.dart:28:12)
<asynchronous suspension>
#4      QuillController.clipboardPaste (package:flutter_quill/src/controller/quill_controller.dart:589:26)
<asynchronous suspension>
#5      QuillRawEditorState.pasteText (package:flutter_quill/src/editor/raw_editor/raw_editor_state.dart:145:9)
<asynchronous suspension>

@CatHood0
Copy link
Collaborator Author

CatHood0 commented Mar 29, 2025

Yeah, i'm noticing that these changes breaks out tests since flutter_quill_test API does not support updateEditingValueWithDeltas. I'll fix it.

@CatHood0 CatHood0 changed the title Chore: Replace TextInputClient with DeltaTextInputClient Chore(breaking changes): Replace TextInputClient with DeltaTextInputClient Mar 29, 2025
@CatHood0
Copy link
Collaborator Author

CatHood0 commented Mar 29, 2025

Is this #2510 (comment) probably related with #2450?

@CatHood0
Copy link
Collaborator Author

CatHood0 commented Mar 29, 2025

Is this issue the same as #2445 ?

I only test this "hack" in Android.

I've managed to reproduce the problem. Now, if you select text, it won't appear. You have to do a little hack.

Steps:

  • Copy the entire contents of the example.
  • Position yourself anywhere in the editor.
  • Paste the text multiple times (even if your phone gets stuck)
  • Select anywhere in the editor and quickly type something.

This may not be easy to reproduce. But it's as simple as not giving the editor time to process what's happening.

It stops happening if you first select, wait a moment, and then type.

What could be happening?

I suspect this is because the getDiff is running in one place, and suddenly, an update from the DeltaTextInputClient/TextInputClient is also executed. This generates an update conflict, where the input client runs first and place the cursor at the "correct" position, but once the getDiff is finished, it doesn't know that it no longer needs to update the selection with an outdated value, and then this happens.

That is, it is an event error that occurs if they are executed at the same time, causing them to collide, since both have their own values, but they do not know which should be executed first and which should not.

@EchoEllet what do you think?

For example, super_editor has a variable to avoid send selection updates (this is into a custom implementation of TextInputConnection) while applying change in their implementation of DeltaTextInputClient.

if (_isApplyingDeltas) {
  // We're in the middle of applying a series of text deltas. Don't
  // send any updates to the IME because it will conflict with the
  // changes we're actively processing.
  editorImeLog.fine("Ignoring new TextEditingValue because we're applying deltas");
  return;
}

@CatHood0
Copy link
Collaborator Author

CatHood0 commented Apr 4, 2025

I'll start updating these PRs and finishing them.

I've been a bit busy, and I'll likely have less time in the near future for certain personal reasons. Still, I'll continue to help however I can.

@jonasbernardo
Copy link

Is there anything I can do to get it fixed? I'm having a lot of bugs due to this problem and I'm available to help

@EchoEllet
Copy link
Collaborator

EchoEllet commented Apr 16, 2025

Do you know what could be happening here?

I missed the notification. This is probably a bug. The plugin should remove this file if it hasn't already been removed by the system. Could you share details including the desktop environment and the distro, and whether if you're using any sandboxing to run the app (e.g. Flatpak). Please consider filing an issue to quill-native-bridge repo if you have the time. Unfortunately, I'm not able to work on the project for this period of time. This line could be related.

@CatHood0
Copy link
Collaborator Author

CatHood0 commented Apr 17, 2025

Could you share details including the desktop environment and the distro, and whether if you're using any sandboxing to run the app (e.g. Flatpak)

I'm using Debian 12 with the Kernel 6.12.12. And I'm just running the app directly using flutter run -d linux in the terminal.

Please consider filing an issue to quill-native-bridge repo if you have the time. Unfortunately, I'm not able to work on the project for this period of time. This line could be related.

Let me reproduce the problem again and add more details to the issue.

And don't worry about the difficulty over time issue. I understand. I'm exactly the same right now.

@CatHood0
Copy link
Collaborator Author

CatHood0 commented Apr 17, 2025

Is there anything I can do to get it fixed? I'm having a lot of bugs due to this problem and I'm available to help

It's not much at the moment.

We're just having issues with how TextSelectionDelegate and TextInputClient communicate, which (still theoretical) seem to be colliding when certain events occur too quickly, causing the issue with the cursor not updating.

Also, on the Web, there seem to be some issues when moving the cursor up or down using ctrl+arrow up/down, which seem to be accumulating (something not from our side).

This last issue is something Flutter is currently having (the same thing is happening with text selection). I was waiting for Flutter to release a new update to check if they had fixed this issue, but nothing has happened yet.

In any case, the comments (most of which are active on this PR) have information about the problems we are having that cause the PR not to be merged.

@EchoEllet
Copy link
Collaborator

i'll need some help with apple OS

Could you provide some steps on how you're testing this change so I can reproduce on macOS and iOS?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request in progress This issue or feature is currently being worked on by someone.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants