[Android] Add support for mentions to the Compose library #854

jmartinesp · 2023-10-23T12:04:00Z

Expose replaceSuggestion and ~~setLinkSuggestion~~ insertMentionAtSuggestion in RichTextEditorState. Also add suggestions UI to the compose example.

Also added MentionDetector to replace the client-side parsing of mentions with something from the RTE library.

codecov-commenter · 2023-10-23T12:09:40Z

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (28de74e) 89.50% compared to head (ac1c05f) 89.34%.
Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #854      +/-   ##
==========================================
- Coverage   89.50%   89.34%   -0.17%     
==========================================
  Files         116       86      -30     
  Lines       16801    15247    -1554     
  Branches      633        0     -633     
==========================================
- Hits        15038    13622    -1416     
+ Misses       1740     1625     -115     
+ Partials       23        0      -23

Flag	Coverage Δ
uitests	`?`
uitests-ios	`?`
unittests	`89.34% <0.00%> (+1.09%)`	⬆️
unittests-ios	`?`
unittests-rust	`89.34% <0.00%> (-0.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
crates/matrix_mentions/src/lib.rs	`0.00% <0.00%> (ø)`
bindings/wysiwyg-ffi/src/lib.rs	`0.00% <0.00%> (ø)`
bindings/wysiwyg-ffi/src/ffi_mention_detector.rs	`0.00% <0.00%> (ø)`

... and 32 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jonnyandrew

Looks good! Just one comment about a potential performance issue with the composable

platforms/android/example-compose/src/main/java/io/element/wysiwyg/compose/MainActivity.kt

jonnyandrew · 2023-10-23T12:54:54Z

...s/android/library-compose/src/main/java/io/element/android/wysiwyg/compose/RichTextEditor.kt

+        if (style != previousStyle) {
+            view.setStyleConfig(style.toStyleConfig(view.context))


I don't think this should be necessary because the update block should only run when one of the variables used inside it is changed. When I tested it looks like there issue causing the update method to fire every time the state is changed which is a problem.

Are you able to check and see if there is a stability issue using the compose compiler reports tool?

MentionDisplayHandler is an interface, so I think it's impossible for compose to check if it has changed. Maybe that's what's causing the unnecessary recompositions.

I wrapped the mentionDisplayHandler parameter in a lambda to make it stable and prevent recompositions, but there's still some weird issue with style and textStyleTypeface, where they won't cause recompositions when the code is build from a clean state, but if anything in the code changes and we re-build, the update block will recompose with every change in the composer.

@jonnyandrew could you double check if you can also reproduce the issue above? If not, it might just be an issue on my side and we can assume this is fixed.

Very odd - I can reproduce this unfortunately.

Now that we have a bit more time to work with, is there any way we can eliminate these workarounds (remember and previousStyle)?

It depends on whether we want to avoid creating a new HtmlConverter every time anything changes in this component or not. The remember makes the update more predictable, but it will still be triggered when any of its params change.

The current AndroidHtmlConverter is quite light-weight since it only creates a lambda, so we could allow this, or even create the converter on demand whenever it's supposed to be used, but this might not be the case for future or alternative implementations.

I was hoping we could try to remove the remember altogether if we can find out why update is being called in an unpredictable way 😅

In terms of the HtmlConverter, I don't see to much problem with doing this in update, given it should only be called infrequently in practice.

Ok, I'll remove the previousStyle check, but the remember will have to stay for now, I don't really see what's causing the lambda to be re-created...

Actually, previousStyle has already been removed 😅 . I completely forgot about that.

...d/library-compose/src/main/java/io/element/android/wysiwyg/compose/RichTextEditorDefaults.kt

… folder of the examples

jonnyandrew

Thanks for the changes! I had a few comments

...s/android/library-compose/src/main/java/io/element/android/wysiwyg/compose/RichTextEditor.kt

...ms/android/library/src/main/java/io/element/android/wysiwyg/display/MentionDisplayHandler.kt

platforms/android/library/src/main/java/io/element/android/wysiwyg/utils/HtmlToSpansParser.kt

crates/wysiwyg/src/lib.rs

crates/matrix_mentions/src/mention.rs

bindings/wysiwyg-ffi/src/ffi_mention_detector.rs

Also solve theming issues.

jmartinesp · 2023-10-24T15:45:19Z

I don't like the way we need to provide the styleConfig and mentionDisplayHandler to create the HtmlConverter after c9af2ab, but I can't think of a better way at the moment and this PR is taking ages to merge. Could we address that in a separate one, unless the solution is obvious?

jmartinesp · 2023-10-25T08:26:48Z

The UI tests do work locally, btw. It seems like the project has had issues running these tests for a while, from what I've seen in the results of run actions for other PRs.

…ons. Also: - Add missing `mentionDetector` lambda to `StyledHtmlConverter`. - Add optimizations for recompositions in `EditorStyledText`.

jonnyandrew

Thanks for the changes! Sorry it's taking so long to merge this. I've added a few more comments.

...oid/example-compose/src/main/java/io/element/wysiwyg/compose/DefaultMentionDisplayHandler.kt

...ew/src/main/java/io/element/android/wysiwyg/poc/matrix/MatrixMentionMentionDisplayHandler.kt

...android/library-compose/src/main/java/io/element/android/wysiwyg/compose/EditorStyledText.kt

...s/android/library-compose/src/main/java/io/element/android/wysiwyg/compose/RichTextEditor.kt

platforms/android/library/src/main/java/io/element/android/wysiwyg/EditorEditText.kt

platforms/android/library/src/main/java/io/element/android/wysiwyg/EditorStyledTextView.kt

...droid/library/src/main/java/io/element/android/wysiwyg/internal/viewmodel/EditorViewModel.kt

platforms/android/library/src/main/java/io/element/android/wysiwyg/utils/HtmlToSpansParser.kt

jonnyandrew · 2023-10-25T09:52:32Z

The UI tests do work locally, btw. It seems like the project has had issues running these tests for a while, from what I've seen in the results of run actions for other PRs.

Yeah indeed, I think there must be a caching issue because the tests seem to pass fairly reliably on the second CI run.

jonnyandrew · 2023-10-25T10:39:04Z

...s/android/library-compose/src/main/java/io/element/android/wysiwyg/compose/RichTextEditor.kt

-                return resolveRoomMentionDisplay?.invoke() ?: TextDisplay.Plain
-            }
-
+    }, update = remember(style, typeface, mentionDisplayHandler) {


This workaround seems to work, thank you! Please could you add some comments to explain the problem and why this is needed? I've created an issue to link to as well #856. Please feel free to add more detail that could help us in future.

(friendly reminder to please link to the issue in the code comments)

jonnyandrew · 2023-10-25T10:45:52Z

...android/library-compose/src/main/java/io/element/android/wysiwyg/compose/EditorStyledText.kt

+    }, update = remember(style != previousStyle, typeface, mentionDisplayHandler, text) {
+        { view ->
+            if (style != previousStyle) {
+                view.styleConfig = style.toStyleConfig(view.context)
+                view.applyStyleInCompose(style)
+                previousStyle = style
+            }


Same as above, thanks for the workaround! Please could you add some comments explaining the problem and why this is needed, linking to the GH issue?

...android/library-compose/src/main/java/io/element/android/wysiwyg/compose/EditorStyledText.kt

Also, unify setting the `StyleConfig` and `MentionDisplayHandler` with a new `setupHtmlConverter` method.

…del` when trying to generate a spanned text from HTML

jmartinesp · 2023-10-25T14:05:20Z

All mentioned issues should have been fixed.

jonnyandrew

Thanks for the changes! I have a few more comments as there are quite a few more changes. I've made a few suggestions - Ii'd be great to address the public API issues to minimise breaking changes that we'd need to release in order to fix them.

...s/android/library-compose/src/main/java/io/element/android/wysiwyg/compose/RichTextEditor.kt

...oid/example-compose/src/main/java/io/element/wysiwyg/compose/DefaultMentionDisplayHandler.kt

...s/android/library-compose/src/main/java/io/element/android/wysiwyg/compose/RichTextEditor.kt

...rary-compose/src/main/java/io/element/android/wysiwyg/compose/internal/FakeViewConnection.kt

platforms/android/library/src/main/java/io/element/android/wysiwyg/EditorEditText.kt

platforms/android/library/src/main/java/io/element/android/wysiwyg/EditorStyledTextView.kt

platforms/android/library/src/main/java/io/element/android/wysiwyg/utils/HtmlToSpansParser.kt

…ionText` or `ViewAction.InsertMentionAtSuggestion` are used in `FakeViewConnection`.

…n `@mentionText` or a proper link.

jonnyandrew

There are some mentions related tests failing when I run them locally - please can you fix those?

After that I think we are OK to merge this in its current state and unblock the work on mentions.

Fix tests.

jmartinesp · 2023-11-03T08:25:55Z

I think the last issues should have been fixed in ac1c05f (it seems like when I paused this work I forgot to migrate the mention detection logic). The tests work locally for me with those changes, but I can't make them work in the CI.

This reverts commit ac1c05f.

sonarcloud · 2023-11-03T09:49:23Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

jmartinesp · 2023-11-03T11:08:03Z

Thanks again for the latest changes, I was over complicating things quite a bit 😅 . Should this be ready to merge now?

jonnyandrew

Nice work, let's keep an eye out for any issues around #856

Add support for mentions to the Compose library

0682888

jmartinesp requested a review from jonnyandrew October 23, 2023 12:04

jmartinesp added the Android label Oct 23, 2023

Rename SetLinkSuggestion

c0db8f3

jonnyandrew reviewed Oct 23, 2023

View reviewed changes

jmartinesp added 4 commits October 23, 2023 17:04

Remove style comparison

8d9eeeb

Use MaterialColor for the default background of a pill

c45252f

Fix recompositions

2734580

Extract suggestion related UI and logic to a separate file

c5ddd52

jmartinesp requested a review from jonnyandrew October 24, 2023 08:53

jmartinesp added 3 commits October 24, 2023 13:06

Add MentionDetector to MentionDisplayHandler impls

7220b54

Fix gitignore for some reason git was taking into account the build…

ac71bb6

… folder of the examples

Fix tests

e2fcab1

jonnyandrew reviewed Oct 24, 2023

View reviewed changes

jmartinesp added 2 commits October 24, 2023 16:23

Fix VM leaking the View thought lambdas.

c9af2ab

Also solve theming issues.

Solve several PR review issues

ce455e7

Fix tests (again)

9bf8844

jmartinesp requested a review from jonnyandrew October 24, 2023 15:52

Wrap update blocks in remember to prevent unnecessary recompositi…

817e5bb

…ons. Also: - Add missing `mentionDetector` lambda to `StyledHtmlConverter`. - Add optimizations for recompositions in `EditorStyledText`.

jonnyandrew reviewed Oct 25, 2023

View reviewed changes

jonnyandrew mentioned this pull request Oct 25, 2023

Composable AndroidView update block is run on every key press #856

Closed

jonnyandrew reviewed Oct 25, 2023

View reviewed changes

jmartinesp added 4 commits October 25, 2023 13:48

Remove contentEditable param in hyperlink parsing

59f03ad

Read the right color value for pills in EditorEditTextAttributeReader

6ba1f1b

Add docs for new functions in RichTextEditorState

b685147

Add default TextDisplay mention resolvers to RichTextEditorDefaults

c332b70

jmartinesp added 3 commits October 25, 2023 13:54

Document the remember(keys..., update) workaround.

1671f7a

Also, unify setting the `StyleConfig` and `MentionDisplayHandler` with a new `setupHtmlConverter` method.

Throw an error if the HtmlConverter is not set in the `EditorViewMo…

b9831de

…del` when trying to generate a spanned text from HTML

Fix UI tests

18ed041

jmartinesp requested a review from jonnyandrew October 25, 2023 14:04

jonnyandrew reviewed Oct 26, 2023

View reviewed changes

jmartinesp added 6 commits October 26, 2023 12:04

Add missing onError arg to remember

2d180dc

Check contenteditable value and apply extra spans accordingly

f20c31e

Remove unused import

8e0aab2

Log error when HtmlConverter is missing instead of throwing an error

f166a9b

Rename setupHtmlConverter to setStyle.

d604672

Remove MenuAction.Suggestion when either `ViewAction.ReplaceSuggest…

d1ca6a4

…ionText` or `ViewAction.InsertMentionAtSuggestion` are used in `FakeViewConnection`.

jmartinesp requested a review from jonnyandrew November 2, 2023 09:53

jmartinesp added 2 commits November 2, 2023 17:06

In FakeViewActionCollector, replace the mentions text with either a…

438441d

…n `@mentionText` or a proper link.

Add tests

cc01c08

jonnyandrew reviewed Nov 2, 2023

View reviewed changes

Add missing dependencies to EditorViewModel.

ac1c05f

Fix tests.

jmartinesp requested a review from jonnyandrew November 3, 2023 08:26

jonnyandrew added 2 commits November 3, 2023 09:37

Revert "Add missing dependencies to EditorViewModel."

ee02bfb

This reverts commit ac1c05f.

Fix mention detection logic

a8972c0

jonnyandrew approved these changes Nov 3, 2023

View reviewed changes

jmartinesp merged commit 57d97a7 into main Nov 3, 2023
8 checks passed

jmartinesp deleted the feat/jme/add-suggestion-replacement-android branch November 3, 2023 11:15

jonnyandrew mentioned this pull request Nov 20, 2023

[Android] expose getMentionsState #852

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Android] Add support for mentions to the Compose library #854

[Android] Add support for mentions to the Compose library #854

jmartinesp commented Oct 23, 2023 •

edited

Loading

codecov-commenter commented Oct 23, 2023 •

edited

Loading

jonnyandrew left a comment

jonnyandrew Oct 23, 2023

jmartinesp Oct 23, 2023

jmartinesp Oct 23, 2023

jmartinesp Oct 24, 2023

jonnyandrew Oct 24, 2023

jonnyandrew Nov 2, 2023 •

edited

Loading

jmartinesp Nov 2, 2023

jonnyandrew Nov 2, 2023

jmartinesp Nov 2, 2023

jmartinesp Nov 2, 2023

jonnyandrew left a comment

jmartinesp commented Oct 24, 2023

jmartinesp commented Oct 25, 2023

jonnyandrew left a comment

jonnyandrew commented Oct 25, 2023

jonnyandrew Oct 25, 2023

jonnyandrew Oct 26, 2023

jonnyandrew Oct 25, 2023

jmartinesp commented Oct 25, 2023

jonnyandrew left a comment

jonnyandrew left a comment

jmartinesp commented Nov 3, 2023

sonarcloud bot commented Nov 3, 2023

jmartinesp commented Nov 3, 2023

jonnyandrew left a comment

		if (style != previousStyle) {
		view.setStyleConfig(style.toStyleConfig(view.context))

[Android] Add support for mentions to the Compose library #854

[Android] Add support for mentions to the Compose library #854

Conversation

jmartinesp commented Oct 23, 2023 • edited Loading

codecov-commenter commented Oct 23, 2023 • edited Loading

Codecov Report

jonnyandrew 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

jonnyandrew Nov 2, 2023 • edited Loading

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

jonnyandrew left a comment

Choose a reason for hiding this comment

jmartinesp commented Oct 24, 2023

jmartinesp commented Oct 25, 2023

jonnyandrew left a comment

Choose a reason for hiding this comment

jonnyandrew commented Oct 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp commented Oct 25, 2023

jonnyandrew left a comment

Choose a reason for hiding this comment

jonnyandrew left a comment

Choose a reason for hiding this comment

jmartinesp commented Nov 3, 2023

sonarcloud bot commented Nov 3, 2023

jmartinesp commented Nov 3, 2023

jonnyandrew left a comment

Choose a reason for hiding this comment

jmartinesp commented Oct 23, 2023 •

edited

Loading

codecov-commenter commented Oct 23, 2023 •

edited

Loading

jonnyandrew Nov 2, 2023 •

edited

Loading