Status Update
Comments
al...@google.com <al...@google.com> #2
Thanks for the detailed analysis and sample. I've prepared a CL, but I'd also be happy to review an external contribution since you already went through the trouble of identifying a fix. Just let me know here.
py...@gmail.com <py...@gmail.com> #3
If you have a CL ready go for it.
That being said, I just went down git history and found when this was introduced:
As you can see, this was introduced to fix a "memory leak" though I don't quite understand why creating a new callback wrapper for every setSupportActionBar()
call is a problem.
ap...@google.com <ap...@google.com> #4
Branch: androidx-main
commit 200ccecbc8c7c5ba555e7fa15890edff67affb58
Author: Alan Viverette <alanv@google.com>
Date: Fri Apr 30 14:55:07 2021
Fix case where setSupportActionBar overwrites the window callback
RelNote: """Fix scenario where calling setSupportActionBar after setting the
window callback would overwrite the callback."""
Test: SupportActionBarTestCase
Fixes: 186791590
Change-Id: Ie43eedf8322caa44e7b201a95cbc64197953e020
A appcompat/appcompat/src/androidTest/java/androidx/appcompat/app/SupportActionBarTestCase.kt
M appcompat/appcompat/src/main/java/androidx/appcompat/app/AppCompatDelegateImpl.java
lb...@gmail.com <lb...@gmail.com> #5
ch...@gmail.com <ch...@gmail.com> #6
I vaguely remember this. The issue was iirc:
- You use multiple fragments (say switched via tabs). Each fragment calls
setSupportActionBar()
when visible. - The old code would wrap the current
Window.Callback
with something fromToolbarActionBar
(TAB), then set it as the window callback. - When the user switches to Tab #2, we'd do the same thing, but this time the callback wrapper chain would be
(((Root callback) TBAB1Wrapper) TBAB2Wrapper)
. TBAB1Wrapper would keep a reference a reference to the views even though the #1 fragment has likely been destroyed.
I've just +2'd Alan's CL, but the proper fix is properly to unwrap the callback from Window.getCallback()
.
al...@google.com <al...@google.com> #7
Let me uh... let me go back and properly fix that.
py...@gmail.com <py...@gmail.com> #8
One thing though, you can't quite unwrap because there's no guarantee you'll get the same callback (e.g. it might have been wrapped again).
It seems like what we want here is:
- ensure the callback is set at least once
- get a reference to it later
Is there a way the internal code can keep a ref to that new callback once its set, and reference it instead of installing or unwrap?
al...@google.com <al...@google.com> #9
there's no guarantee you'll get the same callback (e.g. it might have been wrapped again).
Do you mean the app code may have wrapped the callback?
py...@gmail.com <py...@gmail.com> #10
Yep. That's what got this issue filed after all ;-)
al...@google.com <al...@google.com> #11
Heh, of course.
Is there a way the internal code can keep a ref to that new callback once its set, and reference it instead of installing or unwrap?
This goes beyond my familiarity with (and available time to invest in understanding) the existing codebase -- I'd have to defer to Chris.
ch...@google.com <ch...@google.com> #12
I don't think there's a perfect solution here to be honest. I'm thinking that reverting to the previous solution is the only 'correct' way forward for the 80% case.
If apps want to change the window callback, they can override setSupportActionBar()
and do it in there, after calling super.
py...@gmail.com <py...@gmail.com> #13
That won't work for libraries. The callback delegation allows libraries to play nice together. We shouldn't one library (appcompat) break all othee libraries.
py...@gmail.com <py...@gmail.com> #14
Looking into this a bit more, it seems that what we really need to do is:
- Install the callback only once and keep a ref to it.
- Decouple the callback from ToolbarActionBar, so that we can swap the ToolbarActionBar instance receiving things (or set it to null)
Maybe 1) and 2) means actually killing the callback and adding a nullable ToolbarActionBar to the default callback already install? That'd simplify things a lot it seems.
Thoughts? I can maybe take a stab in between 2 diaper changes.
ch...@google.com <ch...@google.com> #15
Yeah, that should be do-able. Looking now.
ap...@google.com <ap...@google.com> #17
Branch: androidx-main
commit 32144c85d339f680dbffd6ce9d16d57d15cdd65b
Author: Chris Banes <chrisbanes@google.com>
Date: Thu May 06 12:07:14 2021
Re-work support action bar window callback handling
Instead of wrapping the Window.Callback when an action bar
is set, we now just updated our existing callback so
that it receives the necessary events.
Follow-on from Ie43eedf8322caa44e7b201a95cbc64197953e020.
Test: SupportActionBarTestCase
Fixes: 186791590
Change-Id: I091cb6826ce4896cd5a500da7aa741cef862fc64
M appcompat/appcompat/src/main/java/androidx/appcompat/app/AppCompatDelegateImpl.java
M appcompat/appcompat/src/main/java/androidx/appcompat/app/ToolbarActionBar.java
Description
Summary
Calling
AppCompatActivity.setSupportActionBar()
replaces the window callback with a delegate callback that delegates to a previously installedAppCompatWindowCallback
instead of delegating to the current window callback, thus breaking developers expectations around window callback delegation.Root cause
android.view.Window
instance can have a singleandroid.view.Window.Callback
set (viaAppCompatActivity.onCreate()
, as we can see inAppCompatActivity.setSupportActionBar()
does not follow that correct pattern. It replaces the callback but then delegates to the previously installedAppCompatWindowCallback
instead of the current window callback, as we can see inRepro
This bug was originally reported square/curtains#14 . After investigating, I determined this is actually an AppCompat bug. I'm attaching the repro project and video demonstrating the bug from that ticket.
Here's an example activity that reproduces the issue:
The fix is likely that
AppCompatDelegateImpl.setSupportActionBar()
should passmWindow.getCallback()
to theToolbarActionBar
constructor, instead ofmAppCompatWindowCallback
.