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.

Sign up for GitHub

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

Jump to bottom

Migrate share fragment to Jetpack Compose #1185

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Migrate share fragment to Jetpack Compose #1185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4af51cc
Migrated share fragment data binding to Jetpack compose
CalebKL Jul 20, 2023
ac36693
Deleted share fragment xml
CalebKL Jul 20, 2023
e54c130
run linting
CalebKL Jul 20, 2023
68cdfd7
Merge branch 'main' into migrate-share-fragment-to-compose
CalebKL Jul 20, 2023
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
296 changes: 218 additions & 78 deletions ...features/player/src/main/java/au/com/shiftyjelly/pocketcasts/player/view/ShareFragment.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,124 +4,264 @@ import android.os.Bundle
import android.view.LayoutInflater
import android.view.View
import android.view.ViewGroup
import androidx.core.view.isVisible
import androidx.compose.foundation.background
import androidx.compose.foundation.clickable
import androidx.compose.foundation.interaction.MutableInteractionSource
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.height
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.wrapContentSize
import androidx.compose.foundation.shape.RoundedCornerShape
import androidx.compose.material.Divider
import androidx.compose.material.MaterialTheme
import androidx.compose.material.Surface
import androidx.compose.material.Text
import androidx.compose.material.ripple.rememberRipple
import androidx.compose.runtime.Composable
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.remember
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.clip
import androidx.compose.ui.graphics.Color
import androidx.compose.ui.platform.ComposeView
import androidx.compose.ui.res.stringResource
import androidx.compose.ui.text.font.FontFamily
import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.DpSize
import androidx.compose.ui.unit.dp
import androidx.compose.ui.unit.sp
import androidx.fragment.app.activityViewModels
import androidx.lifecycle.asFlow
import au.com.shiftyjelly.pocketcasts.analytics.AnalyticsTrackerWrapper
import au.com.shiftyjelly.pocketcasts.analytics.SourceView
import au.com.shiftyjelly.pocketcasts.compose.AppTheme
import au.com.shiftyjelly.pocketcasts.compose.theme
import au.com.shiftyjelly.pocketcasts.models.entity.BaseEpisode
import au.com.shiftyjelly.pocketcasts.models.entity.Podcast
import au.com.shiftyjelly.pocketcasts.models.entity.PodcastEpisode
import au.com.shiftyjelly.pocketcasts.player.databinding.FragmentShareBinding
import au.com.shiftyjelly.pocketcasts.player.viewmodel.PlayerViewModel
import au.com.shiftyjelly.pocketcasts.repositories.podcast.SharePodcastHelper
import au.com.shiftyjelly.pocketcasts.repositories.podcast.SharePodcastHelper.ShareType
import au.com.shiftyjelly.pocketcasts.ui.helper.StatusBarColor
import au.com.shiftyjelly.pocketcasts.ui.theme.Theme
import au.com.shiftyjelly.pocketcasts.views.extensions.applyColor
import au.com.shiftyjelly.pocketcasts.views.fragments.BaseDialogFragment
import com.google.android.material.bottomsheet.BottomSheetBehavior
import com.google.android.material.bottomsheet.BottomSheetDialog
import dagger.hilt.android.AndroidEntryPoint
import io.reactivex.disposables.Disposable
import javax.inject.Inject
import au.com.shiftyjelly.pocketcasts.localization.R as IR

@AndroidEntryPoint
class ShareFragment : BaseDialogFragment() {

@Inject lateinit var analyticsTracker: AnalyticsTrackerWrapper
override val statusBarColor: StatusBarColor? = null

private lateinit var composeView: ComposeView
private val viewModel: PlayerViewModel by activityViewModels()
private var binding: FragmentShareBinding? = null
private var disposable: Disposable? = null

override fun onPause() {
super.onPause()

disposable?.dispose()
}
Comment on lines 66 to 71
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we're not using disposable anywhere. Can we remove the variable and onPause override?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing


override fun onCreateView(inflater: LayoutInflater, container: ViewGroup?, savedInstanceState: Bundle?): View {
val binding = FragmentShareBinding.inflate(inflater, container, false)
this.binding = binding

val podcast = viewModel.podcast
val episode = viewModel.episode
return ComposeView(requireContext()).also {
composeView = it
}
}

binding.buttonSharePodcast.setOnClickListener {
if (podcast != null) {
SharePodcastHelper(
podcast,
null,
null,
requireContext(),
ShareType.PODCAST,
SourceView.PLAYER,
analyticsTracker
).showShareDialogDirect()
@Suppress("DEPRECATION")
Copy link
Contributor

Choose a reason for hiding this comment

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

onActivityCreated has been deprecated. Should we consider to use OnViewCreated function? wdyt?

override fun onActivityCreated(savedInstanceState: Bundle?) {
super.onActivityCreated(savedInstanceState)
composeView.setContent {
val podcast = viewModel.podcast
val episode = viewModel.episode
val listData = viewModel.listDataLive.asFlow().collectAsState(initial = null)
listData.value?.let {
ShareScreen(
podcast = viewModel.podcast,
episode = viewModel.episode,
backgroundColor = Color(it.podcastHeader.backgroundColor),
onSharePodcast = {
if (podcast != null) {
SharePodcastHelper(
podcast,
null,
null,
requireContext(),
SharePodcastHelper.ShareType.PODCAST,
SourceView.PLAYER,
analyticsTracker
).showShareDialogDirect()
}
},
onShareEpisode = {
if (podcast != null && episode is PodcastEpisode) {
SharePodcastHelper(
podcast,
episode,
null,
requireContext(),
SharePodcastHelper.ShareType.EPISODE,
SourceView.PLAYER,
analyticsTracker
).showShareDialogDirect()
}
},
onShareCurrentPosition = {
if (podcast != null && episode is PodcastEpisode) {
SharePodcastHelper(
podcast,
episode,
episode.playedUpTo,
requireContext(),
SharePodcastHelper.ShareType.CURRENT_TIME,
SourceView.PLAYER,
analyticsTracker
).showShareDialogDirect()
}
},
onShareOpenFileIn = {
if (podcast != null && episode is PodcastEpisode) {
SharePodcastHelper(
podcast,
episode,
episode.playedUpTo,
requireContext(),
SharePodcastHelper.ShareType.EPISODE_FILE,
SourceView.PLAYER,
analyticsTracker
).sendFile()
}
}
)
}
close()
}
binding.buttonShareEpisode.setOnClickListener {
if (podcast != null && episode is PodcastEpisode) {
SharePodcastHelper(
podcast,
episode,
null,
requireContext(),
ShareType.EPISODE,
SourceView.PLAYER,
analyticsTracker
).showShareDialogDirect()
viewModel.playingEpisodeLive.observe(viewLifecycleOwner) { (_, backgroundColor) ->
applyColor(theme, backgroundColor)
}
close()
}
binding.buttonShareCurrentPosition.setOnClickListener {
if (podcast != null && episode is PodcastEpisode) {
SharePodcastHelper(
podcast,
episode,
episode.playedUpTo,
requireContext(),
ShareType.CURRENT_TIME,
SourceView.PLAYER,
analyticsTracker
).showShareDialogDirect()
(dialog as? BottomSheetDialog)?.behavior?.state = BottomSheetBehavior.STATE_EXPANDED
}
}

@Composable
private fun ShareScreen(
podcast: Podcast?,
Copy link
Contributor

Choose a reason for hiding this comment

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

As we are passing Podcast and BaseEpisode object to composable function, Should we mark these classes with @stable for compose-runtime?

episode: BaseEpisode?,
backgroundColor: Color,
onSharePodcast: () -> Unit,
onShareEpisode: () -> Unit,
onShareCurrentPosition: () -> Unit,
onShareOpenFileIn: () -> Unit,
) {
Surface(
modifier = Modifier
.fillMaxWidth(),
color = backgroundColor
) {
Column(
modifier = Modifier
.padding(bottom = 24.dp)
) {
Pill(
modifier = Modifier
.align(Alignment.CenterHorizontally)
)
if (podcast != null) {
ShareButton(
modifier = Modifier
.padding(top = 20.dp),
label = IR.string.share_podcast,
onClick = onSharePodcast
)
}
close()
}
binding.buttonOpenFileIn.setOnClickListener {
if (podcast != null && episode is PodcastEpisode) {
SharePodcastHelper(
podcast,
episode,
episode.playedUpTo,
requireContext(),
ShareType.EPISODE_FILE,
SourceView.PLAYER,
analyticsTracker
).sendFile()
Divider(
color = Color.LightGray.copy(0.3f),
thickness = 1.dp,
)
Divider(thickness = 1.dp)
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate divider? There's one above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

if (podcast != null && episode is BaseEpisode) {
ShareButton(
label = IR.string.podcast_share_episode,
onClick = onShareEpisode
)
Divider(
color = Color.LightGray.copy(0.3f),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider to use MaterialTheme outline color as divider colours rather than hardcoding them. Wdyt?

thickness = 1.dp,
)
ShareButton(
label = IR.string.podcast_share_current_position,
onClick = onShareCurrentPosition
)
}
if (podcast != null && episode is BaseEpisode && episode.isDownloaded) {
ShareButton(label = IR.string.podcast_share_open_file_in, onClick = onShareOpenFileIn)
}
close()
}

binding.sharePodcast.isVisible = podcast != null
binding.shareEpisode.isVisible = episode != null
binding.shareCurrentPosition.isVisible = episode != null
binding.openFileIn.isVisible = episode != null && episode.isDownloaded

return binding.root
}
}

@Suppress("DEPRECATION")
override fun onActivityCreated(savedInstanceState: Bundle?) {
super.onActivityCreated(savedInstanceState)
@Composable
private fun ShareButton(
label: Int,
onClick: () -> Unit,
modifier: Modifier = Modifier
) {
Box(
modifier = modifier
.fillMaxWidth()
.clickable(
interactionSource = remember { MutableInteractionSource() },
indication = rememberRipple(color = Color.LightGray)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider to use MaterialTheme outline color as divider colours rather than hardcoding them. Wdyt?

) { onClick() }
.height(64.dp)
.padding(start = 32.dp)
.wrapContentSize(Alignment.CenterStart)
) {
Text(
text = stringResource(id = label),
color = MaterialTheme.theme.colors.playerContrast01,
fontFamily = FontFamily.SansSerif,
fontSize = 16.sp
)
}
}

viewModel.playingEpisodeLive.observe(viewLifecycleOwner) { (_, backgroundColor) ->
applyColor(theme, backgroundColor)
}
private val PillSize = DpSize(width = 48.dp, height = 4.dp)
private val PillCornerRadius = 10.dp

(dialog as? BottomSheetDialog)?.behavior?.state = BottomSheetBehavior.STATE_EXPANDED
}
@Composable
private fun Pill(
Copy link
Contributor

Choose a reason for hiding this comment

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

There can be reusable components that can be extracted out like this Pill and the component for each option. Can you describe the challenges you had in using the OptionsDialog() that you initially proposed to use?

This will help us decide the architecture for the rest of the "Playback Effects" dialogs.

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 agree, the pill can be reused. Regarding the Options Dialog, it would work but one concern:
If we migrate all fragments using BaseDialogFragment() to optionsDialog(), we would have to add new rows for each specific fragment. That means for each fragment that is being migrated, we would add new custom composables on the dialog. But if this would not be a problem, it would work.

Copy link
Contributor

Choose a reason for hiding this comment

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

That means for each fragment that is being migrated, we would add new custom composables on the dialog.

Could we pass a custom option component (which is only shown on a single screen) to the OptionsDialog() so that we don't increase the class size too much and yet be able to have a common class across all these option fragments?

@mchowning, @geekygecko any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Mzazi25 👋

I had a brief discussion with the team, with OptionsDialog() we'll be able to reuse common components and see some advantages. Can we give it a try?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. Let me work on that

modifier: Modifier = Modifier,
) {
Box(
modifier = modifier
.size(PillSize)
.clip(RoundedCornerShape(PillCornerRadius))
.background(MaterialTheme.theme.colors.primaryText02)
)
}

private fun close() {
dismiss()
@Preview
@Composable
private fun ShareScreenPrev(
theme: Theme.ThemeType = Theme.ThemeType.DARK
) {
AppTheme(themeType = theme) {
ShareScreen(
podcast = Podcast(),
episode = null,
backgroundColor = Color.DarkGray,
onSharePodcast = {},
onShareEpisode = { },
onShareCurrentPosition = {},
onShareOpenFileIn = {}
)
}
}
Loading