Fix TagFinder.findTagAroundPosition for better emoji support #1891

BazinC · 2024-03-12T18:08:31Z

Fixes #1863 and #1882

fixed findTagAroundPosition to support of emojis
Widget Text: added "emojis" group and "with consecutive triggers" in "commits" group.
There was edge case in findTagAroundPosition looping through the parameter text code point one by one.
Fixed it with characters iterators.

BazinC · 2024-03-12T18:36:22Z

golden test fails, I didn't manage to update them.. Can you help me with it?

BazinC · 2024-03-12T18:52:30Z

@matthew-carroll @angelosilvestre

matthew-carroll

Thanks for moving this PR to main - I've done another review.

matthew-carroll · 2024-03-12T19:53:56Z

super_editor/test/super_editor/text_entry/tagging/stable_tags_test.dart

@@ -29,6 +29,25 @@ void main() {
        );
      });

+      testWidgetsOnAllPlatforms("can start with a single character", (tester) async {


What prompted this test? Was there a bug with the first character in a stable tag? Is this situation not thoroughly covered by other tests?

while working on the fix, I arrived to a situation where every test passed but I noticed no compositing tag were detected when typing a single character after the trigger. I noticed and fixed it thanks to your previous review

Please don't resolve a comment unless you're absolutely sure the issue is resolved.

Based on your answer I have multiple questions.

First, the behavior that's being tested here doesn't seem to be broken for me:

Screen.Recording.2024-03-17.at.4.48.50.PM.mov

Second, when you say "@j" wasn't tagged for you, does that also mean that just "@" wasn't tagged for you, either? The composing attribution should be applied as soon as the trigger is typed. You shouldn't need to type any additional character to start composing a tag. So what exactly was working vs not working for you?

If you're trying to ensure that a tag starts composing as early as its supposed to, then you probably want to test "@", not "@j". Moreover, that test would be named "starts when the trigger character is inserted". Lastly, given that such a test verifies the initial behavior for all tag entry, it should be the first test under "composing >".

While I was fixing the emoji bug, I developed an implementation of findTagAroundPosition that resolved the bug, and all existing tests passed. However, I later noticed that the behavior was broken (notice how the popover is not triggered on the first "m" after "@"):

before.fix.mov

Here's a trace of what was happening in findTagAroundPosition:

rawText: "hello @m" splitIndex: 7 // Incorrect ... charactersBefore: StringCharacters (hello @) charactersAfter: StringCharacters (m) ... tokenRange: SpanRange(start: 7, end: 8) ... tagText: "m" -> Since it doesn't start with "@", the method returns null.

I corrected this behavior with this commit (from a previously closed Fix TagFinder.findTagAroundPosition to support emojis).

After this commit, we have:

after.fix.mov

rawText: "hello @m" splitIndex: 8 // Correct ... charactersBefore: StringCharacters (hello @m) charactersAfter: StringCharacters (“”) ... tokenRange: SpanRange(start: 6, end: 8) ... tagText: "@m" // The returned TagAroundPosition is now correct.

This test serves more as a checkpoint to ensure the correct implementation of TagAroundPosition and to capture any regression that was not previously detected.

super_editor/test/super_editor/text_entry/tagging/stable_tags_test.dart

matthew-carroll · 2024-03-12T19:54:41Z

super_editor/test/super_editor/text_entry/tagging/stable_tags_test.dart

@@ -577,6 +598,48 @@ void main() {
          const SpanRange(7, 11),
        );
      });
+
+      testWidgetsOnAllPlatforms("with consecutive triggers", (tester) async {


Was consecutive triggers not already tested elsewhere?

I needed this test because I broke the StableTagIndex behavior while I thought I fixed the main issue. But no test indicaticated something was broken. I added this test to ensure this behavior. I moved it after your comment into 'commit' group test.
I don't see any existing consecutive trigger characters test.

Looking at this test, and looking at the current behavior that I see in the app on main, the behavior for consecutive triggers is wrong. Right now, if I type "@@mike" then I get tags for "@" and "@mike". But the first "@" shouldn't be a tag because it has no value.

We don't want to add a test that verifies a behavior we don't want, so I recommend removing this test and filing an issue that describes the consecutive trigger issue for stable tags.

I use consecutives triggers in our app to support @user/@@task/@@@doc. It's a hack.
I understand your point, I'm removing this test.
We should then think of fixing this behavior for @ in the middle of a sentence without triggering a tag.

matthew-carroll · 2024-03-12T19:55:23Z

super_editor/test/super_editor/text_entry/tagging/stable_tags_test.dart

@@ -1042,19 +1105,222 @@ void main() {
        );
      });
    });
+
+    group("emoji >", () {
+      testWidgetsOnAllPlatforms("typed as first character of a paragraph", (tester) async {


Please describe what you're testing. You've stated that this test types an emoji, but it's not clear what outcome you're looking for. Why does the test exist? Please try to add appropriate details to the test name.

The updated code adds a few more words but still doesn't provide all the useful information to the reader. You mentioned the editor crashing, but you didn't mention why the editor would crash. In theory, we could describe every test as "make sure the editor doesn't crash". The operative detail is why this would crash the editor.

It's good you mentioned the crash in the test name. It's good you linked to the issue ticket.

However, instead of repeating stuff about the editor crashing in the code comment, you should mention what kind of crash you're talking about and/or why it would happen.

Also, in the first test, you failed to mention the relevance of it being the "first character of a paragraph". In the second test, you failed to mention the relevance of moving the caret. Why are these details important? Future readers need to know why these specific details and actions were tested.

super_editor/test/super_editor/text_entry/tagging/stable_tags_test.dart

super_editor/lib/src/default_editor/text_tokenizing/tags.dart

BazinC · 2024-03-13T00:22:11Z

Ready for another review @matthew-carroll

matthew-carroll · 2024-03-17T23:58:01Z

super_editor/test/super_editor/text_entry/tagging/stable_tags_test.dart

@@ -577,6 +598,48 @@ void main() {
          const SpanRange(7, 11),
        );
      });
+
+      testWidgetsOnAllPlatforms("with consecutive triggers", (tester) async {


Looking at this test, and looking at the current behavior that I see in the app on main, the behavior for consecutive triggers is wrong. Right now, if I type "@@mike" then I get tags for "@" and "@mike". But the first "@" shouldn't be a tag because it has no value.

We don't want to add a test that verifies a behavior we don't want, so I recommend removing this test and filing an issue that describes the consecutive trigger issue for stable tags.

matthew-carroll · 2024-03-18T00:02:12Z

super_editor/test/super_editor/text_entry/tagging/stable_tags_test.dart

@@ -1042,19 +1105,222 @@ void main() {
        );
      });
    });
+
+    group("emoji >", () {
+      testWidgetsOnAllPlatforms("typed as first character of a paragraph", (tester) async {


The updated code adds a few more words but still doesn't provide all the useful information to the reader. You mentioned the editor crashing, but you didn't mention why the editor would crash. In theory, we could describe every test as "make sure the editor doesn't crash". The operative detail is why this would crash the editor.

It's good you mentioned the crash in the test name. It's good you linked to the issue ticket.

However, instead of repeating stuff about the editor crashing in the code comment, you should mention what kind of crash you're talking about and/or why it would happen.

Also, in the first test, you failed to mention the relevance of it being the "first character of a paragraph". In the second test, you failed to mention the relevance of moving the caret. Why are these details important? Future readers need to know why these specific details and actions were tested.

matthew-carroll · 2024-03-18T00:03:44Z

super_editor/test/super_editor/text_entry/tagging/stable_tags_test.dart

+        expect(text.text, "💙");
+      });
+
+      testWidgetsOnAllPlatforms("can be captured with trigger", (tester) async {


You added more words to this name, but it doesn't really convey any more information. As a reader of this test, I don't know what "capture" means, and it's not clear why such a test needs to be special for emojis. Please be more specific about what you're verifying in this test.

super_editor/lib/src/default_editor/text_tokenizing/tags.dart

matthew-carroll · 2024-03-22T20:12:31Z

@BazinC any progress on this?

matthew-carroll · 2024-04-10T18:09:16Z

@BazinC checking in one last time - do you plan to update this?

BazinC · 2024-04-10T20:10:21Z

@BazinC checking in one last time - do you plan to update this?

Sorry @matthew-carroll I didn't have time yet. I will give another try tomorrow (I'm afk right now)

matthew-carroll · 2024-04-18T00:29:53Z

@BazinC please be sure to re-request a review whenever you think you've handled the feedback.

fix for superlistapp#1882 and superlistapp#1863 + more tests

8b56dd1

BazinC changed the title ~~fix for https://github.com/superlistapp/super_editor/issues/1882 and …~~ Fix TagFinder.findTagAroundPosition for better emoji support Mar 12, 2024

BazinC mentioned this pull request Mar 12, 2024

Fix TagFinder.findTagAroundPosition to support emojis #1879

Closed

matthew-carroll reviewed Mar 12, 2024

View reviewed changes

BazinC added 3 commits March 12, 2024 23:47

better test naming

26506cd

remove wrong condition in iteratorDownStream loop

c09055d

naming

7d997f7

matthew-carroll reviewed Mar 18, 2024

View reviewed changes

remove consecutive triggers test

3299552

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TagFinder.findTagAroundPosition for better emoji support #1891

Fix TagFinder.findTagAroundPosition for better emoji support #1891

BazinC commented Mar 12, 2024 •

edited

BazinC commented Mar 12, 2024

BazinC commented Mar 12, 2024

matthew-carroll left a comment

matthew-carroll Mar 12, 2024

BazinC Mar 12, 2024

matthew-carroll Mar 17, 2024

BazinC Apr 15, 2024

matthew-carroll Mar 12, 2024

BazinC Mar 12, 2024

matthew-carroll Mar 17, 2024

BazinC Apr 15, 2024

matthew-carroll Mar 12, 2024

matthew-carroll Mar 18, 2024

BazinC commented Mar 13, 2024

matthew-carroll Mar 17, 2024

matthew-carroll Mar 18, 2024

matthew-carroll Mar 18, 2024

matthew-carroll commented Mar 22, 2024

matthew-carroll commented Apr 10, 2024

BazinC commented Apr 10, 2024

matthew-carroll commented Apr 18, 2024

Fix TagFinder.findTagAroundPosition for better emoji support #1891

Are you sure you want to change the base?

Fix TagFinder.findTagAroundPosition for better emoji support #1891

Conversation

BazinC commented Mar 12, 2024 • edited

BazinC commented Mar 12, 2024

BazinC commented Mar 12, 2024

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

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

BazinC commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthew-carroll commented Mar 22, 2024

matthew-carroll commented Apr 10, 2024

BazinC commented Apr 10, 2024

matthew-carroll commented Apr 18, 2024

BazinC commented Mar 12, 2024 •

edited