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

Add clip selector #2406

Merged
merged 14 commits into from
Jul 1, 2024
Merged

Add clip selector #2406

merged 14 commits into from
Jul 1, 2024

Conversation

MiSikora
Copy link
Contributor

@MiSikora MiSikora commented Jun 27, 2024

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:

  • Handle playback progress.
  • Manage the initial clip range depending on where the clip was started.
  • Implement analytics and logging.
  • Accessibility.

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 App doesn't handle direct episode links with timestamps #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

full.mp4

Scrolling

scroll-ezgif com-crop

Dragging

drag-ezgif com-crop

Double tapping

tap-ezgif com-crop

Zooming

zoom-ezgif com-crop

Checklist

  • If this is a user-facing change, I have added an entry in CHANGELOG.md
  • Ensure the linter passes (./gradlew spotlessApply to automatically apply formatting/linting)
  • I have considered whether it makes sense to add tests for my changes
  • All strings that need to be localized are in modules/services/localization/src/main/res/values/strings.xml
  • Any jetpack compose components I added or changed are covered by compose previews
  • I have updated (or requested that someone edit) the spreadsheet to reflect any new or changed analytics.

I have tested any UI changes...

  • with different themes
  • with a landscape orientation
  • with the device set to have a large display and font size
  • for accessibility with TalkBack

@MiSikora MiSikora added this to the 7.68 milestone Jun 27, 2024
@MiSikora MiSikora requested a review from a team as a code owner June 27, 2024 07:14
@MiSikora MiSikora requested review from ashiagr and removed request for a team June 27, 2024 07:14
@dangermattic
Copy link
Collaborator

dangermattic commented Jun 27, 2024

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

Generated by 🚫 Danger

@MiSikora MiSikora changed the title Task/clip selection Add clip selector Jun 27, 2024
Copy link
Contributor

@ashiagr ashiagr left a comment

Choose a reason for hiding this comment

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

This is so awesome!

I tested the following and enjoyed every part of the interaction 🌟

  • Scrolling ✅
  • Dragging clip box edges ✅
  • Double tap gesture ✅
  • Colors ✅
  • Landscape layout ✅
  • Remember the last state on orientation changes ✅
  • Large display ✅

Just a couple of findings:

  1. Grey box at the beginning:

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

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

@MiSikora
Copy link
Contributor Author

MiSikora commented Jul 1, 2024

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.

@MiSikora MiSikora merged commit de650e0 into main Jul 1, 2024
13 checks passed
@MiSikora MiSikora deleted the task/clip-selection branch July 1, 2024 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants