From 962262d56ceacc4e2d186cf9cbb39815600b1f42 Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Wed, 24 Jun 2020 16:37:14 -0400 Subject: [PATCH] Fix MediaCarousel in RTL Fixes: 158025546 Test: manual - launch 2 media players & try every combination of interaction Change-Id: I14cc152c46b99f712010e8cf7afe57dad2c5ba85 --- .../systemui/media/MediaCarouselController.kt | 46 ++++++-- .../media/MediaCarouselScrollHandler.kt | 100 ++++++++++++------ .../android/systemui/media/MediaScrollView.kt | 45 ++++++-- 3 files changed, 142 insertions(+), 49 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt index e2215d57a0946..68625059b2ef6 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt @@ -2,6 +2,7 @@ package com.android.systemui.media import android.content.Context import android.content.Intent +import android.content.res.Configuration import android.graphics.Color import android.provider.Settings.ACTION_MEDIA_CONTROLS_SETTINGS import android.view.LayoutInflater @@ -96,7 +97,6 @@ class MediaCarouselController @Inject constructor( * The measured height of the carousel */ private var carouselMeasureHeight: Int = 0 - private var playerWidthPlusPadding: Int = 0 private var desiredHostState: MediaHostState? = null private val mediaCarousel: MediaScrollView private val mediaCarouselScrollHandler: MediaCarouselScrollHandler @@ -108,6 +108,15 @@ class MediaCarouselController @Inject constructor( private val pageIndicator: PageIndicator private val visualStabilityCallback: VisualStabilityManager.Callback private var needsReordering: Boolean = false + private var isRtl: Boolean = false + set(value) { + if (value != field) { + field = value + mediaFrame.layoutDirection = + if (value) View.LAYOUT_DIRECTION_RTL else View.LAYOUT_DIRECTION_LTR + mediaCarouselScrollHandler.scrollToStart() + } + } private var currentlyExpanded = true set(value) { if (field != value) { @@ -126,6 +135,11 @@ class MediaCarouselController @Inject constructor( override fun onOverlayChanged() { inflateSettingsButton() } + + override fun onConfigChanged(newConfig: Configuration?) { + if (newConfig == null) return + isRtl = newConfig.layoutDirection == View.LAYOUT_DIRECTION_RTL + } } init { @@ -135,6 +149,7 @@ class MediaCarouselController @Inject constructor( mediaCarouselScrollHandler = MediaCarouselScrollHandler(mediaCarousel, pageIndicator, executor, mediaDataManager::onSwipeToDismiss, this::updatePageIndicatorLocation, falsingManager) + isRtl = context.resources.configuration.layoutDirection == View.LAYOUT_DIRECTION_RTL inflateSettingsButton() mediaContent = mediaCarousel.requireViewById(R.id.media_carousel) configurationController.addCallback(configListener) @@ -144,7 +159,7 @@ class MediaCarouselController @Inject constructor( reorderAllPlayers() } // Let's reset our scroll position - mediaCarousel.scrollX = 0 + mediaCarouselScrollHandler.scrollToStart() } visualStabilityManager.addReorderingAllowedCallback(visualStabilityCallback, true /* persistent */) @@ -196,8 +211,13 @@ class MediaCarouselController @Inject constructor( } private fun inflateMediaCarousel(): ViewGroup { - return LayoutInflater.from(context).inflate(R.layout.media_carousel, + val mediaCarousel = LayoutInflater.from(context).inflate(R.layout.media_carousel, UniqueObjectHostView(context), false) as ViewGroup + // Because this is inflated when not attached to the true view hierarchy, it resolves some + // potential issues to force that the layout direction is defined by the locale + // (rather than inherited from the parent, which would resolve to LTR when unattached). + mediaCarousel.layoutDirection = View.LAYOUT_DIRECTION_LOCALE + return mediaCarousel } private fun reorderAllPlayers() { @@ -313,8 +333,12 @@ class MediaCarouselController @Inject constructor( private fun updatePageIndicatorLocation() { // Update the location of the page indicator, carousel clipping - pageIndicator.translationX = (currentCarouselWidth - pageIndicator.width) / 2.0f + - mediaCarouselScrollHandler.contentTranslation + val translationX = if (isRtl) { + (pageIndicator.width - currentCarouselWidth) / 2.0f + } else { + (currentCarouselWidth - pageIndicator.width) / 2.0f + } + pageIndicator.translationX = translationX + mediaCarouselScrollHandler.contentTranslation val layoutParams = pageIndicator.layoutParams as ViewGroup.MarginLayoutParams pageIndicator.translationY = (currentCarouselHeight - pageIndicator.height - layoutParams.bottomMargin).toFloat() @@ -334,7 +358,8 @@ class MediaCarouselController @Inject constructor( if (width != currentCarouselWidth || height != currentCarouselHeight) { currentCarouselWidth = width currentCarouselHeight = height - mediaCarouselScrollHandler.setCarouselBounds(currentCarouselWidth, currentCarouselHeight) + mediaCarouselScrollHandler.setCarouselBounds( + currentCarouselWidth, currentCarouselHeight) updatePageIndicatorLocation() } } @@ -348,7 +373,7 @@ class MediaCarouselController @Inject constructor( if (currentlyShowingOnlyActive != endShowsActive || ((currentTransitionProgress != 1.0f && currentTransitionProgress != 0.0f) && startShowsActive != endShowsActive)) { - /// Whenever we're transitioning from between differing states or the endstate differs + // Whenever we're transitioning from between differing states or the endstate differs // we reset the translation currentlyShowingOnlyActive = endShowsActive mediaCarouselScrollHandler.resetTranslation(animate = true) @@ -416,14 +441,15 @@ class MediaCarouselController @Inject constructor( height != carouselMeasureWidth && height != 0) { carouselMeasureWidth = width carouselMeasureHeight = height - playerWidthPlusPadding = carouselMeasureWidth + context.resources.getDimensionPixelSize( - R.dimen.qs_media_padding) - mediaCarouselScrollHandler.playerWidthPlusPadding = playerWidthPlusPadding + val playerWidthPlusPadding = carouselMeasureWidth + + context.resources.getDimensionPixelSize(R.dimen.qs_media_padding) // Let's remeasure the carousel val widthSpec = desiredHostState?.measurementInput?.widthMeasureSpec ?: 0 val heightSpec = desiredHostState?.measurementInput?.heightMeasureSpec ?: 0 mediaCarousel.measure(widthSpec, heightSpec) mediaCarousel.layout(0, 0, width, mediaCarousel.measuredHeight) + // Update the padding after layout; view widths are used in RTL to calculate scrollX + mediaCarouselScrollHandler.playerWidthPlusPadding = playerWidthPlusPadding } } } diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselScrollHandler.kt b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselScrollHandler.kt index 993c05fbbd6f2..ef2f71100e1a6 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselScrollHandler.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselScrollHandler.kt @@ -58,6 +58,10 @@ class MediaCarouselScrollHandler( private var translationChangedListener: () -> Unit, private val falsingManager: FalsingManager ) { + /** + * Is the view in RTL + */ + val isRtl: Boolean get() = scrollView.isLayoutRtl /** * Do we need falsing protection? */ @@ -121,14 +125,14 @@ class MediaCarouselScrollHandler( field = value // The player width has changed, let's update the scroll position to make sure // it's still at the same place - var newScroll = activeMediaIndex * playerWidthPlusPadding + var newRelativeScroll = activeMediaIndex * playerWidthPlusPadding if (scrollIntoCurrentMedia > playerWidthPlusPadding) { - newScroll += playerWidthPlusPadding - + newRelativeScroll += playerWidthPlusPadding - (scrollIntoCurrentMedia - playerWidthPlusPadding) } else { - newScroll += scrollIntoCurrentMedia + newRelativeScroll += scrollIntoCurrentMedia } - scrollView.scrollX = newScroll + scrollView.relativeScrollX = newRelativeScroll } /** @@ -184,8 +188,9 @@ class MediaCarouselScrollHandler( if (playerWidthPlusPadding == 0) { return } - onMediaScrollingChanged(scrollX / playerWidthPlusPadding, - scrollX % playerWidthPlusPadding) + val relativeScrollX = scrollView.relativeScrollX + onMediaScrollingChanged(relativeScrollX / playerWidthPlusPadding, + relativeScrollX % playerWidthPlusPadding) } } @@ -222,11 +227,19 @@ class MediaCarouselScrollHandler( Math.abs(contentTranslation)) val settingsTranslation = (1.0f - settingsOffset) * -settingsButton.width * SETTINGS_BUTTON_TRANSLATION_FRACTION - val newTranslationX: Float - if (contentTranslation > 0) { - newTranslationX = settingsTranslation + val newTranslationX = if (isRtl) { + // In RTL, the 0-placement is on the right side of the view, not the left... + if (contentTranslation > 0) { + -(scrollView.width - settingsTranslation - settingsButton.width) + } else { + -settingsTranslation + } } else { - newTranslationX = scrollView.width - settingsTranslation - settingsButton.width + if (contentTranslation > 0) { + settingsTranslation + } else { + scrollView.width - settingsTranslation - settingsButton.width + } } val rotation = (1.0f - settingsOffset) * 50 settingsButton.rotation = rotation * -Math.signum(contentTranslation) @@ -259,26 +272,26 @@ class MediaCarouselScrollHandler( } if (isUp || motionEvent.action == MotionEvent.ACTION_CANCEL) { // It's an up and the fling didn't take it above - val pos = scrollView.scrollX % playerWidthPlusPadding - val scollXAmount: Int - if (pos > playerWidthPlusPadding / 2) { - scollXAmount = playerWidthPlusPadding - pos + val relativePos = scrollView.relativeScrollX % playerWidthPlusPadding + val scrollXAmount: Int + if (relativePos > playerWidthPlusPadding / 2) { + scrollXAmount = playerWidthPlusPadding - relativePos } else { - scollXAmount = -1 * pos + scrollXAmount = -1 * relativePos } - if (scollXAmount != 0) { + if (scrollXAmount != 0) { // Delay the scrolling since scrollView calls springback which cancels // the animation again.. mainExecutor.execute { - scrollView.smoothScrollBy(scollXAmount, 0) + scrollView.smoothScrollBy(if (isRtl) -scrollXAmount else scrollXAmount, 0) } } val currentTranslation = scrollView.getContentTranslation() if (currentTranslation != 0.0f) { // We started a Swipe but didn't end up with a fling. Let's either go to the // dismissed position or go back. - val springBack = Math.abs(currentTranslation) < getMaxTranslation() / 2 - || isFalseTouch() + val springBack = Math.abs(currentTranslation) < getMaxTranslation() / 2 || + isFalseTouch() val newTranslation: Float if (springBack) { newTranslation = 0.0f @@ -313,9 +326,11 @@ class MediaCarouselScrollHandler( return gestureDetector.onTouchEvent(motionEvent) } - fun onScroll(down: MotionEvent, - lastMotion: MotionEvent, - distanceX: Float): Boolean { + fun onScroll( + down: MotionEvent, + lastMotion: MotionEvent, + distanceX: Float + ): Boolean { val totalX = lastMotion.x - down.x val currentTranslation = scrollView.getContentTranslation() if (currentTranslation != 0.0f || @@ -339,8 +354,8 @@ class MediaCarouselScrollHandler( } // Otherwise we don't have do do anything, and will remove the unrubberbanded // translation } - if (Math.signum(newTranslation) != Math.signum(currentTranslation) - && currentTranslation != 0.0f) { + if (Math.signum(newTranslation) != Math.signum(currentTranslation) && + currentTranslation != 0.0f) { // We crossed the 0.0 threshold of the translation. Let's see if we're allowed // to scroll into the new direction if (scrollView.canScrollHorizontally(-newTranslation.toInt())) { @@ -394,9 +409,10 @@ class MediaCarouselScrollHandler( scrollView.animationTargetX = newTranslation } else { // We're flinging the player! Let's go either to the previous or to the next player - val pos = scrollView.scrollX + val pos = scrollView.relativeScrollX val currentIndex = if (playerWidthPlusPadding > 0) pos / playerWidthPlusPadding else 0 - var destIndex = if (vX <= 0) currentIndex + 1 else currentIndex + val flungTowardEnd = if (isRtl) vX > 0 else vX < 0 + var destIndex = if (flungTowardEnd) currentIndex + 1 else currentIndex destIndex = Math.max(0, destIndex) destIndex = Math.min(mediaContent.getChildCount() - 1, destIndex) val view = mediaContent.getChildAt(destIndex) @@ -438,8 +454,14 @@ class MediaCarouselScrollHandler( activeMediaIndex = newIndex updatePlayerVisibilities() } - val location = activeMediaIndex.toFloat() + if (playerWidthPlusPadding > 0) + val relativeLocation = activeMediaIndex.toFloat() + if (playerWidthPlusPadding > 0) scrollInAmount.toFloat() / playerWidthPlusPadding else 0f + // Fix the location, because PageIndicator does not handle RTL internally + val location = if (isRtl) { + mediaContent.childCount - relativeLocation - 1 + } else { + relativeLocation + } pageIndicator.setLocation(location) updateClipToOutline() } @@ -480,13 +502,20 @@ class MediaCarouselScrollHandler( * where it was and update our scroll position. */ fun onPrePlayerRemoved(removed: MediaControlPanel) { - val beforeActive = mediaContent.indexOfChild(removed.view?.player) <= activeMediaIndex + val removedIndex = mediaContent.indexOfChild(removed.view?.player) + // If the removed index is less than the activeMediaIndex, then we need to decrement it. + // RTL has no effect on this, because indices are always relative (start-to-end). + // Update the index 'manually' since we won't always get a call to onMediaScrollingChanged + val beforeActive = removedIndex <= activeMediaIndex if (beforeActive) { - // also update the index here since the scroll below might not always lead - // to a scrolling changed activeMediaIndex = Math.max(0, activeMediaIndex - 1) - scrollView.scrollX = Math.max(scrollView.scrollX - - playerWidthPlusPadding, 0) + } + // If the removed media item is "left of" the active one (in an absolute sense), we need to + // scroll the view to keep that player in view. This is because scroll position is always + // calculated from left to right. + val leftOfActive = if (isRtl) !beforeActive else beforeActive + if (leftOfActive) { + scrollView.scrollX = Math.max(scrollView.scrollX - playerWidthPlusPadding, 0) } } @@ -501,6 +530,13 @@ class MediaCarouselScrollHandler( } } + /** + * Reset the MediaScrollView to the start. + */ + fun scrollToStart() { + scrollView.relativeScrollX = 0 + } + companion object { private val CONTENT_TRANSLATION = object : FloatPropertyCompat( "contentTranslation") { diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaScrollView.kt b/packages/SystemUI/src/com/android/systemui/media/MediaScrollView.kt index a079b06a0b10c..b8872250bb6cb 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaScrollView.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaScrollView.kt @@ -15,7 +15,10 @@ import com.android.systemui.util.animation.physicsAnimator * when only measuring children but not the parent, when trying to apply a new scroll position */ class MediaScrollView @JvmOverloads constructor( - context: Context, attrs: AttributeSet? = null, defStyleAttr: Int = 0) + context: Context, + attrs: AttributeSet? = null, + defStyleAttr: Int = 0 +) : HorizontalScrollView(context, attrs, defStyleAttr) { lateinit var contentContainer: ViewGroup @@ -37,6 +40,26 @@ class MediaScrollView @JvmOverloads constructor( contentContainer.translationX } + /** + * Convert between the absolute (left-to-right) and relative (start-to-end) scrollX of the media + * carousel. The player indices are always relative (start-to-end) and the scrollView.scrollX + * is always absolute. This function is its own inverse. + */ + private fun transformScrollX(scrollX: Int): Int = if (isLayoutRtl) { + contentContainer.width - width - scrollX + } else { + scrollX + } + + /** + * Get the layoutDirection-relative (start-to-end) scroll X position of the carousel. + */ + var relativeScrollX: Int + get() = transformScrollX(scrollX) + set(value) { + scrollX = transformScrollX(value) + } + /** * Allow all scrolls to go through, use base implementation */ @@ -55,15 +78,15 @@ class MediaScrollView @JvmOverloads constructor( } override fun onInterceptTouchEvent(ev: MotionEvent?): Boolean { - var intercept = false; + var intercept = false touchListener?.let { intercept = it.onInterceptTouchEvent(ev) } - return super.onInterceptTouchEvent(ev) || intercept; + return super.onInterceptTouchEvent(ev) || intercept } override fun onTouchEvent(ev: MotionEvent?): Boolean { - var touch = false; + var touch = false touchListener?.let { touch = it.onTouchEvent(ev) } @@ -75,9 +98,17 @@ class MediaScrollView @JvmOverloads constructor( contentContainer = getChildAt(0) as ViewGroup } - override fun overScrollBy(deltaX: Int, deltaY: Int, scrollX: Int, scrollY: Int, - scrollRangeX: Int, scrollRangeY: Int, maxOverScrollX: Int, - maxOverScrollY: Int, isTouchEvent: Boolean): Boolean { + override fun overScrollBy( + deltaX: Int, + deltaY: Int, + scrollX: Int, + scrollY: Int, + scrollRangeX: Int, + scrollRangeY: Int, + maxOverScrollX: Int, + maxOverScrollY: Int, + isTouchEvent: Boolean + ): Boolean { if (getContentTranslation() != 0.0f) { // When we're dismissing we ignore all the scrolling return false