From c6d7d8fe39ae0f84051482961c3d2ad5ae64137a Mon Sep 17 00:00:00 2001 From: OxygenCobalt Date: Fri, 27 May 2022 14:31:48 -0600 Subject: [PATCH] playback: implement "safe" slider wrapper Implement a safe slider wrapper that does not crash with invalid values as often. Slider is a terrible component that is not designed with Auxio's use-case in the slightest. Instead of gracefully degrading with invalid values, it just crashes the entire app, which is horrible for UX. Since SeekBar is a useless buggy version-specific sh******ed mess too, we have no choice but to wrap Slider in a safe view layout that hopefully hacks with the input enough to not crash the app when doing simple seeking actions. I hate android so much. Resolves #140. --- CHANGELOG.md | 3 + .../java/org/oxycblt/auxio/music/Music.kt | 1 + .../org/oxycblt/auxio/music/MusicViewModel.kt | 4 + .../auxio/playback/PlaybackPanelFragment.kt | 50 ++----- .../oxycblt/auxio/playback/StyledSeekBar.kt | 125 ++++++++++++++++++ .../layout-land/fragment_playback_panel.xml | 36 +---- .../fragment_playback_panel.xml | 36 +---- .../fragment_playback_panel.xml | 36 +---- .../fragment_playback_panel.xml | 35 +---- .../res/layout/fragment_playback_panel.xml | 36 +---- app/src/main/res/layout/view_seek_bar.xml | 49 +++++++ app/src/main/res/values/dimens.xml | 1 + 12 files changed, 201 insertions(+), 211 deletions(-) create mode 100644 app/src/main/java/org/oxycblt/auxio/playback/StyledSeekBar.kt create mode 100644 app/src/main/res/layout/view_seek_bar.xml diff --git a/CHANGELOG.md b/CHANGELOG.md index 85908a00d..30bb7ce4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## dev [v2.3.1, v2.4.0, or v3.0.0] +#### What's Fixed +- Fixed crash when seeking to the end of a track as the track changed to a track with a lower duration + ## v2.3.0 #### What's New diff --git a/app/src/main/java/org/oxycblt/auxio/music/Music.kt b/app/src/main/java/org/oxycblt/auxio/music/Music.kt index ee14c26b6..65cf60be1 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/Music.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/Music.kt @@ -102,6 +102,7 @@ data class Song( val uri: Uri get() = ContentUris.withAppendedId(MediaStore.Audio.Media.EXTERNAL_CONTENT_URI, _mediaStoreId) + /** The duration of this song, in seconds (rounded down) */ val durationSecs: Long get() = durationMs / 1000 diff --git a/app/src/main/java/org/oxycblt/auxio/music/MusicViewModel.kt b/app/src/main/java/org/oxycblt/auxio/music/MusicViewModel.kt index ee0c8a701..1b7a08704 100644 --- a/app/src/main/java/org/oxycblt/auxio/music/MusicViewModel.kt +++ b/app/src/main/java/org/oxycblt/auxio/music/MusicViewModel.kt @@ -57,6 +57,10 @@ class MusicViewModel : ViewModel(), MusicStore.Callback { } } + /** + * Reload the music library. Note that this call will result in unexpected behavior in the case + * that music is reloaded after a loading process has already exceeded. + */ fun reloadMusic(context: Context) { logD("Reloading music library") _loaderResponse.value = null diff --git a/app/src/main/java/org/oxycblt/auxio/playback/PlaybackPanelFragment.kt b/app/src/main/java/org/oxycblt/auxio/playback/PlaybackPanelFragment.kt index c682f8889..cd13e6365 100644 --- a/app/src/main/java/org/oxycblt/auxio/playback/PlaybackPanelFragment.kt +++ b/app/src/main/java/org/oxycblt/auxio/playback/PlaybackPanelFragment.kt @@ -24,8 +24,6 @@ import androidx.appcompat.widget.Toolbar import androidx.core.view.updatePadding import androidx.fragment.app.Fragment import androidx.fragment.app.activityViewModels -import com.google.android.material.slider.Slider -import kotlin.math.max import org.oxycblt.auxio.R import org.oxycblt.auxio.databinding.FragmentPlaybackPanelBinding import org.oxycblt.auxio.music.MusicParent @@ -34,7 +32,6 @@ import org.oxycblt.auxio.playback.state.RepeatMode import org.oxycblt.auxio.ui.MainNavigationAction import org.oxycblt.auxio.ui.NavigationViewModel import org.oxycblt.auxio.ui.ViewBindingFragment -import org.oxycblt.auxio.util.formatDuration import org.oxycblt.auxio.util.logD import org.oxycblt.auxio.util.systemBarInsetsCompat import org.oxycblt.auxio.util.textSafe @@ -50,8 +47,7 @@ import org.oxycblt.auxio.util.textSafe */ class PlaybackPanelFragment : ViewBindingFragment(), - Slider.OnChangeListener, - Slider.OnSliderTouchListener, + StyledSeekBar.Callback, Toolbar.OnMenuItemClickListener { private val playbackModel: PlaybackViewModel by activityViewModels() private val navModel: NavigationViewModel by activityViewModels() @@ -93,10 +89,7 @@ class PlaybackPanelFragment : playbackModel.song.value?.let { navModel.exploreNavigateTo(it.album) } } - binding.playbackSeekBar.apply { - addOnChangeListener(this@PlaybackPanelFragment) - addOnSliderTouchListener(this@PlaybackPanelFragment) - } + binding.playbackSeekBar.callback = this binding.playbackRepeat.setOnClickListener { playbackModel.incrementRepeatMode() } binding.playbackSkipPrev.setOnClickListener { playbackModel.prev() } @@ -110,8 +103,6 @@ class PlaybackPanelFragment : binding.playbackSkipNext.setOnClickListener { playbackModel.next() } binding.playbackShuffle.setOnClickListener { playbackModel.invertShuffled() } - binding.playbackSeekBar.apply {} - // --- VIEWMODEL SETUP -- playbackModel.song.observe(viewLifecycleOwner, ::updateSong) @@ -133,8 +124,7 @@ class PlaybackPanelFragment : override fun onDestroyBinding(binding: FragmentPlaybackPanelBinding) { binding.playbackToolbar.setOnMenuItemClickListener(null) binding.playbackSong.isSelected = false - binding.playbackSeekBar.removeOnChangeListener(this) - binding.playbackSeekBar.removeOnChangeListener(this) + binding.playbackSeekBar.callback = null } override fun onMenuItemClick(item: MenuItem): Boolean { @@ -147,19 +137,8 @@ class PlaybackPanelFragment : } } - override fun onStartTrackingTouch(slider: Slider) { - requireBinding().playbackPosition.isActivated = true - } - - override fun onStopTrackingTouch(slider: Slider) { - requireBinding().playbackPosition.isActivated = false - playbackModel.seekTo(slider.value.toLong()) - } - - override fun onValueChange(slider: Slider, value: Float, fromUser: Boolean) { - if (fromUser) { - requireBinding().playbackPosition.textSafe = value.toLong().formatDuration(true) - } + override fun seekTo(positionSecs: Long) { + playbackModel.seekTo(positionSecs) } private fun updateSong(song: Song?) { @@ -171,14 +150,7 @@ class PlaybackPanelFragment : binding.playbackSong.textSafe = song.resolveName(context) binding.playbackArtist.textSafe = song.resolveIndividualArtistName(context) binding.playbackAlbum.textSafe = song.album.resolveName(context) - - // Normally if a song had a duration - val seconds = song.durationSecs - binding.playbackDuration.textSafe = seconds.formatDuration(false) - binding.playbackSeekBar.apply { - isEnabled = seconds > 0L - valueTo = max(seconds, 1L).toFloat() - } + binding.playbackSeekBar.durationSecs = song.durationSecs } private fun updateParent(parent: MusicParent?) { @@ -186,14 +158,8 @@ class PlaybackPanelFragment : parent?.resolveName(requireContext()) ?: getString(R.string.lbl_all_songs) } - private fun updatePosition(position: Long) { - // Don't update the progress while we are seeking, that will make the SeekBar jump - // around. - val binding = requireBinding() - if (!binding.playbackPosition.isActivated) { - binding.playbackSeekBar.value = position.toFloat() - binding.playbackPosition.textSafe = position.formatDuration(true) - } + private fun updatePosition(positionSecs: Long) { + requireBinding().playbackSeekBar.positionSecs = positionSecs } private fun updateRepeat(repeatMode: RepeatMode) { diff --git a/app/src/main/java/org/oxycblt/auxio/playback/StyledSeekBar.kt b/app/src/main/java/org/oxycblt/auxio/playback/StyledSeekBar.kt new file mode 100644 index 000000000..0754d4e92 --- /dev/null +++ b/app/src/main/java/org/oxycblt/auxio/playback/StyledSeekBar.kt @@ -0,0 +1,125 @@ +/* + * Copyright (c) 2022 Auxio Project + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +package org.oxycblt.auxio.playback + +import android.content.Context +import android.util.AttributeSet +import android.widget.FrameLayout +import com.google.android.material.slider.Slider +import kotlin.math.max +import org.oxycblt.auxio.databinding.ViewSeekBarBinding +import org.oxycblt.auxio.util.formatDuration +import org.oxycblt.auxio.util.inflater +import org.oxycblt.auxio.util.logD +import org.oxycblt.auxio.util.textSafe + +/** + * A wrapper around [Slider] that shows not only position and duration values, but also basically + * hacks in behavior consistent with a normal SeekBar in a way that will not crash the app. + * + * SeekBar, like most android OS components, is a version-specific mess that requires constant + * hacks on older versions. Instead, we use the more "modern" slider component, but it is not + * designed for the job that Auxio's progress bar has. It does not gracefully degrade when + * positions don't make sense (which happens incredibly often), it just crashes the entire app, + * which is insane but also checks out for something more meant for configuration than seeking. + * + * Instead, we wrap it in a safe class that hopefully implements enough safety to not crash the + * app or result in blatantly janky behavior. Mostly. + * + * @author OxygenCobalt + */ +class StyledSeekBar +@JvmOverloads +constructor( + context: Context, + attrs: AttributeSet? = null, + defStyleAttr: Int = 0, +) : + FrameLayout(context, attrs, defStyleAttr), + Slider.OnSliderTouchListener, + Slider.OnChangeListener { + private val binding = ViewSeekBarBinding.inflate(context.inflater, this, true) + + init { + binding.seekBarSlider.addOnSliderTouchListener(this) + binding.seekBarSlider.addOnChangeListener(this) + } + + var callback: Callback? = null + + /** + * The current position, in seconds. This is the current value of the SeekBar and is indicated + * by the start TextView in the layout. + */ + var positionSecs: Long + get() = binding.seekBarSlider.value.toLong() + set(value) { + // Sanity check: Ensure that this value is within the duration and will not crash + // the app, and that the user is not currently seeking (which would cause the SeekBar + // to jump around). + if (value <= durationSecs && !isActivated) { + binding.seekBarSlider.value = value.toFloat() + } + } + + /** + * The current duration, in seconds. This is the end value of the SeekBar and is indicated + * by the end TextView in the layout. + */ + var durationSecs: Long + get() = binding.seekBarSlider.valueTo.toLong() + set(value) { + // Sanity check 1: If this is a value so low that it effectively rounds down to + // zero, use 1 instead and disable the SeekBar. + val to = max(value, 1) + isEnabled = value > 0 + + // Sanity check 2: If the current value exceeds the new duration value, clamp it + // down so that we don't crash and instead have an annoying visual flicker. + if (positionSecs > to) { + binding.seekBarSlider.value = to.toFloat() + } + + binding.seekBarSlider.valueTo = to.toFloat() + binding.seekBarDuration.textSafe = value.formatDuration(false) + } + + override fun onStartTrackingTouch(slider: Slider) { + // User has begun seeking, place the SeekBar into a "Suspended" mode in which no + // position updates are sent and is indicated by the position value turning accented. + isActivated = true + } + + override fun onStopTrackingTouch(slider: Slider) { + // End of seek event, send off new value to callback. + isActivated = false + callback?.seekTo(slider.value.toLong()) + } + + override fun onValueChange(slider: Slider, value: Float, fromUser: Boolean) { + binding.seekBarPosition.textSafe = value.toLong().formatDuration(true) + } + + interface Callback { + /** + * Called when a seek event was completed and the new position must be seeked to by + * the app. + */ + fun seekTo(positionSecs: Long) + } +} diff --git a/app/src/main/res/layout-land/fragment_playback_panel.xml b/app/src/main/res/layout-land/fragment_playback_panel.xml index 6fbfc8df2..16d07e073 100644 --- a/app/src/main/res/layout-land/fragment_playback_panel.xml +++ b/app/src/main/res/layout-land/fragment_playback_panel.xml @@ -77,46 +77,14 @@ app:layout_constraintTop_toBottomOf="@+id/playback_artist" tools:text="Album Name" /> - - - - - + app:layout_constraintStart_toStartOf="parent" /> - - - - - + app:layout_constraintTop_toBottomOf="@+id/playback_album" /> - - - - - + app:layout_constraintStart_toStartOf="parent" /> - - - - - + app:layout_constraintTop_toBottomOf="@+id/playback_album" /> - - - - - + app:layout_constraintStart_toStartOf="parent" /> + + + + + + + + + \ No newline at end of file diff --git a/app/src/main/res/values/dimens.xml b/app/src/main/res/values/dimens.xml index 883792cd1..81b2dc3cb 100644 --- a/app/src/main/res/values/dimens.xml +++ b/app/src/main/res/values/dimens.xml @@ -1,6 +1,7 @@ + 4dp 8dp 16dp 24dp