Automattic / pocket-casts-android

Pocket Casts Android 🎧
Mozilla Public License 2.0
2.53k stars 209 forks source link

Add clip selector #2406

Closed MiSikora closed 4 days ago

MiSikora commented 1 week ago

Description

This PR adds clip selection to clip sharing. After several iterations, I ultimately used LazyRow as the UI element for the timeline. Initially, I experimented with SubcomposeLayout, but I soon realized that I would be implementing something very similar to LazyRow, which would likely be not as optimized. Although it might be possible to create a more efficient custom layout, I am satisfied with the current design.

The UI is divided into two main components: ClipTimeline and ClipBox. ClipTimeline is responsible for displaying ticks, while ClipBox handles the clipping window. They communicate via ClipSelectorState, which is remembered in the ShareClipPage, a higher-level component. This setup preserves the state of zoom, scroll offset, etc., during configuration changes. The state also manages the translation between pixels and duration, and vice versa.

Double-tapping is implemented in ClipTimeline instead of ClipBox for a key reason. Adding it to ClipBox makes ClipTimeline unresponsive to scroll events. While this could be addressed by adding another .pointerInput() that delegates to scroll state, my testing found it a bit janky. For example, it wasn't always 100% responsive, and simple delegation lost nice features like flings or bounce animations.

The zoom gesture is somewhat problematic, but I don't know if we can improve it. I'm currently delegating to .transformable(), which handles zooming. The issue seems to stem from LazyRow also supporting scrolling, which interferes with zooming.

Designs: Figma

Next steps:

Testing Instructions

  1. Start clip sharing.
  2. Verify that the clip window follows the timeline scroll position.
  3. Ensure you can drag handles on the timeline.
  4. Check that double-tapping on the timeline moves handles to the tapped position, moving the nearest handle.
  5. Confirm that you can zoom in and out on the timeline. The clip box should stay locked to the same time marks when zooming. If an episode is very short, like a couple of seconds, zooming out is intentionally impossible to prevent very narrow timelines displayed.
  6. Share the clip and verify that the timestamps are correct. Note that we support 1-second resolution, even though the UI might suggest otherwise. This can be adjusted in the future. Be aware that you won't be able to open this link in the app due to issue #2405. You can test them in the web.
  7. Check if the box and timeline state is preserved during configuration changes, such as device rotation.

Screenshots or Screencast

Demo

https://github.com/Automattic/pocket-casts-android/assets/30936061/c19689fb-e786-4cc6-92f5-02bc462ff806

Scrolling

scroll-ezgif com-crop

Dragging

drag-ezgif com-crop

Double tapping

tap-ezgif com-crop

Zooming

zoom-ezgif com-crop

Checklist

I have tested any UI changes...

dangermattic commented 1 week ago
2 Warnings
:warning: This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
:warning: Class ClipSelectorState is missing tests, but unit-tests-exemption label was set to ignore this.

Generated by :no_entry_sign: Danger

MiSikora commented 5 days ago

When the clip box edge is moved to the start of the timeline, and timeline view is scrolled, I see a small grey box:

Yes, this is expected. The start handle is offset so that its end position aligns with a time mark. When you scroll to the left, the handle moves out of the screen, but the connecting frame remains visible. If the frame were clipped to the timeline edge, there would be an empty space between the handle and the frame when you start scrolling.

In Figma, I can see the current position in the timeline view, do we plan to add it in a later PR?

Yes, I plan to address it in one of the next PRs.