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

TrackSelectionDialogBuilder does not use androidx AlertDialog #7357

Closed
sixtsense opened this issue May 11, 2020 · 11 comments
Closed

TrackSelectionDialogBuilder does not use androidx AlertDialog #7357

sixtsense opened this issue May 11, 2020 · 11 comments
Assignees

Comments

@sixtsense
Copy link

TrackSelectionDialogBuilder uses legacy android.app.AlertDialog instead of androidx.appcompat.app.AlertDialog. The problems arises when using the TrackSelectionDialogBuilder with a DayNight theme, because the legacy alert dialog do not respect the styles.

@krocard krocard assigned krocard and tonihei and unassigned krocard May 14, 2020
@krocard
Copy link
Contributor

krocard commented May 14, 2020

@tonihei, the code is here:

AlertDialog.Builder builder = new AlertDialog.Builder(context);

I guess this should be replace by androidx.appcompat.app.AlertDialog

ojw28 pushed a commit that referenced this issue May 27, 2020
This ensure style themes are correctly applied.

issue:#7357
PiperOrigin-RevId: 313145345
@ojw28 ojw28 closed this as completed May 27, 2020
ojw28 pushed a commit that referenced this issue May 28, 2020
This ensure style themes are correctly applied.

issue:#7357
PiperOrigin-RevId: 313145345
@fgl27
Copy link

fgl27 commented May 29, 2020

This single change adds a lot to a app final size.

with the change

Screenshot from 2020-05-29 02-08-54

without

Screenshot from 2020-05-29 02-44-45

Is that correct? anything to do to get a smaller final apk?

@ojw28 @tonihei

@ojw28
Copy link
Contributor

ojw28 commented May 29, 2020

Are you shrinking your code and resources for your release build, as per this guide? You should be using:

shrinkResources true
minifyEnabled true

@fgl27
Copy link

fgl27 commented May 29, 2020

yes if I don't the app size is around 4 to 5MB.

@tonihei
Copy link
Collaborator

tonihei commented May 29, 2020

@fgl27 Could you answer these questions to understand your change a bit more?

  • Are you using TrackSelectionDialogBuilder in your code? If not, then this change should have zero impact on your apk size.
  • Are you using any other androidx modules? If yes, I'd assume that most classes in there (at least the ones used by a Dialog) are already used somewhere else and thus shouldn't increase the apk size.
  • Are you already using Guava? We also addd a Guava dependency recently (and where unsure if this could cause apk size increases).
  • Do you have any custom proguard/R8 rules that may prevent certain classes from being stripped from your apk?

@fgl27
Copy link

fgl27 commented May 29, 2020

@fgl27 Could you answer these questions to understand your change a bit more?

  • Are you using TrackSelectionDialogBuilder in your code? If not, then this change should have zero impact on your apk size.

No, I don't have any dialog on that app, it's a basic app just a webview UI that uses Exoplayer to play videos, I build exoplayer-library-ui so I can use exoplayer ProgressBar that show when the player is buffering.

I don't understand how the auto generate file app/build/outputs/mapping/release/usage.txt works, but when I grep my source for TrackSelectionDialogBuilder I get this:

Binary file apk/.gradle/6.4.1/javaCompile/classAnalysis.bin matches
Binary file apk/.gradle/6.4.1/javaCompile/jarAnalysis.bin matches
Binary file apk/.gradle/6.4.1/executionHistory/executionHistory.bin matches
Binary file apk/app/build/intermediates/dex/debug/mergeLibDexDebug/classes_0.dex matches
apk/app/build/outputs/mapping/release/usage.txt:com.google.android.exoplayer2.ui.TrackSelectionDialogBuilder$DialogCallback
apk/app/build/outputs/mapping/release/usage.txt:com.google.android.exoplayer2.ui.TrackSelectionDialogBuilder

Maybe that helps understand the issue.

  • Are you using any other androidx modules? If yes, I'd assume that most classes in there (at least the ones used by a Dialog) are already used somewhere else and thus shouldn't increase the apk size.

App dependencies

https://github.com/fgl27/SmartTwitchTV/blob/master/apk/app/build.gradle#L58

I don't think anyone of those androidx dependencies the app uses is causing this issue.

  • Are you already using Guava? We also addd a Guava dependency recently (and where unsure if this could cause apk size increases).

No the app doesn't uses Guava.

To achive the small apk size the only commit I revert was this
f099f57

I reset the head of exoplayer here:
0de9c00

Before all Guava changes this is the app size

Screenshot from 2020-05-29 12-07-44

So the Guava changes add less then 20K to the apk

  • Do you have any custom proguard/R8 rules that may prevent certain classes from being stripped from your apk?

I don't think so, app proguard file

https://github.com/fgl27/SmartTwitchTV/blob/master/apk/app/proguard-rules.pro

I don't think that proguard file is the issue, but I erase the file content and made a clean build the size is this

Screenshot from 2020-05-29 11-57-51

Only 10K bigger from when using the proguard file, so it's not the issue. On that test build I didn't revert the commit f099f57 to make sure that the proguard file is not the issue, so it shows app size is bigger no matter what.

I don't know what is causing this is not a big deal, I'm not complaining I build Exo from source and reverting a commit is not a problem, I just wanna inform the problem I notice, and maybe there is something that can be improve here not just for me but for others.

I doing all this builds clean using latest version of studio for linux.

Screenshot from 2020-05-29 12-11-30

@tonihei
Copy link
Collaborator

tonihei commented May 29, 2020

Thanks for detailed answers! That's really helpful.

No, I don't have any dialog on that app, it's a basic app just a webview UI that uses Exoplayer to play videos, I build exoplayer-library-ui so I can use exoplayer ProgressBar that show when the player is buffering.

In this case, it's even stranger that this happens given that there are no dependencies from your code to the new androidx lib. I'll reopen the issue to see if we can reproduce that somehow.

@fgl27
Copy link

fgl27 commented May 30, 2020

OK thank you, I will do some other test on the weekend if I find something I inform.

andrewlewis pushed a commit that referenced this issue Jun 1, 2020
This ensure style themes are correctly applied.

issue:#7357
PiperOrigin-RevId: 313145345
@tonihei
Copy link
Collaborator

tonihei commented Jun 1, 2020

I investigated this a little bit with your example project.

  • It seems that simply adding is enough to add 600KB to the apk, even if no code is actually using it.
  • This also happens when you add the dependency directly to your app, so the problem is not really ExoPlayer specific.
  • Removing your leanback dependency also saved 500KB :)
  • Replacing it by the legacy 'com.android.support:appcompat-v7:28.0.0' makes no difference.
  • Using android.enableR8.fullMode=true reduces the impact to 500KB, which is better but doesn't seem to solve the root cause.
  • Disabling android.enableJetifier=true makes no difference.
  • Going back to the legacy Proguard tool (instead of R8) with android.EnableR8=false doesn't even build the apk because it can't find various referenced classes.
  • Removing all the -keep rules in proguard-rules.pro doesn't make a difference
  • The build process generates a file called aapt_rules.txt that contains all the final "-keep" rules. In there, there are many -keep class androidx.appcompat.{...} rules that are the reason why all these classes and resources are kept. But it's unclear why they are in there. These seem to be auto-generated based on some UI file usage. Other people found a similar issue and weren't able to get rid of it: example1, example2

Based on the last point it looks like this can't be solved easily. For people who already depend on the AndroidX appcompat module it won't be an issue. But in cases like yours where you don't use it already, it will create a big apk size impact.

I see 3 options to work around that:

  1. Revert the change entirely. Then the original request won't be solved.
  2. Use androidx appcompat as a compileOnly dependency and document TrackSelectionDialogBuilder to require this extra dependency on the app side to work. This could also be a new build() method that let's you build the dialog for AndroidX specifically.
  3. Check via reflection whether the appcompat AlertDialog is available, and if not, fall back to the System AlertDialog.

@fgl27
Copy link

fgl27 commented Jun 2, 2020

Thanks you for the detailed description, on my test I didn't find any way to prevent the issue. I assume on thi issue the best solution is leave as it's to prevent confusion and others issue the size is not a big problem.

But that is just me, you guys decide what is best for the project.

ojw28 pushed a commit that referenced this issue Jul 24, 2020
The dependency is only used to create a dialog in
TrackSelectionDialogBuilder that is compatible with newer styling
options.

This dependendy adds over 500Kb to the apk (even if unused) and we
shoudn't force this on an app. Instead make the dependency optional by
automatically falling back to the platform version if the AndroidX one
doesn't exist.

Issue: #7357
PiperOrigin-RevId: 322143005
@tonihei
Copy link
Collaborator

tonihei commented Jul 31, 2020

The commit above solved the apk size issue by only using the AndroidX dialog if the dependency is already available in the app.

@tonihei tonihei closed this as completed Jul 31, 2020
zxzx74147 pushed a commit to zxzx74147/ExoPlayer that referenced this issue Sep 7, 2020
This ensure style themes are correctly applied.

issue:google#7357
PiperOrigin-RevId: 313145345
@google google locked and limited conversation to collaborators Sep 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants