Fix some issues in the new timeline renderer - #7751
Conversation
ParagraphView follow LocalTextStyle.current|
📱 Scan the QR code below to install the build (arm64 only) for this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7751 +/- ##
===========================================
- Coverage 81.11% 81.09% -0.02%
===========================================
Files 2797 2797
Lines 82992 83006 +14
Branches 11437 11440 +3
===========================================
Hits 67316 67316
- Misses 11273 11287 +14
Partials 4403 4403 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This should make them look like they did in the previous renderer
… url annotation is clicked or long clicked `detectTapGestures` always consumes the touch events, no matter if they are handled or not, blocking these events from reaching parent components. Should fix the issue of `ParagraphView` blocking the long press action in the message bubbles
0be2b99 to
bf5af4b
Compare
There was a problem hiding this comment.
The composer is all kinds of broken in the Added here. Race/timing issue?
There was a problem hiding this comment.
In fact, quite a few of the screenshots are like this either before or after. Weird.
There was a problem hiding this comment.
It's also happening in other PRs too https://github.com/element-hq/element-x-android/pull/7759/changes#diff-8213ffc39b625c193fa8d085a94513113d7d735cb284b88fe1f46037343dd626
There was a problem hiding this comment.
This is a problem with recorded screenshots since the last version of paparazzi, which we can't downgrade. It's sadly expected.
|
|
||
| // Previews and screenshots can't render the fading edge gradients properly, everything is obscured by them, so we skip them in the preview mode. | ||
| if (LocalInspectionMode.current.not()) { | ||
| if (LocalInspectionMode.current.not() && scrollState.maxValue > 0) { |
There was a problem hiding this comment.
Should we not use canScrollForward || canScrollBackward instead?
There was a problem hiding this comment.
I could have sworn last time I tried to use that it didn't work. Maybe the problem was in the previews, but it does work well in an actual device. Will change it.
| }, | ||
| onLongPress = { offset -> | ||
| val (url, urlText) = layoutResult.value?.urlAt(offset, text) ?: return@detectTapGestures | ||
| awaitEachGesture { |
There was a problem hiding this comment.
Why are we moving away from detectTapGestures? I'm not familiar enough with the api, but detectTapGesture is using a loop, here I think we'll mostly drop relevant events? Have you tried on a regular device, not emulator?
There was a problem hiding this comment.
Why are we moving away from detectTapGestures?
Because detectTapGestures consumes the events and any touch events that should go to the message bubble are ignored.
detectTapGesture is using a loop, here I think we'll mostly drop relevant events?
I thought so too, but it's working fine. Since we just want to check down -> up, it seems to work. Maybe it won't if there is a move, but I think we might want to ignore those. I can double check.
Have you tried on a regular device, not emulator?
Yes, in fact I only tested it in my test device, I didn't even use the emulator.
| if (up.pressed) return@withTimeout null | ||
| up.consume() | ||
|
|
||
| up.uptimeMillis - down.uptimeMillis > longPressTimeout |
There was a problem hiding this comment.
It's probably deadcode because of the withTimeout(longPressTimeout)
There was a problem hiding this comment.
Hmm, true. At first I wanted to trigger this just on the up event, but that led to some weird interactions, so I added the timeout, but having the exact same value as the check before it'll just ignore the up event.
…g edges in the code blocks
|
ganfra
left a comment
There was a problem hiding this comment.
Ok let's merge this and continue to iterate!



Content
Fixes some reported issues about the new timeline renderer:
ParagraphViewhaving a smaller size than the rest of the timeline contents.ParagraphViewnot passing the events to the parent, so long pressing without a URL annotation below had no effect in the message bubble.Motivation and context
These issues were reported by @frebib .
Screenshots / GIFs
In the PR.
Tests
Enable the feature flag and check these work as expected, I guess?
Tested devices
Checklist