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

[Reader Customization] Allow dragging to close Preferences bottom sheet #20640

Merged
merged 5 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package org.wordpress.android.ui.reader

import android.app.Dialog
import android.content.DialogInterface
import android.graphics.Color
import android.os.Bundle
import android.view.LayoutInflater
import android.view.View
Expand All @@ -14,6 +15,7 @@ import androidx.compose.ui.platform.ComposeView
import androidx.fragment.app.FragmentManager
import androidx.fragment.app.viewModels
import androidx.lifecycle.lifecycleScope
import com.google.android.material.bottomsheet.BottomSheetBehavior
import com.google.android.material.bottomsheet.BottomSheetDialog
import com.google.android.material.bottomsheet.BottomSheetDialogFragment
import dagger.hilt.android.AndroidEntryPoint
Expand Down Expand Up @@ -61,7 +63,7 @@ class ReaderReadingPreferencesDialogFragment : BottomSheetDialogFragment() {
val isFeedbackEnabled by viewModel.isFeedbackEnabled.collectAsState()
ReadingPreferencesScreen(
currentReadingPreferences = readerPreferences,
onCloseClick = viewModel::saveReadingPreferencesAndClose,
onCloseClick = viewModel::onExitActionClick,
onSendFeedbackClick = viewModel::onSendFeedbackClick,
onThemeClick = viewModel::onThemeClick,
onFontFamilyClick = viewModel::onFontFamilyClick,
Expand All @@ -81,33 +83,53 @@ class ReaderReadingPreferencesDialogFragment : BottomSheetDialogFragment() {

override fun onCreateDialog(savedInstanceState: Bundle?): Dialog =
super.onCreateDialog(savedInstanceState).apply {
(this as? BottomSheetDialog)?.fillScreen()
(this as? BottomSheetDialog)?.apply {
fillScreen(isDraggable = true)

behavior.addBottomSheetCallback(object : BottomSheetBehavior.BottomSheetCallback() {
private var isStatusBarTransparent = false
override fun onStateChanged(bottomSheet: View, newState: Int) {
if (newState == BottomSheetBehavior.STATE_EXPANDED && isStatusBarTransparent) {
isStatusBarTransparent = false
val currentTheme = viewModel.currentReadingPreferences.value.theme
handleUpdateStatusBarColor(currentTheme)
} else if (newState != BottomSheetBehavior.STATE_EXPANDED && !isStatusBarTransparent) {
isStatusBarTransparent = true
dialog?.window?.setWindowStatusBarColor(Color.TRANSPARENT)
}

if (newState == BottomSheetBehavior.STATE_HIDDEN) {
viewModel.onBottomSheetHidden()
}
}

override fun onSlide(bottomSheet: View, slideOffset: Float) {
// no-op
}
})
}

(this as ComponentDialog).onBackPressedDispatcher.addCallback(this@ReaderReadingPreferencesDialogFragment) {
viewModel.saveReadingPreferencesAndClose()
viewModel.onExitActionClick()
}
}

override fun onDismiss(dialog: DialogInterface) {
super.onDismiss(dialog)
viewModel.onScreenClosed()
super.onDismiss(dialog)
}

private fun observeActionEvents() {
viewModel.actionEvents.onEach {
when (it) {
is ActionEvent.Close -> dismiss()
is ActionEvent.UpdatePostDetails -> postDetailViewModel.onReadingPreferencesThemeChanged()
is ActionEvent.UpdateStatusBarColor -> handleUpdateStatusBarColor(it.theme)
is ActionEvent.Close -> handleClose(it.isDirty)
is ActionEvent.OpenWebView -> handleOpenWebView(it.url)
}
}.launchIn(viewLifecycleOwner.lifecycleScope)
}

private fun handleClose(isDirty: Boolean) {
if (isDirty) postDetailViewModel.onReadingPreferencesThemeChanged()
dismiss()
}

private fun handleUpdateStatusBarColor(theme: ReaderReadingPreferences.Theme) {
val context = requireContext()
val themeValues = ReaderReadingPreferences.ThemeValues.from(context, theme)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ class ReaderReadingPreferencesViewModel @Inject constructor(
}

fun onScreenClosed() {
if (isDirty()) {
// here we assume the code for saving preferences has been called before reaching this point
launch {
_actionEvents.emit(ActionEvent.UpdatePostDetails)
}
}
readingPreferencesTracker.trackScreenClosed()
}

Expand All @@ -65,15 +71,24 @@ class ReaderReadingPreferencesViewModel @Inject constructor(
readingPreferencesTracker.trackItemTapped(fontSize)
}

fun saveReadingPreferencesAndClose() {
/**
* An exit action has been triggered by the user. This means that we need to save the current preferences and emit
* the close event, so the dialog is dismissed.
*/
fun onExitActionClick() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto here. Just to save some curly braces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same reasoning for not returning Job here.

launch {
val currentPreferences = currentReadingPreferences.value
val isDirty = currentPreferences != originalReadingPreferences
if (isDirty) {
saveReadingPreferences(currentPreferences)
readingPreferencesTracker.trackSaved(currentPreferences)
}
_actionEvents.emit(ActionEvent.Close(isDirty))
saveReadingPreferencesInternal()
_actionEvents.emit(ActionEvent.Close)
}
}

/**
* The bottom sheet has been hidden by the user, which means the dismiss process is already on its way. All we need
* to do is save the current preferences.
*/
fun onBottomSheetHidden() {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the function curly braces = launch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a big fan of that approach since doing that causes the function to return a Job, which is not really what I'm going for and it would be just a side-effect of trying to make the code look prettier.

I get that using = launch is kinda common practice. Still, I personally think that the function signature should better match the intention of the code, so if no callers need the returned Job (and they shouldn't as this function is public and the coroutine is an implementation detail relevant to the ViewModel only) I prefer "returning" Unit.

launch {
saveReadingPreferencesInternal()
}
}

Expand All @@ -84,8 +99,21 @@ class ReaderReadingPreferencesViewModel @Inject constructor(
}
}

private suspend fun saveReadingPreferencesInternal() {
val currentPreferences = currentReadingPreferences.value
if (isDirty()) {
saveReadingPreferences(currentPreferences)
readingPreferencesTracker.trackSaved(currentPreferences)
}
}

private fun isDirty(): Boolean {
return currentReadingPreferences.value != originalReadingPreferences
thomashorta marked this conversation as resolved.
Show resolved Hide resolved
}

sealed interface ActionEvent {
data class Close(val isDirty: Boolean) : ActionEvent
data object Close : ActionEvent
data object UpdatePostDetails : ActionEvent
data class UpdateStatusBarColor(val theme: ReaderReadingPreferences.Theme) : ActionEvent
data class OpenWebView(val url: String) : ActionEvent
}
Expand Down
1 change: 1 addition & 0 deletions WordPress/src/main/res/values/styles.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1371,6 +1371,7 @@
</style>

<style name="ReaderReadingPreferencesDialogFragment" parent="WordPress.NoActionBar">
<item name="android:windowBackground">@android:color/transparent</item>
<item name="android:windowFullscreen">false</item>
<item name="android:windowIsFloating">false</item>
<item name="android:windowAnimationStyle">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import org.junit.Test
import org.mockito.Mock
import org.mockito.kotlin.argThat
import org.mockito.kotlin.verify
import org.mockito.kotlin.verifyNoInteractions
import org.mockito.kotlin.whenever
import org.wordpress.android.BaseUnitTest
import org.wordpress.android.ui.reader.models.ReaderReadingPreferences
Expand Down Expand Up @@ -127,43 +128,89 @@ class ReaderReadingPreferencesViewModelTest : BaseUnitTest() {
}

@Test
fun `when saveReadingPreferencesAndClose is called then it emits Close action event`() = test {
fun `when onExitActionClick is called then it emits Close action event`() = test {
// When
viewModel.saveReadingPreferencesAndClose()
viewModel.onExitActionClick()

// Then
val closeEvent = collectedEvents.last() as ActionEvent.Close
assertThat(closeEvent.isDirty).isFalse()
val closeEvent = collectedEvents.last()
assertThat(closeEvent).isEqualTo(ActionEvent.Close)
}

@Test
fun `when saveReadingPreferencesAndClose is called with updated preferences then it emit Close action event`() =
fun `when onExitActionClick is called with original preferences then it doesn't save them`() =
test {
// Given
val newTheme = ReaderReadingPreferences.Theme.SEPIA
viewModel.onThemeClick(newTheme)
// When
viewModel.onExitActionClick()

// Then
verifyNoInteractions(saveReadingPreferences)
}

@Test
fun `when onExitActionClick is called with updated preferences then it saves them`() = test {
// Given
val newTheme = ReaderReadingPreferences.Theme.SOFT
viewModel.onThemeClick(newTheme)

// When
viewModel.onExitActionClick()

// Then
verify(saveReadingPreferences).invoke(argThat { theme == newTheme })
}

@Test
fun `when onBottomSheetHidden is called with original preferences then it doesn't save them`() =
test {
// When
viewModel.saveReadingPreferencesAndClose()
viewModel.onBottomSheetHidden()

// Then
val closeEvent = collectedEvents.last() as ActionEvent.Close
assertThat(closeEvent.isDirty).isTrue()
verifyNoInteractions(saveReadingPreferences)
}

@Test
fun `when saveReadingPreferencesAndClose is called with updated preferences then it saves them`() = test {
fun `when onBottomSheetHidden is called with updated preferences then it saves them`() = test {
// Given
val newTheme = ReaderReadingPreferences.Theme.SOFT
viewModel.onThemeClick(newTheme)

// When
viewModel.saveReadingPreferencesAndClose()
viewModel.onBottomSheetHidden()

// Then
verify(saveReadingPreferences).invoke(argThat { theme == newTheme })
}

@Test
fun `when onScreenClosed is called with original preferences then it doesn't emit UpdatePostDetail`() = test {
// Given
viewModel.onExitActionClick()

// When
viewModel.onScreenClosed()

// Then
val updateEvent = collectedEvents.last()
assertThat(updateEvent).isNotEqualTo(ActionEvent.UpdatePostDetails)
}

@Test
fun `when onScreenClosed is called with updated preferences then it emits UpdatePostDetail`() = test {
// Given
val newTheme = ReaderReadingPreferences.Theme.SOFT
viewModel.onThemeClick(newTheme)
viewModel.onExitActionClick()

// When
viewModel.onScreenClosed()

// Then
val updateEvent = collectedEvents.last()
assertThat(updateEvent).isEqualTo(ActionEvent.UpdatePostDetails)
}

@Test
fun `when onSendFeedbackClick is called then it emits OpenWebView action event`() = test {
// When
Expand Down Expand Up @@ -270,7 +317,7 @@ class ReaderReadingPreferencesViewModelTest : BaseUnitTest() {
viewModel.onThemeClick(newTheme)

// When
viewModel.saveReadingPreferencesAndClose()
viewModel.onExitActionClick()

// Then
verify(readingPreferencesTracker).trackSaved(argThat { theme == newTheme })
Expand Down
Loading