androidx / media

Jetpack Media3 support libraries for media use cases, including ExoPlayer, an extensible media player for Android
Apache License 2.0
1.35k stars 320 forks source link

Ability to set Dialog for TrackSelectionDialogBuilder #1058

Open yoobi opened 5 months ago

yoobi commented 5 months ago

[REQUIRED] Use case description

Currently TrackSelectionDialogBuilder uses either androidx.appcompat.app.AlertDialog$Builder or AlertDialog.Builder in the build(). My app is using Material3, but other people might be using Material2 or even there own theme. My current issue is that I can't theme the Dialog to Material3 even when using the setTheme().

Proposed solution

I'm proposing to either add a new constructor which could take in an object extending AlertDialog.Builder or adding a new setter to do so. There is even a solution of making TrackSelectionDialogBuilder an abstract class and let the users implement their own which would shift the issue of displaying the right dialog/theming onto the user.

Note: I could propose a PR for the selected solution

oceanjules commented 3 months ago

Sorry for a delay in replying. Could you explain why setTheme doesn't work?

From what I can tell, building for platform with android.app.AlertDialog.Builder: https://github.com/androidx/media/blob/d13a0f4ec62ca092b79746a5725b62a3244cc5b4/libraries/ui/src/main/java/androidx/media3/ui/TrackSelectionDialogBuilder.java#L251-L252

and building with AndroidX with androidx.appcompat.app.AlertDialog.Builder https://github.com/androidx/media/blob/d13a0f4ec62ca092b79746a5725b62a3244cc5b4/libraries/ui/src/main/java/androidx/media3/ui/TrackSelectionDialogBuilder.java#L272-L278

should both respect the themeResId that is passed. Are you saying that neither builder inflates the dialog correctly?

yoobi commented 3 months ago

Hello,

I should have put some code. Yes the themeResId isn't respected for Material3.

<style name="CustomTrackSelectionTheme" parent="ThemeOverlay.Material3.MaterialAlertDialog" />
TrackSelectionDialogBuilder(
            context,
            "Select Quality",
            player,
            C.TRACK_TYPE_VIDEO
        ).apply {
            setTrackFormatComparator { o1, o2 -> o2.height - o1.height }
            setTheme(R.style.CustomTrackSelectionTheme)
        }.build().show()
trackselector

And the result looks like above while material3 dialog have rounded corner and looks more like https://m3.material.io/components/dialogs/overview#310aaace-3c78-4b14-a878-ab8e724c5c06

oceanjules commented 3 months ago

Stumbled upon https://developer.android.com/reference/com/google/android/material/dialog/MaterialAlertDialogBuilder

and I quote:

An extension of AlertDialog.Builder for use with a Material theme (e.g., Theme.MaterialComponents).

This Builder must be used in order for AlertDialog objects to respond to color and shape theming provided by Material themes. The type of dialog returned is still an AlertDialog; there is no specific Material implementation of AlertDialog.

We can possible try to have something like

TrackSelectionDialogBuilder(
            context,
            "Select Quality",
            player,
            C.TRACK_TYPE_VIDEO
        ).apply {
            setTrackFormatComparator { o1, o2 -> o2.height - o1.height }
            setIsMaterial(true)
            setTheme(R.style.CustomTrackSelectionTheme)
        }.build().show()

and have the build() method make a choice between 3 types

  /** Builds the dialog. */
  public Dialog build() {
    @Nullable Dialog dialog;
    if (isMaterial) {
      dialog = buildForMaterial3();
    } else {
      dialog = buildForAndroidX();
    }
    return dialog == null ? buildForPlatform() : dialog;
  }

where buildForMaterial3() makes use of MaterialAlertDialogBuilder(Context context, int overrideThemeResId)

The problem of course is that since MaterialAlertDialogBuilder extends androidx.appcompat.app.AlertDialog.Builder, then both buildForMaterial3() and buildForAndroidX() will have to use reflection.

We will discuss internally and I will get back to you on this, but thank you in the meantime for suggesting this enhancement.

yoobi commented 3 months ago

Thank you

The problem of course is that since MaterialAlertDialogBuilder extends androidx.appcompat.app.AlertDialog.Builder, then both buildForMaterial3() and buildForAndroidX() will have to use reflection.

The issue about this is that I'm using Material3 but some people might be using Material2 that's why I was proposing to delegate this to users with an abstract class or something like this.