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.

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

[Rich text editor] Integrate rich text editor library #1172

Merged
merged 14 commits into from Sep 7, 2023

Conversation

jonnyandrew
Copy link
Contributor

@jonnyandrew jonnyandrew commented Aug 29, 2023

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Replace the text input with the Matrix rich text editor.

Note that markdown formatting is no longer respected as the editor is now in WYSIWYG mode, in preparation for UI formatting buttons. Markdown characters such as *, ., - are now interpreted as literal text and not as markdown syntax.

Motivation and context

This editor supports WYSIWYG editing and lays the foundation for further text formatting functionality.

Screenshots / GIFs

N/A, no change.

Tests

  • Compose a message in the editor.
  • Note that markdown formatting is no longer respected.

Tested devices

  • Physical
  • Emulator
  • OS version(s): Android 13

Checklist

@github-actions
Copy link
Contributor

github-actions bot commented Aug 29, 2023

📱 Scan the QR code below to install the build (arm64 only) for this PR.
QR code
If you can't scan the QR code you can install the build via this link: https://i.diawi.com/6Yav4A

@jonnyandrew jonnyandrew force-pushed the jonny/integrate-rte branch 5 times, most recently from edc0018 to a597272 Compare August 31, 2023 13:24
@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Patch coverage: 74.56% and project coverage change: -0.04% ⚠️

Comparison is base (33dd0b2) 57.71% compared to head (74d28ee) 57.67%.
Report is 2 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1172      +/-   ##
===========================================
- Coverage    57.71%   57.67%   -0.04%     
===========================================
  Files         1078     1081       +3     
  Lines        28038    28028      -10     
  Branches      5777     5779       +2     
===========================================
- Hits         16181    16166      -15     
- Misses        9330     9339       +9     
+ Partials      2527     2523       -4     
Files Changed Coverage Δ
...impl/messagecomposer/RichTextEditorStateFactory.kt 0.00% <0.00%> (ø)
...meline/model/event/TimelineItemTextBasedContent.kt 0.00% <0.00%> (ø)
...nt/android/libraries/matrix/api/room/MatrixRoom.kt 33.33% <ø> (ø)
...droid/libraries/matrix/impl/room/RustMatrixRoom.kt 0.00% <0.00%> (ø)
...roid/libraries/textcomposer/MessageComposerMode.kt 95.00% <ø> (ø)
...ndroid/features/messages/impl/MessagesPresenter.kt 81.81% <25.00%> (-1.01%) ⬇️
...ssages/impl/messagecomposer/MessageComposerView.kt 48.38% <60.00%> (-4.74%) ⬇️
...pl/messagecomposer/MessageComposerStateProvider.kt 90.00% <66.66%> (-5.24%) ⬇️
...ent/android/libraries/textcomposer/TextComposer.kt 84.05% <90.47%> (ø)
...s/impl/messagecomposer/MessageComposerPresenter.kt 90.32% <93.75%> (-0.07%) ⬇️
... and 12 more

... and 3 files with indirect coverage changes

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

@jonnyandrew jonnyandrew force-pushed the jonny/integrate-rte branch 3 times, most recently from 7bb14ef to 66ca091 Compare September 1, 2023 08:15
isFullScreen = isFullScreen.value,
hasFocus = hasFocus.value,
mode = messageComposerContext.composerMode,
showAttachmentSourcePicker = showAttachmentSourcePicker,
canShareLocation = canShareLocation.value,
canCreatePoll = canCreatePoll.value,
attachmentsState = attachmentsState.value,
eventSink = ::handleEvents
eventSink = { handleEvents(it) }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Workaround for a Kotlin reflection error when toString() is called in test assertion failures... Related: #1206

@jonnyandrew jonnyandrew marked this pull request as ready for review September 1, 2023 14:06
@jonnyandrew jonnyandrew requested a review from a team as a code owner September 1, 2023 14:06
@jonnyandrew jonnyandrew requested review from bmarty and removed request for a team September 1, 2023 14:06
@jonnyandrew
Copy link
Contributor Author

jonnyandrew commented Sep 1, 2023

I've marked as ready for review but note that I haven't gotten to the bottom of the test flakiness. However, I suspect the flakiness is unrelated to this change as it is mainly occurring in :features:createroom, features:leaveroom and :features:invitelist modules.

Edit: test flakiness addressed in #1226 and 7ae036e

ElementBot and others added 2 commits September 6, 2023 10:33
Fixes issue where screenshot tests are terminated due to lack of CI
resources.

See actions/runner-images#7188 (comment)
@jonnyandrew
Copy link
Contributor Author

@bmarty FYI this PR is ready for review - the CI test suite is now passing (after both #1226 and 7ae036e). The remaining failures are due to a reduction in total project test coverage (as this change is primarily UI) and an unrelated issue with Sonar.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

I have done some test and:

  • typing and sending message
  • editing a message
  • replying to a message

are all working as expected.

@ElementBot
Copy link
Collaborator

ElementBot commented Sep 7, 2023

Warnings
⚠️

gradle/libs.versions.toml#L13 - A newer version of androidx.core:core-ktx than 1.10.1 is available: 1.12.0

⚠️

gradle/libs.versions.toml#L18 - A newer version of androidx.lifecycle:lifecycle-runtime-ktx than 2.6.1 is available: 2.6.2

⚠️

gradle/libs.versions.toml#L96 - A newer version of androidx.compose.material3:material3 than 1.2.0-alpha06 is available: 1.2.0-alpha07

Generated by 🚫 dangerJS against 74d28ee

@sonarcloud
Copy link

sonarcloud bot commented Sep 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

@jonnyandrew
Copy link
Contributor Author

Merging without maestro tests passing as it seems to be a separate issue related to the login flow tests

@jonnyandrew jonnyandrew merged commit f214493 into develop Sep 7, 2023
16 of 19 checks passed
@jonnyandrew jonnyandrew deleted the jonny/integrate-rte branch September 7, 2023 15:21
@jonnyandrew jonnyandrew restored the jonny/integrate-rte branch September 8, 2023 11:58
@jonnyandrew jonnyandrew deleted the jonny/integrate-rte branch September 8, 2023 11:59
@RokeJulianLockhart
Copy link

Is Markdown to be added again later? Apologies if this is a stupid question, just confirming.

@jonnyandrew
Copy link
Contributor Author

@RokeJulianLockhart since this PR we have reintroduced markdown support. You can now go to advanced settings and turn the rich text editor off to enable markdown.

julioromano added a commit that referenced this pull request Sep 25, 2023
Has been changed in #1172 but in general method references should always preferred to lambdas in composable functions (because they have higher stability guarantees).
julioromano added a commit that referenced this pull request Sep 25, 2023
Has been changed in #1172 but in general method references should always be preferred to lambdas in composable functions (because they have higher stability guarantees).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Message is interpreted as markdown and cannot be escaped
4 participants