Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Updated RawEditorStateTextInputClientMixin to implement DeltaTextInputClient #35

Merged
merged 12 commits into from
Feb 5, 2023
Merged

Updated RawEditorStateTextInputClientMixin to implement DeltaTextInputClient #35

merged 12 commits into from
Feb 5, 2023

Conversation

Amir-P
Copy link
Member

@Amir-P Amir-P commented Sep 1, 2022 •
edited
Loading

Fixes #34 and #31 .

Thanks to @cgestes for testing and finding selection issue.

@Amir-P
Copy link
Member Author

Amir-P commented Sep 1, 2022

It's still draft but can you test and provide feedback on it? @amantoux

@Amir-P Amir-P requested a review from amantoux September 1, 2022 20:02
@Amir-P Amir-P linked an issue Sep 1, 2022 that may be closed by this pull request
Typing in editor adds the previous typed text after recreating controller #31
Closed
amantoux
amantoux requested changes Sep 8, 2022
View reviewed changes
Copy link
Member

@amantoux amantoux left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Amir-P

packages/fleather/lib/src/widgets/editor_input_client_mixin.dart Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 8, 2022 •
edited
Loading

Codecov Report

Merging #35 (ac27990) into master (68ae54f) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
+ Coverage   75.45%   75.49%   +0.04%     
==========================================
  Files          57       57              
  Lines        8644     8644              
==========================================
+ Hits         6522     6526       +4     
+ Misses       2122     2118       -4
Impacted Files Coverage Δ
...her/lib/src/widgets/editor_input_client_mixin.dart 47.15% <100.00%> (+3.25%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Amir-P Amir-P requested a review from amantoux September 8, 2022 17:21
@cgestes
Copy link
Contributor

cgestes commented Sep 27, 2022

what's the status of this PR? (have been using daily, works good :))

@amantoux
Copy link
Member

amantoux commented Sep 27, 2022 •
edited
Loading

what's the status of this PR? (have been using daily, works good :))

There's a problem in Flutter web
Hitting suppr, the deletedRange is the same as a del when it should be TextRange(start:deletedRange.start + 1, end: deletedRange.end + 1)

@cgestes no issues on web?

Note: flutter/engine#28527

@cgestes
Copy link
Contributor

cgestes commented Sep 27, 2022 •
edited
Loading

didn't try on the web !

dang!

@amantoux
Copy link
Member

flutter/samples#1424

@cgestes
Copy link
Contributor

cgestes commented Oct 6, 2022

thanks to @amantoux for pointing out the bugs in flutter/samples.

they made a PR to fix the web issue. flutter/samples#1458

time to go forward?

@amantoux
Copy link
Member

amantoux commented Oct 6, 2022 •
edited
Loading

Don't we need flutter/engine#36616?

Beside, unfortunately (or fortunately depends on the point of view) I'm using this in production, so need to wait for it to land in a release

@amantoux amantoux force-pushed the use_delta_text_input_client branch from 676111e to 72ee09f Compare December 9, 2022 14:11
@Amir-P
Copy link
Member Author

Amir-P commented Jan 25, 2023

Looks like the issue with deltas for Web is fixed on 3.7 stable. @amantoux @cgestes

@amantoux
Copy link
Member

It needs a rebase I guess

@amantoux
Copy link
Member

@Amir-P we'll upgrade to Flutter 3.7 with #89
Can you rebase once this is done?
btw this PR is still draft, is it still intended?

@Amir-P Amir-P force-pushed the use_delta_text_input_client branch from af51eca to 9f5cb70 Compare January 28, 2023 14:23
@Amir-P
Copy link
Member Author

Amir-P commented Jan 28, 2023

There were problems with history tests using old text editing API. I've updated them and now I think we're good to go. Please review. @amantoux

@Amir-P Amir-P marked this pull request as ready for review January 28, 2023 14:29
amantoux
amantoux reviewed Feb 5, 2023
View reviewed changes
Copy link
Member

@amantoux amantoux left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Amir-P, overall it look good to me
I just have concerns about the dependency on Mocktail package (even if only a dev dependency)
It doesn't seem to be maintained (no code update for the last year almost)

(as discussed, it is reliable)

@amantoux amantoux merged commit b86fb8b into master Feb 5, 2023
@Amir-P Amir-P deleted the use_delta_text_input_client branch February 5, 2023 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amantoux amantoux amantoux requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Flutter 3.3 Typing in editor adds the previous typed text after recreating controller
3 participants
@Amir-P @cgestes @amantoux