Skip to content
This repository has been archived by the owner on Sep 27, 2024. It is now read-only.

[Android] Add support for subcomposition and sharing state #822

Merged
merged 4 commits into from
Sep 26, 2023

Conversation

jonnyandrew
Copy link
Contributor

@jonnyandrew jonnyandrew commented Sep 22, 2023

Changes

  • Add support for subcomposition. By adding a new a mode that doesn't register any state updates, the editor can be composed purely for measurement purposes and not trigger a state update.
  • Add support for sharing state between editors at different call-sites; allow the editor to be constructed at a new call site without side effects. The AndroidView factory and update blocks can be called again by the replacement editor instance safely.
  • Preserve trailing new lines on restoring state by saving and restoring the internal HTML representation of the editor rather than the message HTML.
  • Make the state observable so that, when an editor leaves the composition, it doesn't need to clean up its reference from the state and cause unwanted state updates.
  • Fix some issues with unstable classes in the state.
  • Update some compose dependencies including compose compiler.

Context

To enable the editor to be embedded in complex layouts that require pre-measurement and/or multiple layout trees.

@jonnyandrew jonnyandrew requested a review from a team September 25, 2023 15:21
@jonnyandrew jonnyandrew marked this pull request as ready for review September 26, 2023 06:59
Copy link

@julioromano julioromano left a comment

Choose a reason for hiding this comment

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

Just a few nits hoping they area useful. Sorry for not being more of help here.

Although I don't understand what's the reason for switching off registerStateUpdates in a subcomposition. Without state updates the RTE is pretty much useless isn't it?

@jonnyandrew
Copy link
Contributor Author

jonnyandrew commented Sep 26, 2023

Although I don't understand what's the reason for switching off registerStateUpdates in a subcomposition. Without state updates the RTE is pretty much useless isn't it?

In a subcomposition the editor is composed twice, once for measurement and once again to be placed in the layout given the first measurement. During the first measurement I need the editor to just be a dumb view and read the state but not try to update it in any way that might impact the second step. Hope that makes sense?

An alternative could be to create a new instance of the state for every subcomposition but we'd also have the issue of the first composition requesting focus each frame, which I'm not sure how to avoid.

@codecov-commenter
Copy link

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (7fa6e63) 87.06% compared to head (36e44f7) 89.88%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #822      +/-   ##
============================================
+ Coverage     87.06%   89.88%   +2.82%     
============================================
  Files           155       82      -73     
  Lines         17983    14725    -3258     
  Branches        901        0     -901     
============================================
- Hits          15656    13236    -2420     
+ Misses         2068     1489     -579     
+ Partials        259        0     -259     
Flag Coverage Δ
uitests ?
uitests-android ?
unittests 89.88% <ø> (+3.36%) ⬆️
unittests-android ?
unittests-ios ?
unittests-rust 89.88% <ø> (ø)

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

see 73 files with indirect coverage changes

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

@sonarcloud
Copy link

sonarcloud bot commented Sep 26, 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
No Duplication information No Duplication information

@jonnyandrew jonnyandrew merged commit 861ea18 into main Sep 26, 2023
8 checks passed
@jonnyandrew jonnyandrew deleted the jonny/subcompose-support-rebase branch September 26, 2023 10:46
@julioromano
Copy link

Although I don't understand what's the reason for switching off registerStateUpdates in a subcomposition. Without state updates the RTE is pretty much useless isn't it?

In a subcomposition the editor is composed twice, once for measurement and once again to be placed in the layout given the first measurement. During the first measurement I need the editor to just be a dumb view and read the state but not try to update it in any way that might impact the second step. Hope that makes sense?

An alternative could be to create a new instance of the state for every subcomposition but we'd also have the issue of the first composition requesting focus each frame, which I'm not sure how to avoid.

I still did not understand this completely. Can you point me to a piece of code that shows this? (using the RTE in a subcomp).

@jonnyandrew
Copy link
Contributor Author

Sure, my draft work for full screen mode in Element X shows this in action: vector-im/element-x-android/jonny/rte-full-screen

jonnyandrew added a commit to element-hq/element-x-android that referenced this pull request Sep 29, 2023
- Add full screen mode for the rich text editor (RTE). When text formatting options are enabled, the editor can be dragged to full screen.
- Remove `ConstraintLayout` from `textcomposer` module, now made much simpler now the RTE supports being called in multiple layouts matrix-org/matrix-rich-text-editor#822

- Part of element-hq/element-meta#1973
- Includes design from #1315
- Fixes #1293 (through new layout)
- Fixes #1394 (through inclusion of matrix-org/matrix-rich-text-editor#824)
- Fixes #1259 (through inclusion of matrix-org/matrix-rich-text-editor#820)

---------

Co-authored-by: ElementBot <benoitm+elementbot@element.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants