SensibleSwipeDismissBehavior is more a fix for the inconsistent behavior
of SwipeDismissBehavior, rather than a custom implementation.
This addresses the following issues:
* When the snackbar is swiped to the right, its opacity changes,
but not when swiped to the left;
* When moving the snackbar to dismiss it, the target distance
calculation does not take margins into account, which makes the
snackbar briefly appear stuck near the edge;
* Any amount of horizontal velocity will dismiss the snackbar,
which can result in user dismissing the snackbar even though they
didn't want to;
* If you drag the snackbar to the right, and then flinging it to the
left, it will suddenly change course and start moving to the right.
This changes the way snackbars are created. Instead of calling a
function with many arguments, the new methods consistently only take
text, duration, and a builder block--whether it is called on an
activity or a view. I chose builder pattern instead of call chaining as
it makes sure that the snackbar is actually shown in the end.
Note that if showSnackbar is called on an Activity that does not have a
view with id root_layout--which does not allow proper placing and
interaction with snackbars--a runtime exception will be intentionally
thrown in a debug build. Release builds will show a toast with the
contents of the snackbar instead.
Some durations were changed, notably, Card browser's snackbar that allow
searching in all decks now stay until dismissed. This is something user
may want to do at any time later; and if the snackbar is in the way,
they can dismiss it by swiping.
In com.ichi2.anki.CoroutineHelpersKt.launchCatchingTask, a few unrelated
to this change TODO comments were added.
Previously, snackbars were sliding from the bottom. Material snackbars
instead fade in, and slightly scale out. These snackbars are meant to be
placed above the fab, but our fab opens a menu, and what are you going
to do with that, huh?
This makes fab slide up with some deceleration when a snackbar starts
appearing, and down after it is removed. This plays well with fab's
rotating animation.
This changes the parent themes to Material Bridge themes, and adds the
necessary attributes to style snackbars. The colors for the action
button are not exactly right since current themes misuse colors a bit.
From the tutorial:
Bridge themes inherit from AppCompat themes, but also define the new
Material Components theme attributes for you. If you use a bridge
theme, you can start using Material Design components without changing
your app theme.
The way SwipeDismissBehavior uses ViewDragHelper, event interception may
happen well past the initial start of the gesture. ViewDragHelper will
capture the view under the finger, which at this point might be some
unrelated view, and *that* view will be moved instead. Looks very funny.
This solves the issue by never intercepting touch events unless the
finger is over the snackbar.
While you can can dismiss snackbars by flinging, due to a bug in the
library, they stay in place while you do so, and only ever disappear
to the right, which might not be the direction of the fling.
This makes snackbars move along with the finger.
Snackbars are dismissible by swiping and thus should appear above the
bottom gesture area. Some of the snackbars were appearing on the bottom
of the screen even though they were using CoordinatorLayout as the root,
this fixes it. Also, this fixes excessive relayout/remeasure while
snackbars are showing.
This does not fix snackbars not using CoordinatorLayout as the root.
An attentive reader might ask, what on Earth does this change have to do
with Snackbars? Well, let me tell you a story.
I noticed that in Full screen navigation drawer mode, which I was using,
snackbars are appearing too high. (This is another issue.) When I almost
made a commit to make them appear on the very bottom of the screen,
by setting bottomMarginGestureInsetRunnable to true, I realized that
while this does make snackbars look consistent, they shouldn't be at
the bottom of the screen in the first place. So in one case they were
too high, and in another case too low, and, if dismissed via swiping,
they left the fab too high. (This is yet another issue.)
After minute investigation, that totally didn't take several hours,
I found what I think is the issue. The timing is the problem.
Animating snackbars in should work like this:
* Ask the framework for insets
* Post a callback, bottomMarginGestureInsetRunnable,
that adjusts margins taking insets into account
* This removes any callbacks posted previously!
* Post an animation
Looks okay? Well, turns out that if Full screen drawer mode is not on,
there's *a lot* of remeasuring going on. Remeasuring results in the
above callback getting reposted again and again, and it ends up running
*after* animation.
Here's a debug log in case of Full screen drawer mode:
showing snackbar
posting callback
posting callback
callback run
animating
And here's one in regular mode:
showing snackbar
posting callback ¹
posting callback ²
posting callback †
posting callback †
posting callback †
animating
posting callback †
callback run ‡
posting callback
callback run ‡
posting callback
callback run ‡
(many more lines)
posting callback
callback run ‡
I guess you see the issue. What happens here:
* Callback gets posted “normally” twice (¹, ²).
* DrawerLayout.onMeasure sees if its child, CoordinatorLayout,
fits system windows. Since it does, it calls its method
dispatchApplyWindowInsets, which propagates to
BaseTransientBottomBar's inset listener, which reposts
bottomMarginGestureInsetRunnable (†).
* The callback ends up running too late ... and when it is run, its
parent has already ended its own layout. This is why, I think,
its call to requestLayout propagates to the root, which begins to
remeasure things again (‡).
* The last two steps are repeated on and on in a loop.
Why does this happen? Well, I think that:
* The positioning problem is an issue with Snackbar itself.
It does not make sure that animation is run after inset listener
has been called.
* The loop problem is likely one of those weird Android issues that
come out of nowhere. Maybe it's this particular combination of
views? Or some funny flag somewhere deep in the code? Who knows.
How this solves it? Well, I looked at why this is not an issue if Full
screen navigation drawer setting is on. As mentioned above,
DrawerLayout.onMeasure checks if its child fits system windows, and then
calls dispatchApplyWindowInsets. In this case, CoordinatorLayout is
wrapped in FullDraggableContainer, which does not have the fit system
windows flag set. Removing this flag in CoordinatorLayout makes the
behavior the same.
***
For reference, here's tracebacks for ¹ and ²:
Breakpoint reached
at com.google.android.material.snackbar.BaseTransientBottomBar.updateMargins(BaseTransientBottomBar.java:438)
at com.google.android.material.snackbar.BaseTransientBottomBar.onAttachedToWindow(BaseTransientBottomBar.java:737)
at com.google.android.material.snackbar.BaseTransientBottomBar$SnackbarBaseLayout.onAttachedToWindow(BaseTransientBottomBar.java:1221)
at android.view.View.dispatchAttachedToWindow(View.java:20479)
at android.view.ViewGroup.dispatchAttachedToWindow(ViewGroup.java:3489)
at android.view.ViewGroup.addViewInner(ViewGroup.java:5278)
at android.view.ViewGroup.addView(ViewGroup.java:5064)
at android.view.ViewGroup.addView(ViewGroup.java:5004)
at android.view.ViewGroup.addView(ViewGroup.java:4976)
at com.google.android.material.snackbar.BaseTransientBottomBar$SnackbarBaseLayout.addToTargetParent(BaseTransientBottomBar.java:1275)
at com.google.android.material.snackbar.BaseTransientBottomBar.showView(BaseTransientBottomBar.java:715)
at com.google.android.material.snackbar.BaseTransientBottomBar$1.handleMessage(BaseTransientBottomBar.java:244)
at android.os.Handler.dispatchMessage(Handler.java:102)
...
Breakpoint reached
at com.google.android.material.snackbar.BaseTransientBottomBar.updateMargins(BaseTransientBottomBar.java:438)
at com.google.android.material.snackbar.BaseTransientBottomBar.recalculateAndUpdateMargins(BaseTransientBottomBar.java:840)
at com.google.android.material.snackbar.BaseTransientBottomBar.showView(BaseTransientBottomBar.java:716)
at com.google.android.material.snackbar.BaseTransientBottomBar$1.handleMessage(BaseTransientBottomBar.java:244)
at android.os.Handler.dispatchMessage(Handler.java:102)
...
And here's traceback for †
Breakpoint reached
at com.google.android.material.snackbar.BaseTransientBottomBar.updateMargins(BaseTransientBottomBar.java:438)
at com.google.android.material.snackbar.BaseTransientBottomBar.access$900(BaseTransientBottomBar.java:96)
at com.google.android.material.snackbar.BaseTransientBottomBar$3.onApplyWindowInsets(BaseTransientBottomBar.java:383)
at androidx.core.view.ViewCompat$Api21Impl$1.onApplyWindowInsets(ViewCompat.java:4858)
at android.view.View.dispatchApplyWindowInsets(View.java:11309)
at android.view.ViewGroup.dispatchApplyWindowInsets(ViewGroup.java:7320)
at android.view.ViewGroup.brokenDispatchApplyWindowInsets(ViewGroup.java:7334)
at android.view.ViewGroup.dispatchApplyWindowInsets(ViewGroup.java:7325)
at android.view.ViewGroup.brokenDispatchApplyWindowInsets(ViewGroup.java:7334)
at android.view.ViewGroup.dispatchApplyWindowInsets(ViewGroup.java:7325)
at androidx.drawerlayout.widget.DrawerLayout.onMeasure(DrawerLayout.java:1128)
at android.view.View.measure(View.java:25466)
at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:6957)
at android.widget.FrameLayout.onMeasure(FrameLayout.java:194)
at androidx.appcompat.widget.ContentFrameLayout.onMeasure(ContentFrameLayout.java:145)
at android.view.View.measure(View.java:25466)
at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:6957)
at android.widget.FrameLayout.onMeasure(FrameLayout.java:194)
at android.view.View.measure(View.java:25466)
at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:6957)
at android.widget.FrameLayout.onMeasure(FrameLayout.java:194)
at android.view.View.measure(View.java:25466)
at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:6957)
at android.widget.LinearLayout.measureChildBeforeLayout(LinearLayout.java:1552)
at android.widget.LinearLayout.measureVertical(LinearLayout.java:842)
at android.widget.LinearLayout.onMeasure(LinearLayout.java:721)
at android.view.View.measure(View.java:25466)
at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:6957)
at android.widget.FrameLayout.onMeasure(FrameLayout.java:194)
at com.android.internal.policy.DecorView.onMeasure(DecorView.java:747)
at android.view.View.measure(View.java:25466)
at android.view.ViewRootImpl.performMeasure(ViewRootImpl.java:3402)
at android.view.ViewRootImpl.measureHierarchy(ViewRootImpl.java:2246)
at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:2544)
at android.view.ViewRootImpl.doTraversal(ViewRootImpl.java:1948)
at android.view.ViewRootImpl$TraversalRunnable.run(ViewRootImpl.java:8177)
at android.view.Choreographer$CallbackRecord.run(Choreographer.java:972)
at android.view.Choreographer.doCallbacks(Choreographer.java:796)
at android.view.Choreographer.doFrame(Choreographer.java:731)
at android.view.Choreographer$FrameDisplayEventReceiver.run(Choreographer.java:957)
at android.os.Handler.handleCallback(Handler.java:938)
at android.os.Handler.dispatchMessage(Handler.java:99)
* Removed unnecessary callbacks
When adding a single note, it is not required to show continuous progress since it a short task and user can be notified after the task has been finished. So instead of using onProgressUpdate callback, it is good to use onPostExecute for UI updates while adding notes.
* Code simplification in NoteEditor.SaveNoteHandler
Both the conditions perform same task, sequential execution of both eventually nullifies the effect of !closeEditorAfterSave, so can be removed.
Themes.kt goal is to fit Themes utilities.
Having two files to do the same thing is confusing, scatters the code and may be overcomplicated especially for new contributors.
Change the javadoc of Themes, as it was describing the same as `updateCurrentTheme()`
NotetypeId is defined upstream in models.py.
Since we may not convert Models.java, using the definition from PythonTypes is
the best solution.
Those variables were found by searching for "mid: Long" and "odelId: Long".
https://github.com/ankidroid/Anki-Android/pull/11849#issuecomment-1211775736
Android's onCreateOptionsMenu does not play well with coroutines, as
it expects the menu to have been fully configured by the time the routine
returns. This results in flicker, as the menu gets blanked out, and then
configured a moment later when the coroutine runs. To work around this,
the current state is stored in the deck picker, so that we can redraw the
menu immediately, and then make any potential changes in the background.
Other changes:
- refactored onCreateOptionsMenu to make it simpler
- instead of the sdCardAvailable checks (which I assume is a proxy for
"col is available"), the entire menu is wrapped in a group, and the
visibility of the group is toggled depending on whether the col is available
or not. This also fixes the error on a full sync.
- there are three sets of unit tests (one for search icon, one for sync icon,
one for entire menu) that have been a pain since I originally introduced
this PR, and and I've sunk a number of hours into trying to get them to work
properly at this point. The issue appears to be that when mixing coroutine
calls and invalidateOptionsMenu(), onCreateOptionsMenu() is not getting called
before trying to await the job, leading to a hang or stale data. I tried
advancing robolectric, but it did not help. Maybe someone more experienced
in this area can figure it out, but for now I've changed these routines
to be more of a unit test and less of an integration test: rather than
checking the menu itself, they directly invoke the function that updates
the menu state, and check the state instead. This takes onCreateOptionsMenu()
out of the loop, and avoids the problems (and probably allows these tests
to be re-enabled on Windows as well). The sync tests I've removed, as the
entire menu is hidden/shown now when the col is closed, so they are redundant.
Currently there are many instances in the AnkiDroid codebase where
the collection is accessed a) on the UI thread, blocking the UI, and
b) in unsafe ways (eg by checking to see if the collection is open,
and then assuming it will remain open for the rest of a method call.
"Fix full download crashing in new schema case" (159a108dcb) demonstrates
a few of these cases.
This PR is an attempt at addressing those issues. It introduces
a `withCol` function that is intended to be used instead of
CollectionHelper.getCol(). For example, code that previously looked
like:
val col = CollectionHelper.getInstance().getCol(this)
val count = col.decks.count()
Can now be used like this:
val count = withCol { decks.count() }
The block is run on a background thread, and other withCol calls made
in parallel will be queued up and executed sequentially. Because of
the exclusive access, routines can safely close and reopen the
collection inside the block without fear of affecting other callers.
It's not practical to update all the legacy code to use withCol
immediately - too much work is required, and coroutines are not
accessible from Java code. The intention here is that this new path is
gradually bought into. Legacy code can continue calling CollectionHelper.
getCol(), which internally delegates to CollectionManager.
Two caveats to be aware of:
- Legacy callers will wait for other pending operations to complete
before they receive the collection handle, but because they retain it,
subsequent access is not guaranteed to be exclusive.
- Because getCol() and colIsOpen() are often used on the main thread,
they will block the UI if a background operation is already running.
Logging has been added to help diagnose this, eg messages like:
E/CollectionManager: blocked main thread for 2626ms:
com.ichi2.anki.DeckPicker.onCreateOptionsMenu(DeckPicker.kt:624)
Other changes:
- simplified CoroutineHelpers
- added TR function for accessing translations without a col reference
- onCreateOptionsMenu() needed refactoring to avoid blocking the UI.
- The blocking colIsOpen() call in onPrepareOptionsMenu() had to be
removed. I can not reproduce the issue it reports, and the code checks
for col in onCreateOptionsMenu().
- The subscribers in ChangeManager need to be cleared out at the start
of each test, or two tests are flaky when run with the new schema.
