singerdmx
Chore(breaking changes): Replace TextInputClient with DeltaTextInputClient
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:
ime: In this folder, we find the most common methods used to apply theTextEditingDeltaswe obtained fromupdateEditingValueWithDeltas. Here, you can find methods such asonReplace,onInsert,onDelete, oronNonTextUpdate, which have their own logic for each use case.
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
TextEditingDeltagives 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
- Fix: #2370
Type of Change
- [ ] ✨ Feature: New functionality without breaking existing features.
- [ ] 🛠️ Bug fix: Resolves an issue without altering current behavior.
- [x] 🧹 Refactor: Code reorganization, no behavior change.
- [x] ❌ Breaking: Alters existing functionality and requires updates.
- [ ] 🧪 Tests: New or modified tests
- [ ] 📝 Documentation: Updates or additions to documentation.
- [x] 🗑️ 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)
- [x] fix buggy behavior on deletion operations in web browsers
- [x] fix buggy behavior in android (the caret is not synced with the
TextEditingValuepassed)
Idk exactly why, but, when i use web browsers, this exceptions are throwed:
DartError: Unsupported operation: Platform._operatingSystem
dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 307:3 throw_
dart-sdk/lib/_internal/js_dev_runtime/private/profile.dart 117:39 _operatingSystem
dart-sdk/lib/io/platform_impl.dart 56:40 get operatingSystem
dart-sdk/lib/io/platform.dart 83:44 get operatingSystem
dart-sdk/lib/_internal/js_dev_runtime/private/profile.dart 117:39 get
dart-sdk/lib/io/platform.dart 146:48 get isMacOS
dart-sdk/lib/_internal/js_dev_runtime/private/profile.dart 117:39 get
packages/flutter_quill/src/editor/raw_editor/input/raw_editor_state_input_client_mixin.dart 266:18 [_updateComposing]
packages/flutter_quill/src/editor/raw_editor/input/raw_editor_state_input_client_mixin.dart 223:7 <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 622:19 <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 647:23 <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 541:3 _asyncStartSync
packages/flutter_quill/src/editor/raw_editor/input/raw_editor_state_input_client_mixin.dart 216:16 [_apply]
packages/flutter_quill/src/editor/raw_editor/input/raw_editor_state_input_client_mixin.dart 213:5 updateEditingValue
packages/flutter/src/services/text_input.dart 2153:15 [_updateEditingValue]
packages/flutter/src/services/text_input.dart 1985:29 <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 622:19 <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 647:23 <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 541:3 _asyncStartSync
packages/flutter/src/services/text_input.dart 1888:19 [_handleTextInputInvocation]
packages/flutter/src/services/text_input.dart 1866:20 <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 622:19 <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 647:23 <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 541:3 _asyncStartSync
packages/flutter/src/services/text_input.dart 1864:19 [_loudlyHandleTextInputInvocation]
packages/flutter/src/services/platform_channel.dart 611:55 <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 622:19 <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 647:23 <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 541:3 _asyncStartSync
packages/flutter/src/services/platform_channel.dart 605:21 [_handleAsMethodCall]
packages/flutter/src/services/platform_channel.dart 601:55 <fn>
packages/flutter/src/services/binding.dart 650:35 <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 622:19 <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 647:23 <fn>
dart-sdk/lib/_internal/js_dev_runtime/patch/async_patch.dart 541:3 _asyncStartSync
packages/flutter/src/services/binding.dart 649:9 <fn>
lib/_engine/engine/platform_dispatcher.dart 1377:5 invoke2
lib/ui/channel_buffers.dart 25:12 invoke
lib/ui/channel_buffers.dart 70:7 push
lib/ui/channel_buffers.dart 136:16 push
lib/_engine/engine/platform_dispatcher.dart 451:10 invokeOnPlatformMessage
lib/_engine/engine/text_editing/text_editing.dart 2170:30 updateEditingState
lib/_engine/engine/text_editing/text_editing.dart 2286:11 <fn>
lib/_engine/engine/text_editing/text_editing.dart 1458:7 handleChange
dart-sdk/lib/_internal/js_dev_runtime/patch/js_allow_interop_patch.dart 224:27 _callDartFunctionFast1
I know that the issue comes from _updateComposing() method. I'll try to know how to fix it.
If you're wondering why this PR suddenly has commits from other PRs, it was my mistake when trying to get the latest changes from master, using git pull instead of git merge.
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 upand then try to use thebackspacekey to remove the text in selection, it will only update the position (note that on the web, it's actually just sending aTextEditingDeltaNonTextUpdateinstead of aTextEditingDeltaDeletion). - If I try to go right after performing the previous action, it will now completely ignore that event.
- If I try to use the
deletekey specifically, instead of removing the text to the right, it will only delete it as if it were the backspace key.
https://github.com/user-attachments/assets/7e194447-e3a1-41d7-96e7-9d1965921218
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.
[!IMPORTANT]
I've been testing the change made in #2505, and it seems that when you press
shift+arrow downin a Header block, it suddenly selects more text than it should (i'm gonna add a video later).I can't say exactly why, but it seems like the change made in that PR needs to be tested, as it could impact the experience for users who aren't using any Chinese IME.
Idk exactly why, but, when i use web browsers, this exceptions are throwed:
That's because you're using dart:io on the web, which is not supported. We prefer not to import dart:io at all or use conditional imports if necessary (which is error-prone and will need Dart analysis rule) to get a full score on pub.dev.
I'm going to check why bug_fix_test is suddenly failing (when i'm actually using the selection that comes from the new TextEditingValue which contains the correct values to make the changes in the QuillController is replaceText.
Small update: when i used the TextSelection that comes from the QuillController, the tests automatically passed without showing any exception, but when i use the selection that comes from the TextEditingValue, they do not pass (i notice that something is wrong with this one, but i do not know exactly what). It's weird.
That's because you're using dart:io on the web, which is not supported. We prefer not to import dart:io at all or use conditional imports if necessary (which is error-prone and will need Dart analysis rule) to get a full score on pub.dev.
Thanks for the clarification! I really didn't know this. That explains why some of my projects had problems with the web.
Better code readability
I think clients prefer stability over readability.
Allows new contributors to enter the code and modify it without much fear of breaking something
This should be covered with tests instead. If you're interested in working on the tests and there are any blockers, please let me know.
Also, we should be responsible for merging the PR and checking it properly, not the contributors.
I'm not very confident about the merge of this PR. That's a lot to cover and check (this and the last few PRs, including the incoming ones) and I'm not able to do much for this period of time, as such, I can't really maintain the project with new PRs so I will leave it to you.
Does this PR impact the functionality or aim to change it?
I think clients prefer stability over readability.
Stability hasn't been affected by these changes. I've run some tests on Desktop, Android, and Web, and the performance remains the same as the previous implementation (I need to check on iOS and macOS).
I'll see how I can make performance comparisons between the two implementations, to have a clearer perspective of whether this changes, maintains, improves or worsens the stability of this part of the project.
Does this PR impact the functionality or aim to change it?
Actually, this is more of a change focused on making the code easier to maintain, while maintaining exactly the same functionality and stability.
I'm not very confident about the merge of this PR. That's a lot to cover and check (this and the last few PRs, including the incoming ones) and I'm not able to do much for this period of time, as such, I can't really maintain the project with new PRs so I will leave it to you.
There's no problem. We can take all the time that we need to check this PR.
Web browsers don't seem to work as expected.
When you press shift+arrow up, events gradually accumulate unnecessarily. That is, the first time, it selects an entire line, then two full lines, and so on until you reach the beginning of the document just by pressing shift+arrow up
Steps to reproduce it
- Position the cursor at some point of the document.
- Do
shift+arrow up. - Move the cursor to remove the selection.
- Go to first step again and repeat the times you wang.
I can conclude that, in some way, events are piling up.
https://github.com/user-attachments/assets/63c88b4a-8bfb-41ec-b8b8-000625778e57
I don't know if it's worth mentioning, but before updating to Flutter 3.29.1, while testing how this PR worked on mobile devices, the cursor out-of-sync issue suddenly returned. No matter what i did, i thought it was back.
As I noticed there was a new version available, i update the SDK and i retested the example app, and to my relief, it finally disappeared.
It's strange. When i uploaded this PR, i tested it on mobile devices (on version 3.29.0) and it was working fine, and then suddenly, until a few minutes ago, the issue appeared and disappeared. I wonder what's causing this issue.
I tried looking at the Flutter SDK Release Notes, but they don't mention fixing anything similar.
This is confusing, to be honest.
Is this issue the same as #2445 ?
Yes, they're closely related since they reproduce in the same way.
The last time i tested using a mobile device, the error didn't appear and it worked as expected.
If you notice that typing on Android is a bit slow, it's due to a change i made locally for testing purposes. That change isn't included in this PR.
https://github.com/user-attachments/assets/921a5d39-9fa9-4c12-a9c9-777774241109
I've been investigating a few things. And one of the most interesting is how super_editor manages to listen for keyboard events regardless of the platform. And, without needing to use text strings, it's able to apply the changes, since it's based on the same TextEditingDelta.
I'm deciding whether we should find another implementation for this aspect, which would eliminate this disadvantage of having to differentiate changes from one string to another, when we could base it on specific positions and specific changes.
For now, I'll continue analyzing how super_editor manages to do this. It's quite interesting, so for anyone interested in merging this PR, I'm sorry to say it will take even longer.
Possibly related to: #2517
@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,
),
@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>
Yeah, i'm noticing that these changes breaks out tests since flutter_quill_test API does not support updateEditingValueWithDeltas. I'll fix it.
Is this https://github.com/singerdmx/flutter-quill/pull/2510#issuecomment-2719891365 probably related with #2450?
Is this issue the same as https://github.com/singerdmx/flutter-quill/issues/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;
}
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.
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
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.
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.
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.
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?