Create a Sequencing snackbar feature by develric · Pull Request #10856 · wordpress-mobile/WordPress-Android

develric · 2019-11-26T21:50:14Z

Fixes #10771 and supersedes #10820

The code in this PR was mostly done in Pair Programming/Brainstorming with @malinajirka , so credits to him 🙇‍♂️.

The basic idea is to introduce a snackbar sequencer to avoid that multiple snackbars, triggered nearly together, overlap not allowing to the user to check/understand the ongoing messages. We introduced a SnackbarSequencer class and a SnackbarSequencerInfo class. The client code can enqueue SnackbarSequencerInfo element into the SnackbarSequencer instead of directly use the WPSnackbar class. In this way the sequencer consumes the queue of SnackbarSequencerInfo visualuazing in sequence the snackbars.

To avoid memory leaks due to the fact that we keep into the sequencer queue references to view/context and listeners that can eventually be no more active at the time we try to show the snackbar, we keep those references as Weak/Soft references.

For now we have used the sequencer only in upload posts related snackbars to limit the impact as much as possible to the #10771 issue. It will be eventually extended in a future time to all snackbars. NOTE: only snackbars that are not Snackbar.LENGTH_INDEFINITE are allowed, otherwise an IllegalArgumentException is triggered.

For the unit testing we introduced kotlinx-coroutines-test version 1.2.1 in the testImplementation tree. In this way we can use the advanceTimeBy function to test the coroutine/delay.

Pinging also @planarvoid to have a third couple of eyes on it.

To test:

Use the instructions in the Failed upload Snackbar shown after successful upload #10771 (or use Charles Proxy and its Block List feature) to trigger an upload fail and check that now the multiple snackbars are shown in sequence and you can open the snackbar actions in between
(this is tricky to execute and describe tbh) Use the AS to start debugging WP app and attach a memory profiler session:
- place a breakpoint in SnackbarSequencer.start (in val item = snackBarQueue.peek())
- Open PostsList > Drafts and trash a post
- the above breakpoint is hit
- in the app exit from PostsListActivity using the back button and going to the main activity
- in the profiler check that the PostsListActivity is still in memory
- resume debugger and check in the logcat that you get the message SnackbarSequencer > start context was null or SnackbarSequencer > start context was not alive (this means the snackbar got the Activity was no more valid)
- in the memory profiler force some cycles of garbage collection (using the bin icon); and check the PostsListActivity gets garbaged (can need 10/12 cycles or maybe more depending on memory pressure)
Unit testing must pass

PR submission checklist:

I have considered adding unit tests where possible.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

…f show

…app. Leaving show method only where duration was Snackbar.LENGTH_INDEFINITE

peril-wordpress-mobile · 2019-11-26T21:50:20Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

peril-wordpress-mobile · 2019-11-26T21:56:27Z

You can test the changes on this Pull Request by downloading the APK here.

malinajirka

Thanks @develric, great job;)!

The tests look really good. I have left just one minor comment.

We might want to consider adding a test which would test the capacity. I'm not sure it's worth it, only if it would be straightforward. Wdyt?

I'd also consider decreasing the capacity to 5 items -> I can't imagine a real case scenario in which we'd like to show 30+ seconds old snackbar. Wdyt?

I'm going to test the behavior now. ;)

WordPress/src/test/java/org/wordpress/android/util/SnackbarSequencerConcurrentTest.kt

WordPress/src/main/java/org/wordpress/android/util/SnackbarSequencer.kt

malinajirka · 2019-11-27T10:28:16Z

I've tested the app and it works as expected except of the animation issue I mentioned above. It seems the PostsListActivity is getting gc collected even when there are pending snackbars, so that's working as expected as well ;).

@planarvoid Since we wrote big parts of the code with Riccardo, I'll leave the review + approval + merge to you. Thanks! ;)

WordPress/src/main/java/org/wordpress/android/util/SnackbarSequencerInfo.kt

planarvoid · 2019-11-27T14:04:58Z

WordPress/src/main/java/org/wordpress/android/util/SnackbarSequencer.kt

+        get() = bgDispatcher + job
+
+    fun enqueue(item: SnackbarSequencerInfo) {
+        synchronized(this@SnackbarSequencer) {


I'd consider using launch(mainThread) here instead since using the main thread would ensure synchronous access and I think it makes it easier to understand. There is also no heavy work on the thread so I don't think it's even necessary to go on the bg thread.

Ohh, great idea! I was curious so I tested it and it works as expected and the code is easier to understand.

Here is a patch in case you want to use it @develric - https://cloudup.com/cdAXfGQft6N

Makes sense, thanks for the patch 👍!

WordPress/src/main/java/org/wordpress/android/util/SnackbarSequencerInfo.kt

… to SnackbarItem.

develric · 2019-11-28T11:24:10Z

Hi @malinajirka and @planarvoid , i should have followed up to all your comments. Waiting for your second pass review, thanks! 😊

malinajirka

Thanks @develric for all the changes ;)! (Great suggestions @planarvoid !)

I've reviewed the changes + tested the app + verified the activity is not leaking. LGTM - I'll leave the final review to Vojta!

WordPress/src/test/java/org/wordpress/android/util/SnackbarSequencerConcurrentTest.kt

malinajirka · 2019-11-28T11:39:32Z

WordPress/src/test/java/org/wordpress/android/util/SnackbarSequencerConcurrentTest.kt

+        advanceTimeBy(checkPoints[0] - 2 * SNACKBAR_DURATION_MARGIN)
+        verify(wpSnackbar, times(1)).show()
+        advanceTimeBy(2 * SNACKBAR_DURATION_MARGIN)
        verify(wpSnackbar, times(2)).show()


We might want to consider removing all the lines below this line (exclusive). I think we are technically testing the same thing as "snackbars are shown in sequence with the correct duration".It would make the purpose of the test clearer imo. Wdyt?

Well, I think you are right 😊! Let me know what you think of my answers to the previous points and I will change also this one as you suggested, thanks 🙇‍♂️!

Maybe we can consider this addressed here?

planarvoid

Looks good 👍, I have two minor comments and I need to spend a little more time testing this PR :-)

planarvoid · 2019-11-28T14:58:14Z

WordPress/src/main/java/org/wordpress/android/util/SnackbarItem.kt

+
+const val INDEFINITE_SNACKBAR_NOT_ALLOWED = "Snackbar.LENGTH_INDEFINITE not allowed in getSnackbarDurationMs."
+
+fun getSnackbarDurationMs(snackbarItem: SnackbarItem): Long {


Do you think this could be a method in the SnackbarItem class? I think it would make it little better 👍

Good point, agree! Changed here f0dba9a

WordPress/src/main/java/org/wordpress/android/util/SnackbarItem.kt

planarvoid

I think it works well, good job! So much better than before 👍 I don't feel strongly about the tests so feel free to change them or leave them as they are.

WordPress/src/test/java/org/wordpress/android/util/SnackbarSequencerConcurrentTest.kt

develric · 2019-11-28T22:07:49Z

Hey @malinajirka, @planarvoid ! Thanks both of you for the thorough review and comments! I hope I followed up to all of them accordingly. Let me know wdyt! 😊

planarvoid

Looks great 👍

malinajirka

LGTM, thank you for both for the suggestions + changes 🙇

mzorz · 2020-03-26T14:09:33Z

While working on some other feature I need to user Snackbars for, found out about the Sequencer and so came here looking for info about why / how the Sequencer was needed, given the following:

The basic idea is to introduce a snackbar sequencer to avoid that multiple snackbars, triggered nearly together, overlap not allowing to the user to check/understand the ongoing messages.

collided with my assumptions about how Snackbars work, as can be read in the training documentation:

The system does not show multiple Snackbar objects at the same time, so if the view is currently displaying another Snackbar, the system queues your Snackbar and displays it after the current Snackbar expires or is dismissed.

I ran a quick test and called Snackbar several times only to confirm what you guys noted - any snackbar currently being shown on the screen will be closed immediately as a new call to .show() is issued 💥

Been reading the code implementation of Snackbar and SnackbarManager in the support library. Looks like other people have discovered the same issue as well, but their workarounds are much simpler given they still could count on setCallback() not being deprecated 😕.

I guess this is yet another inconsistency in the Android OS 🙃 - I wonder why they don't fix it
[...]
Went to Android's issue tracker to search for it and / or open a new issue and saw this one is closed as won't fix https://issuetracker.google.com/issues/37069975 😞

Thanks for the work done here @develric @malinajirka @planarvoid , hope this can help others as well. 👏

develric and others added 17 commits November 19, 2019 22:45

Adding SnackbarSequencer to the WPSnackbar

00b62c4

Changing relevant UploadUtils methods to use addToSequencer instead o…

44a2628

…f show

Using addToSequencer instead of show where WPSnackbar is used in the …

7393633

…app. Leaving show method only where duration was Snackbar.LENGTH_INDEFINITE

Restoring some files to their original version.

f9ed833

Refactoring with weakreference.

787482f

Refactored with SoftReference

42454e5

Adding queue to Sequencer.

3ca8e7b

Refactor SnackbarSequencer

7c5b2f8

Make WPSnackbar testable

48f7739

Replace static access to uploadUtils with an injectable wrapper

c943f45

Refactor SnackbarSequencer and its info

b7bd62b

Fix lint issues

a1667c2

Remove unnecessary context parameters

7260517

Use SnackBarSequencer in EditPostActivity

7676b67

Removing unneeded commented lines.

d2cce70

Moving getSnackbarDurationMs to SnackbarSequencerInfo.kt

48e9056

Adding basic unit testing for the SnackbarSequencer.

f73f5ee

develric added [Type] Enhancement Posting/Editing labels Nov 26, 2019

develric added this to the 13.8 milestone Nov 26, 2019

develric requested review from malinajirka and planarvoid November 26, 2019 21:50

develric mentioned this pull request Nov 26, 2019

Create a Sequencing snackbar feature #10820

Closed

2 tasks

malinajirka reviewed Nov 27, 2019

View reviewed changes

WordPress/src/test/java/org/wordpress/android/util/SnackbarSequencerConcurrentTest.kt Show resolved Hide resolved

malinajirka reviewed Nov 27, 2019

View reviewed changes

WordPress/src/main/java/org/wordpress/android/util/SnackbarSequencer.kt Outdated Show resolved Hide resolved

planarvoid self-assigned this Nov 27, 2019

planarvoid reviewed Nov 27, 2019

View reviewed changes

WordPress/src/main/java/org/wordpress/android/util/SnackbarSequencerInfo.kt Outdated Show resolved Hide resolved

planarvoid reviewed Nov 27, 2019

View reviewed changes

WordPress/src/main/java/org/wordpress/android/util/SnackbarSequencerInfo.kt Outdated Show resolved Hide resolved

develric added 7 commits November 28, 2019 03:24

Reduced snackbar queue to 5 elements and rename SnackbarSequencerInfo…

b8290a0

… to SnackbarItem.

Moving sequencer logic to main thread.

9bc72a0

Aligning unit testing.

c97e592

Fixing lint.

015b85f

Changing callback creation.

3f60566

Defaulting callback to null.

756ba50

Fixing Lint.

85aff0c

develric requested review from malinajirka and planarvoid November 28, 2019 11:24

malinajirka reviewed Nov 28, 2019

View reviewed changes

planarvoid reviewed Nov 28, 2019

View reviewed changes

planarvoid approved these changes Nov 28, 2019

View reviewed changes

develric added 2 commits November 28, 2019 18:25

Moving fun getSnackbarDurationMs into SnackbarItem.

f0dba9a

Change unit test as per review comments.

353e66d

wpmobilebot reviewed Nov 28, 2019

View reviewed changes

WordPress/src/test/java/org/wordpress/android/util/SnackbarSequencerConcurrentTest.kt Outdated Show resolved Hide resolved

Fixing linter.

e162fef

planarvoid approved these changes Nov 29, 2019

View reviewed changes

malinajirka approved these changes Nov 29, 2019

View reviewed changes

malinajirka merged commit eca7eed into develop Nov 29, 2019

malinajirka deleted the try/snackbar-sequencer branch November 29, 2019 12:38

mzorz mentioned this pull request Mar 27, 2020

Start / Success / Error snackbars Automattic/stories-android#286

Merged

1 task

ashiagr mentioned this pull request Sep 1, 2021

Quick Start Tour Tweaks #15258

Open


		const val INDEFINITE_SNACKBAR_NOT_ALLOWED = "Snackbar.LENGTH_INDEFINITE not allowed in getSnackbarDurationMs."

		fun getSnackbarDurationMs(snackbarItem: SnackbarItem): Long {

Conversation

develric commented Nov 26, 2019

To test:

Uh oh!

peril-wordpress-mobile bot commented Nov 26, 2019

Uh oh!

peril-wordpress-mobile bot commented Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malinajirka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

malinajirka commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

planarvoid Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malinajirka Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

malinajirka Nov 27, 2019

Choose a reason for hiding this comment

Uh oh!

develric Nov 28, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

develric commented Nov 28, 2019

Uh oh!

malinajirka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

malinajirka Nov 28, 2019

Choose a reason for hiding this comment

Uh oh!

develric Nov 28, 2019

Choose a reason for hiding this comment

Uh oh!

develric Nov 28, 2019

Choose a reason for hiding this comment

Uh oh!

planarvoid left a comment

Choose a reason for hiding this comment

Uh oh!

planarvoid Nov 28, 2019

Choose a reason for hiding this comment

Uh oh!

develric Nov 28, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

planarvoid left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

develric commented Nov 28, 2019

Uh oh!

planarvoid left a comment

Choose a reason for hiding this comment

Uh oh!

malinajirka left a comment

Choose a reason for hiding this comment

Uh oh!

mzorz commented Mar 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

peril-wordpress-mobile bot commented Nov 26, 2019 •

edited

Loading

malinajirka commented Nov 27, 2019 •

edited

Loading

planarvoid Nov 27, 2019 •

edited

Loading

malinajirka Nov 27, 2019 •

edited

Loading