flutter-quill
flutter-quill copied to clipboard
singerdmx
[Mobile] Text Selection only preserves the last boundary update, all previous updates are lost during the same session.
My issue is about [Android] on Quill Version 5.03 read only editor with enableinteractiveselection set to true
I have tried running example directory successfully before creating an issue here.
Issue Description and steps to reproduce:
Given text "... abcdefghijklmn ...", follow the following steps:
- long press on character e, now text becomes abcdEfghijklmn ... [correct so far]
- extend the starting boundary from before e to before d, now text becomes abcDEfghijklmn ... [correct so far]
- extend the ending boundary from after e to after f, now text becomes abcdEFghijklmn ... [incorrect]
Desired behavior
should produce abcDEFghijklmn ... after step 3.
Analysis
The problem could be fixed in either
- widgets/controller.dart [line 326] _updateSelection(textSelection, source) method
- widgets/text_selection.dart [line 517] _handleDragUpdate(details)
changes to the text selection is originally generated in _handleDragUpdate, however when newSelection is generated, in line 531 and/or line 539, it does not take into account of the old selection's value because by the time execution reaches 529, the original value in widget.selection is already overwritten. So even though line 531/539 APPEARS to take into account of the old value on the supposedly 'not updated' boundary,
When the inaccurate newSelection is passed downstream at line 554, unfortuntely the important information of _TextSelectionHandlePosition is forever lost, hence controller.dart _updateSelection(textSelection, source) method does not have enough information at this stage to merge old and not-so-accurate-new-value into the correct new value at line [email protected].
Furthermore, I am not sure about the rationale behind line 327 to 331 in controller.dart _updateSelection method
void _updateSelection(TextSelection textSelection, ChangeSource source) { _selection = textSelection; final end = document.length - 1; _selection = selection.copyWith( baseOffset: math.min(selection.baseOffset, end), extentOffset: math.min(selection.extentOffset, end));
simply by static analysis, at this stage the aforemetioned lines is just equivalent to _selection = textSelection;
I propose either directly produce the correct newSelection value in text_selection.dart or modify the api to accomodate passing _TextSelectionHandlePosition flag info downstream so that _updateSelection in controller.dart can reconstruct the correct final value.
Furthermore, I am not sure about the rationale behind line 327 to 331 in controller.dart _updateSelection method void _updateSelection(TextSelection textSelection, ChangeSource source) { _selection = textSelection; final end = document.length - 1; _selection = selection.copyWith( baseOffset: math.min(selection.baseOffset, end), extentOffset: math.min(selection.extentOffset, end));
This is to deal with image/video, the offset could be off by 1
Check out https://github.com/singerdmx/flutter-quill/blob/master/CodeIntroduction.md
@singerdmx ,
Furthermore, I am not sure about the rationale behind line 327 to 331 in controller.dart _updateSelection method void _updateSelection(TextSelection textSelection, ChangeSource source) { _selection = textSelection; final end = document.length - 1; _selection = selection.copyWith( baseOffset: math.min(selection.baseOffset, end), extentOffset: math.min(selection.extentOffset, end));
This is to deal with image/video, the offset could be off by 1
Thanks for the clarification, now that I understand the original intention surrounding the boundary check, let's suppose the offset is indeed off (by syntax we know the 'off' is an overflow) by 1 after crossing over some image/video, and suppose this is indeed corrected correctly by the boundary check when it is hitting doc.length, since regardless of hitting doc.length, off by 1 is a known post-condition of TextSelection crossing over image/video, then am I correct to deduce when there is media crossing and it did not hit or reach doc.length, this code would actually update to the wrong value?
On requesting PR, I would be happy to fix the issue, but given the massive refactorings, my limited experience with the code base and the limited scope my usecases, I am not sure if it is fit to be merged into the main branch as I don't know if I am breaking other things and affecting other users. I was thinking someone from your dev team originally working on this feature with competent background knowledge and issue awareness could pick up the issue and locate the bug easily.
Based on my own finding, at the moment quill is one of the best open source rich text editor on the flutter platform, I don't think forking off quill to start a new project and dilute its user base is fair to the original dev team, but I do think the code base has some potential for improvement. It has the potential to become the single goto editor in flutter, I was hoping you guys have some plans for redesigning the code base to strip away all the technical debts and feature burdens and built a core library with no bullshit and just refactor the current quill to depend on the core library, the current quill then maintains expansion features building on top of the core, saving your userbase while making future bug-fixing and maintenance much easier.
I was hoping in the core library, these dependencies could be removed.
- flutter_colorpicker: ^1.0.3
- image_picker: ^0.8.5
- photo_view: ^0.13.0
- url_launcher: ^6.1.0
- video_player: ^2.4.0
- youtube_player_flutter: ^8.0.0
- i18n_extension: ^4.2.1
- gallery_saver: ^2.3.2
My argument goes like this, even though the inner API and message passing is a bit messy at this stage, the front facing API is well designed enough, to the extent that for whatever new features I needed in the past, I could easily build on top of quill without modding its source code, quill being a foundational infrastructure should not be too concerned with feature sets, especially when UI features and functions are so easy to implement in flutter. Most of the libraries mentioned above are not production-grade, they are not adding value to quill. Colorpicker, photo-view, video-player, you-tubeplayer, gallery save are just trash. On the other hand, you already have functional embedBuilder, and urlLanch handler in your editor's constructor arguments, why then import url_launcher and video players etc in quill, users making their own import can achieve the same goal with one line of code. For i18n, infrastructure library should not dictate users on what other infrastructure lib to use, just like you don't want to develop quill with getX, leave those decisions open to the user, depending on i18n 4.2.1 in quill would force some users to include multiple i18n solutions in their pubspec.
Regarding getting rid of those dependencies, you are welcome to replace the so-called trash with better one. Forcing users to implement their own is not ideal. You are entitled to your opinion but not entitled to facts. Most users just want a library that just works without needing to implement Additionally. You have to understand there are different people out there using this library. By making it better we should provide better color picker and etc.
The so easy to implement features should be included here and you are welcome to replace existing one with better. Your ask may be legit but most users are not like you.
when it is hitting doc.length
Off by 1 is subtracting 1, when would it exceed the doc length. I think the root cause is not here
For example if you want to get rid of image_picker, please submit a PR replacing it with whatever you have to make it better. I am happy to accept your PR and then image_picker is gone.
The point here is that we provide the best implementation in this library for image picking, not forcing users to implement.
I think I misspoke about line 327 to 331 in controller.dart Let me try to recall what logic is behind it.
Maybe line 327 to 331 in controller.dart is useless like you said. We probably can remove it
@singerdmx
I was hoping you guys have some plans for redesigning the code base to strip away all the technical debts and feature burdens and built a core library with no bullshit and just refactor the current quill to depend on the core library, the current quill then maintains expansion features building on top of the core, saving your userbase while making future bug-fixing and maintenance much easier
I think you misunderstood my point. I am not saying you ditch those libraries in quill
I was hoping in the core library, these dependencies could be removed
I am saying in there is a set of mission critical core functions which need to be correct with the highest priority, these can be isolated into a core package, so that when a bug is found in those mission critical core functions, it can be fixed faster, without meandering into mysteries whether my fix in core is going to break some video embed or color picker. On the other hand, because implementations of those expansion feature set in quill such as color picker now moves out of quill, it is forced not to rely on easy access to private state in core, but only publicly exposed apis, those additional features will be automatically more robust against core's internal changes.
Anyway feel free to close the ticket for now, I will reopen/link to this the issue when I get a fix ready, you can then take a look if you like those changes, then migrate, if not then so be it. It will be sometime in September.
BTW you mean we need to publish two packages in pub.dev, right? One package for quill logic and second package for those dependencies.
On the other hand, because implementations of those expansion feature set in quill such as color picker now moves out of quill, it is forced not to rely on easy access to private state in core, but only publicly exposed apis, those additional features will be automatically more robust against core's internal changes.
I agree. I wanted to do this at one time. Then some users told me it is ok... then I let it go :P
Thank you for the report, this issue has been open for a while and since there are no interactions with it, we had to close it
but if you still facing this issue, or other issues or you have any other questions, feel free to contact us anytime you want.
