880454 - Support a queue for button toasts

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Split off of the toast stuff in bug 872388, we need to better handle the case where two button toasts are shown at once (i.e. we should queue them up and show them in order).

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This creates a Toast class in ButtonToast that implements parcelable, and also fixes a bug where we weren't saving these when going into the background.

It also keeps a queue (LinkedList) of toasts and shows the next one in the queue whenever the toast is hidden.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch v2

I killed off the saved state stuff because I'm not sure we want to restore state any time we are killed anyway. Instead, I made the queue static. onAnimationEnd is called when we return from the background (while showing a toast), and will trigger walking the queue again.

Comment 3

[Brian Nicholson (:bnicholson)]

Comment on attachment 759962 [details] [diff] [review]
Patch v2

Review of attachment 759962 [details] [diff] [review]:
-----------------------------------------------------------------

r-'d for fixing the NPE and some cleanup.

::: mobile/android/base/widget/ButtonToast.java
@@ +26,5 @@
>  import android.view.View;
>  import android.widget.Button;
>  import android.widget.TextView;
>  
> +import java.util.Arrays;

Unused import

@@ +28,5 @@
>  import android.widget.TextView;
>  
> +import java.util.Arrays;
> +import java.util.LinkedList;
> +import java.util.List;

Unused import

@@ +35,5 @@
>  import org.mozilla.gecko.R;
>  
>  public class ButtonToast {
> +    private final static String LOGTAG = "GeckoButtonToast";
> +    private final static int TOAST_DURATION = 2000;

Did you lower from 5 -> 2 seconds just for testing, or did you mean to keep this at 2? 2 seconds seems a bit low.

@@ +68,5 @@
>  
>      public ButtonToast(View view, ToastListener listener) {
>          mView = view;
>          mListener = listener;
> +        sQueue = new LinkedList<Toast>();

Move this into a static initializer

@@ +80,2 @@
>                          hide(false);
> +                        mListener.onButtonClicked(t.token);

This can lead to a NPE (see https://bugzilla.mozilla.org/show_bug.cgi?id=872388#c30).

Note there's also a bug here where the click event can be associated with the wrong view. If two views are shown back to back and the button is held down on the first toast but released after the second toast appears, it will fire the callback for the second toast. Another reason to consider keeping the toast around as long as it's being touched.

@@ +94,3 @@
>  
> +    private void show(boolean immediate, Toast t) {
> +        synchronized (sQueue) {

I don't think this needs to be synchronized since show(), hide(), and onSaveInstanceState() must all be called from the UI thread.

@@ +154,4 @@
>      }
>  
> +    private void showNextInQueue() {
> +        synchronized (sQueue) {

Same here -- no need for synchronization.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v2

I bet lucas has opinions on this.

I added a check for isPressed on the button before hiding it. If it is pressed, we just reschedule the runnable. Its still possible that you tap the notification while its hiding and run into this sort of problem. I nulled out the mCurrentToast earlier in that case, disabled the button, and added a null check in the listener (that shouldn't be necessary anymore since the button is disabled, but in this case I don't really mind the extra safety).

Ideally, I think we'd cancel the fadeout animation and wait until the user lifted/moved their finger to decide what to do. I think that requires a MotionEvent listener and sticking a GestureDetector inside.

I started to do that and decided its overcomplicated.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v2

qrefed.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 763652 [details] [diff] [review]
Patch v2

Review of attachment 763652 [details] [diff] [review]:
-----------------------------------------------------------------

Giving r- just to get some more background on some of the aspects of the patch.

::: mobile/android/base/widget/ButtonToast.java
@@ +44,5 @@
>      private ToastListener mListener;
> +    private static LinkedList<Toast> sQueue;
> +    private Toast mCurrentToast;
> +    
> +    {

Add "static" for clarity.

@@ +53,5 @@
> +    private static class Toast {
> +        public CharSequence token;
> +        public CharSequence buttonMessage;
> +        public int buttonIcon;
> +        public CharSequence message;

Make all these properties final if your intentional is that Toast objects are immutable.

@@ +100,2 @@
>  
> +    private void show(boolean immediate, Toast t) {

Somehow I feel the order of these arguments should be "Toast, boolean".

@@ +100,3 @@
>  
> +    private void show(boolean immediate, Toast t) {
> +        if (mView.getVisibility() == View.VISIBLE) {

What's this check about. Not obvious.

@@ +101,5 @@
> +    private void show(boolean immediate, Toast t) {
> +        if (mView.getVisibility() == View.VISIBLE) {
> +            sQueue.offer(t);
> +            return;
> +        }

nit: empty line here.

@@ +123,5 @@
>          mView.startAnimation(alpha);
>      }
>  
>      public void hide(boolean immediate) {
> +        if (mButton.isPressed()) {

Ugh, this looks suspicious. What is this about?

@@ +142,5 @@
>              AlphaAnimation alpha = new AlphaAnimation(1.0f, 0.0f);
>              alpha.setDuration(duration);
>              alpha.setFillAfter(true);
>              alpha.setAnimationListener(new Animation.AnimationListener () {
> +                // if we are showing a toast and go in the background

if -> If

@@ -125,3 @@
>      }
>  
> -    public void onRestoreInstanceState(Bundle savedInstanceState) {

No state restoring anymore? Not sure I follow.

@@ +164,5 @@
>      }
>  
> +    private void showNextInQueue() {
> +        Toast t = sQueue.poll();
> +        if (t != null) {

What are the situations when t is null?

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v2.0.1

This is basically addressing the nit comments. Responses to questions below:

(In reply to Lucas Rocha (:lucasr) from comment #6)
> > +    private void show(boolean immediate, Toast t) {
> > +        if (mView.getVisibility() == View.VISIBLE) {
> What's this check about. Not obvious.

This is basically us queuing the item to show later. I added a comment here.

> >      public void hide(boolean immediate) {
> > +        if (mButton.isPressed()) {
> 
> Ugh, this looks suspicious. What is this about?

Brian pointed out that if you tap on a toast button and hold your finger, then lift it once the toast is gone, you can cause a crash. The idea was that we shouldn't hide a toast if you're touching it.

If the toast is in the process of hiding, I disabled the button so you shouldn't be able to press it. There are other ways to solve this (MotionEvent listeners), but this seemed the simplest to me. Happy to do something else...

> > -    public void onRestoreInstanceState(Bundle savedInstanceState) {
> No state restoring anymore? Not sure I follow.

Me and brian talked about this for a bit. You'll only hit this if Fennec has been killed while you were away. I think that in most (but not all cases) that would likely mean you haven't been using it for a bit, and it would seem odd to show an old toast. We decided that maybe the simplest thing to do was just to not show toasts in that situation. We could save a timestamp and try to use that to decide, but I'm not sure its worth the effort. Opinions welcome.

For cases where Fennec isn't killed, the toasts are already restored since onAnimationEnd is called when we return to Fennec and triggers showNextInQueue. We add the current toast back on the queue in onSaveInstanceState

> > +    private void showNextInQueue() {
> > +        Toast t = sQueue.poll();
> > +        if (t != null) {
> 
> What are the situations when t is null?

We poll the queue anytime when we finish showing a toast. "Is there anything else? If so, show it!". So I expect t == null to be the most common case.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 765643 [details] [diff] [review]
Patch v2.0.1

Review of attachment 765643 [details] [diff] [review]:
-----------------------------------------------------------------

Not sure I understand the state saving side of this patch. Just want some clarification before r+'ing it.

::: mobile/android/base/widget/ButtonToast.java
@@ +43,5 @@
>  
>      private ToastListener mListener;
> +    private static LinkedList<Toast> sQueue;
> +    private Toast mCurrentToast;
> +    

nit: remove trailing space.

@@ +51,4 @@
>  
>      // State objects
> +    private static class Toast {
> +        final public CharSequence token;

"public final", for consistency

@@ +84,2 @@
>                          hide(false);
> +                        if (mListener != null) {

This null check seems unrelated to this specific patch, no?

@@ +160,5 @@
>  
>      public void onSaveInstanceState(Bundle outState) {
> +        // Add whatever toast we're currently showing to the front of the queue
> +        if (mCurrentToast != null) {
> +            sQueue.add(0, mCurrentToast);

What's the assumption about state saving here? Not sure I follow. Is sQueue expected to live beyong the app life cycle somehow?

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch v2.0.2

(In reply to Lucas Rocha (:lucasr) from comment #8)
> @@ +84,2 @@
> >                          hide(false);
> > +                        if (mListener != null) {
> 
> This null check seems unrelated to this specific patch, no?

Unrelated in some sense. I pulled it into a second patch. Yay qcrecord extension!

> @@ +160,5 @@
> >  
> >      public void onSaveInstanceState(Bundle outState) {
> > +        // Add whatever toast we're currently showing to the front of the queue
> > +        if (mCurrentToast != null) {
> > +            sQueue.add(0, mCurrentToast);
> 
> What's the assumption about state saving here? Not sure I follow. Is sQueue
> expected to live beyong the app life cycle somehow?

Thought about this for a bit, and no, I don't think so. If the App is killed, killing GeckoApp.mToast, there's no reason to keep the queue around. It will not be shown until the next time someone shows a button toast. Made it a non-static variable.

Comment 10

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

This is the protection to make sure we don't crash if you touch the button while the toast is shown, wait for it to disappear, and then release the button.

I left the listener check in, because we don't require you to set mListener to something non-null ever. That becomes more useful with some of the refactoring I wound up doing in bug 884075 though, so that each toast could pass in their own button listener.

Comment 11

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #9)
> Thought about this for a bit, and no, I don't think so. If the App is
> killed, killing GeckoApp.mToast, there's no reason to keep the queue around.
> It will not be shown until the next time someone shows a button toast. Made
> it a non-static variable.

I thought you made it static so the queue wouldn't get lost when Don't Keep Activities is enabled.

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 769209 [details] [diff] [review]
Patch v2.0.2

I must have forgotten to r? here.

Comment 13

[Wesley Johnston (:wesj)]

Comment on attachment 769209 [details] [diff] [review]
Patch v2.0.2

lucas is out.

Comment 14

[Brian Nicholson (:bnicholson)]

Comment on attachment 769209 [details] [diff] [review]
Patch v2.0.2

Review of attachment 769209 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/widget/ButtonToast.java
@@ +27,5 @@
>  import android.widget.Button;
>  import android.widget.TextView;
>  
> +import java.util.LinkedList;
> +import android.util.Log;

Unused import

@@ +38,5 @@
>  
>      private View mView;
>      private TextView mMessageView;
>      private Button mButton;
>      private Handler mHideHandler = new Handler();

Nit: Make all of these final

@@ +41,5 @@
>      private Button mButton;
>      private Handler mHideHandler = new Handler();
>  
>      private ToastListener mListener;
> +    private LinkedList<Toast> mQueue = new LinkedList<Toast>();

Nit: Make both of these final

Comment 15

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54327a911bb0
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff7d39327dfe

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/54327a911bb0
https://hg.mozilla.org/mozilla-central/rev/ff7d39327dfe