[SuperTextField] Fix new line duplication (Resolves #1468) #1469

angelosilvestre · 2023-09-26T00:34:36Z

[SuperTextField] Fix new line duplication. Resolves #1468

On macOS, pressing ENTER or SHIFT + ENTER is inserting two lines. This is caused because pressing ENTER generates both an onPerformAction call and a \n insertion.

On Windows this issue doesn't happen because we are using key event handlers to insert new lines. If we remove the key handler that inserts new lines, the issue happens on Windows as well. This issue was introduced when the selector handlers were implemented.

This PR changes _onPerformAction to avoid inserting new lines.

matthew-carroll · 2023-09-28T20:05:44Z

super_editor/test/super_editor/supereditor_test_tools.dart

@@ -738,3 +739,21 @@ class FakeSuperEditorScroller implements DocumentScroller {
  @override
  void detach() => throw UnimplementedError();
 }
+
+/// The platform to be used when simulating a keyboard event with `sendKeyEvent`,


Why would this platform be different than the defaultTargetPlatform? Also, where is this being used?

It's used in _pressEnterOnSuperTextField. I extracted this from pressEnter.

Why did you extract it instead of use it? And also I'd still like to know why it doesn't use defaultTargetPlatform. I don't remember what we did for pressEnter originally, or why.

I didn't use pressEnter because I had to check whether or not the key was handled in order to generate the input action and the insertion delta.

matthew-carroll · 2023-09-28T20:06:02Z

super_editor/lib/src/super_textfield/desktop/desktop_textfield.dart

@@ -1288,7 +1288,13 @@ class _SuperTextFieldImeInteractorState extends State<SuperTextFieldImeInteracto
  void _onPerformAction(TextInputAction action) {
    switch (action) {
      case TextInputAction.newline:
-        widget.textController.insertNewline();
+        // On mac desktop, we don't insert new lines via keyboard events.


This says "On mac desktop" but I don't see any platform inspection nearby....

I'm not inspecting the platform because for other desktop platforms, the new line is inserted via keyboard handlers. Because of that, the IME doesn't reports input actions nor insertion deltas.

If, at some point, we remove the ENTER keyboard handler, we would be in the same situation as on mac...

Should I just update the comment?

Is the purpose of this code comment to explain why we're not doing anything when we get a newline?

If so, does the following comment better capture the situation?

// Do nothing for IME newline actions // // Mac: Key presses flow, unhandled, to the OS and turn into IME selectors. We handle newlines there. // Windows/Linux: Newlines are inserted by Super Editor key handlers, instead of IME actions, because Flutter doesn't dispatch newline IME actions for these platforms. // Android/iOS: This text field implementation is only for desktop, mobile is handled elsewhere.

Also, can you remind me why we're using key handlers for Windows/Linux and not using the IME actions? Is that because Flutter doesn't send us IME actions for those?

Previously, we used key handlers to insert new line on all desktop platforms. After the performSelection implementation, we stopped doing that on mac.

We could probably could do the same on the other desktop platforms. I tested on Windows and we also receive the input actions and insertion deltas if we don't handle the ENTER key.

Ok. Let's try removing the ENTER handler from the IME handler list for Windows and Linux.

matthew-carroll · 2023-09-28T20:08:26Z

super_editor/test/super_textfield/super_textfield_ime_test.dart

@@ -683,6 +621,38 @@ void main() {
  });
 }

+Future<void> _pressEnterOnSuperTextField(WidgetTester tester) async {


Is this actually specific to SuperTextField? Why doesn't this apply to SuperEditor, too? Also, does this belong in the testing robots package for general use?

We could add this to the test robots. Should I open a PR for this?

Yeah you can open a PR for that. What about the first two questions?

This will apply for SuperEditor also. I was looking into implementing it on testing robots pressEnter, but then we will need to add an optional imeClientGetter parameter.

To generate deltas, we need to access DeltaTextInputClient in order to obtain the current editing value.

Can we introduce a pressEnterOnIme() to the robots package?

Opened Flutter-Bounty-Hunters/flutter_test_robots#20

Can you release a new version of flutter_test_robots? We'll need a pub version with the new methods.

I just released v0.0.22

matthew-carroll · 2023-09-28T20:08:46Z

super_editor/test/super_textfield/super_textfield_ime_test.dart

+
+  // The ENTER key event wasn't handled by the textfield.
+  // The OS generates both a "\n" insertion and a new line action.
+  await tester.ime.typeText('\n', getter: imeClientGetter);


Should we be checking for an open connection instead of assuming that there is one?

matthew-carroll · 2023-10-04T17:36:43Z

@angelosilvestre is this ready for review? Or still doing work?

matthew-carroll · 2023-10-04T17:36:56Z

Also, looks like tests are failing, FYI.

angelosilvestre · 2023-10-04T22:17:13Z

@matthew-carroll This is ready for another review now.

matthew-carroll · 2023-10-16T00:55:25Z

Looks like this PR has a merge conflict.

angelosilvestre · 2023-10-16T01:08:31Z

@matthew-carroll Rebased.

matthew-carroll

LGTM with removal of one superfluous import

matthew-carroll · 2023-10-16T05:31:04Z

super_editor/test/super_editor/supereditor_test_tools.dart

@@ -1,6 +1,7 @@
 import 'dart:math';
 import 'dart:ui' as ui;

+import 'package:flutter/foundation.dart';


Do you need this import?

Nope. Removed.

matthew-carroll · 2023-10-17T21:19:10Z

FYI @angelosilvestre CI is going to automatically create a PR when we merge, but you'll have to explicitly close and re-open the PR to get other CI checks to run.

angelosilvestre · 2023-10-17T22:42:13Z

@matthew-carroll I didn't find any PR created for after the merge

matthew-carroll · 2023-10-17T22:45:30Z

Ugh - CI is failing for some reason....complaining that GitHub bot doesn't have permission, which doesn't make sense given that this has worked previously.

https://github.com/superlistapp/super_editor/actions/runs/6552954869/job/17799413806

superlistapp#1469)

angelosilvestre requested a review from matthew-carroll September 26, 2023 00:50

matthew-carroll reviewed Sep 28, 2023

View reviewed changes

matthew-carroll mentioned this pull request Oct 2, 2023

Action tags don't work on web #1478

Closed

angelosilvestre requested a review from matthew-carroll October 4, 2023 22:17

angelosilvestre mentioned this pull request Oct 14, 2023

Created a SuperTextFieldContext with a BuildContext and a TextFieldScroller - passed it key handlers (Resolves #1487) #1507

Merged

angelosilvestre added 10 commits October 15, 2023 22:00

[SuperTextField] Fix new line duplication (Resolves superlistapp#1468)

fd43623

Update comment

e680a7c

Check for ime connections

8f5b94c

Update comments and naming

3f357a3

Generate insertion deltas using test robots

d049437

Remove new line handlers for IME mode

9b8aea5

Use adaptive robot methods

6b400d2

Always use deltas to insert new lines

81b29e3

Bump flutter_test_robots dependency

96c5c7c

PR updates

77754b0

angelosilvestre force-pushed the 1468_newline_duplication branch from bc580eb to 77754b0 Compare October 16, 2023 01:01

matthew-carroll approved these changes Oct 16, 2023

View reviewed changes

Remove import

ac6ea26

matthew-carroll merged commit c851667 into superlistapp:main Oct 17, 2023
10 of 11 checks passed

angelosilvestre deleted the 1468_newline_duplication branch October 17, 2023 22:38

angelosilvestre added a commit to angelosilvestre/super_editor that referenced this pull request Oct 17, 2023

[SuperTextField] Fix new line duplication (Resolves superlistapp#1468) (

3cc0df6

superlistapp#1469)

angelosilvestre mentioned this pull request Oct 17, 2023

Cherry-pick "[SuperTextField] Fix new line duplication (Resolves #1468) (#1469)" to stable #1532

Merged

matthew-carroll pushed a commit that referenced this pull request Oct 17, 2023

[SuperTextField] Fix new line duplication (Resolves #1468) (#1469)

83bcc6f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SuperTextField] Fix new line duplication (Resolves #1468) #1469

[SuperTextField] Fix new line duplication (Resolves #1468) #1469

angelosilvestre commented Sep 26, 2023

matthew-carroll Sep 28, 2023

angelosilvestre Sep 28, 2023

matthew-carroll Sep 29, 2023

angelosilvestre Sep 30, 2023

matthew-carroll Sep 28, 2023

angelosilvestre Sep 28, 2023

matthew-carroll Sep 29, 2023

angelosilvestre Sep 30, 2023

matthew-carroll Sep 30, 2023

matthew-carroll Sep 28, 2023

angelosilvestre Sep 28, 2023

matthew-carroll Sep 29, 2023

angelosilvestre Sep 30, 2023

matthew-carroll Sep 30, 2023

angelosilvestre Sep 30, 2023

angelosilvestre Oct 4, 2023

matthew-carroll Oct 4, 2023

matthew-carroll Sep 28, 2023

angelosilvestre Sep 28, 2023

matthew-carroll commented Oct 4, 2023

matthew-carroll commented Oct 4, 2023

angelosilvestre commented Oct 4, 2023

matthew-carroll commented Oct 16, 2023

angelosilvestre commented Oct 16, 2023

matthew-carroll left a comment

matthew-carroll Oct 16, 2023

angelosilvestre Oct 16, 2023

matthew-carroll commented Oct 17, 2023

angelosilvestre commented Oct 17, 2023

matthew-carroll commented Oct 17, 2023

[SuperTextField] Fix new line duplication (Resolves #1468) #1469

[SuperTextField] Fix new line duplication (Resolves #1468) #1469

Conversation

angelosilvestre commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-carroll commented Oct 4, 2023

matthew-carroll commented Oct 4, 2023

angelosilvestre commented Oct 4, 2023

matthew-carroll commented Oct 16, 2023

angelosilvestre commented Oct 16, 2023

matthew-carroll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-carroll commented Oct 17, 2023

angelosilvestre commented Oct 17, 2023

matthew-carroll commented Oct 17, 2023