1018417 - ButtonToast doesn't do anything with duration parameter

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

I was thinking that the new toasts we introduced are hanging around for too long, and in fact, they're not obeying that "short" duration. They're hard-coded to use a long duration:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ButtonToast.java#125

Is this intentional? It seems like an oversight to me.

Comment 1

[Wesley Johnston (:wesj)]

Semi-Intentional. I didn't want to allow "asking" for user input for a short period of time. i.e. people need time to read the toast, think about it, and then decide if they want to tap before it disappears.

Comment 2

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #0)
> I was thinking that the new toasts we introduced are hanging around for too
> long, and in fact, they're not obeying that "short" duration. They're
> hard-coded to use a long duration:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/
> ButtonToast.java#125

Actually, I'm wrong, this is not what's dictating the duration the toast is showing, but it's still hard-coded:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ButtonToast.java#35

Comment 3

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #1)
> Semi-Intentional. I didn't want to allow "asking" for user input for a short
> period of time. i.e. people need time to read the toast, think about it, and
> then decide if they want to tap before it disappears.

At the very least, we should update the documentation to explain that this is how button toasts work. However, I feel like they hang around a super long time (5 seconds!), and it should be up to the developer to decide how they want to use the UI widget.

Let's ask antlam what he thinks.

Comment 4

[Chenxia Liu [:liuche]]

Yeah, I was looking at that too, and I found it really annoying that the toast hung around for so long. I feel like personally, my window for undoing a closed tab is either ~a second, or 10 minutes later. 5 seconds seems way too long.

Comment 5

[Wesley Johnston (:wesj)]

The use case here is a bit different from our original use case. i.e. we were originally trying to expose functionality that was hard to find. Flashing it by for 2 seconds defeated the purpose. Maybe in these cases it doesn't (although I don't really think 5s is that long). Alternatively, maybe with these we lean more towards "Show these until the user starts doing something else" rather than having timeouts.

Comment 6

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I think these are all fair points. To me, it would seem as though there are a couple tweaks we could make around positioning and duration, but these would also depend on situation (as Wes pointed out).

Can we try moving the toast down closer to the bottom? I'd like to try a distance from the bottom equal to ~80% of the height of the actual toast.

For the undo-ing a deleted toast, it could probably stay at 5 seconds since this is really the only chance to recover after a delete action. Maybe position and transparency will help it feel less "in your face".

For the switching-to-tab toast, 3 seconds seems enough to me (maybe we could give that a try and see?). Again, the overall position/transparency might help get it out of the way a bit more.

Comment 7

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #6)

> For the undo-ing a deleted toast, it could probably stay at 5 seconds since
> this is really the only chance to recover after a delete action. Maybe
> position and transparency will help it feel less "in your face".
> 
> For the switching-to-tab toast, 3 seconds seems enough to me (maybe we could
> give that a try and see?). Again, the overall position/transparency might
> help get it out of the way a bit more.

These points argue in favor of letting the individual toasts decide how long they show for, which means that we should first fix ButtonToast to do something with the "short" or "long" parameters passes into toast.show() :)

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Anthony Lam (:antlam) from comment #6)

> Can we try moving the toast down closer to the bottom? I'd like to try a
> distance from the bottom equal to ~80% of the height of the actual toast.

Might be an issue. We currently try to mimic the platform toast, or we should. I don't want to get too different for ButtonToast if possible.

> For the undo-ing a deleted toast, it could probably stay at 5 seconds since
> this is really the only chance to recover after a delete action. Maybe
> position and transparency will help it feel less "in your face".
> 
> For the switching-to-tab toast, 3 seconds seems enough to me (maybe we could
> give that a try and see?). Again, the overall position/transparency might
> help get it out of the way a bit more.

I agree with Margret. Let's make the API accept some parameters and then tweak the callers as desired.

Comment 9

[:Margaret Leibovic]

I was just using gmail and decided to pay closer attention to what they do. It appears they keep the undo toast around indefinitely, until you touch the screen somewhere else. I think we should experiment with that, since that seems to optimize both for users who take a long time to read and process what's going on, as well as those who don't want to do anything with the action and just move on with other tasks.

Comment 10

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Awesome!

The more I think about it the more I can tell a minor transparency will help with this too. Can we try to implement that as well? We could start by assuming what looks closest to native transparency for the time being.

Comment 11

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #10)
> Awesome!
> 
> The more I think about it the more I can tell a minor transparency will help
> with this too. Can we try to implement that as well? We could start by
> assuming what looks closest to native transparency for the time being.

We should do this in a separate bug, it sounds like another follow-up to bug 997288.

Comment 12

[:Margaret Leibovic]

Let's make this bug about fixing the toast API, and bug 1019735 can be about fixing the more immediately problem of these toasts hanging around too long and blocking things.

Comment 13

[Wesley Johnston (:wesj)]

Can you clarify? "Fixing that toast API" is vague.

Comment 14

[:Margaret Leibovic]

(In reply to Wesley Johnston (:wesj) from comment #13)
> Can you clarify? "Fixing that toast API" is vague.

I mean fixing NativeWindow.toast.show.

Right now NativeWindow.toast.show("foo", "short", { button: ... }) does not do anything with the second parameter. Instead it should show the toast for a "short" time (or a "long" time if that was what was specified).

https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/NativeWindow/toast/show

Comment 15

[Wesley Johnston (:wesj)]

Snorp was complaining about this being ignored yesterday. I wonder if we could re-enable it for api consumes with the option (default?) to use the current behavior?

i.e.
duration = "short" = 1sec
duration = "long" = 3sec
duration = "touch" = until the user touches the browser somewhere else?

maybe we could even 'or' those together?

duration = "short|touch" for 1 sec or until the user touches fennec?
duration = "short&touch" wait at least one second and then wait until the user touches fennec? (that would have best suited snorp yesterday I think).

I think we can do the first part easily. The second would be a separate bug. Ideas? Opinions?

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #15)
> Snorp was complaining about this being ignored yesterday. I wonder if we
> could re-enable it for api consumes with the option (default?) to use the
> current behavior?
> 
> i.e.
> duration = "short" = 1sec
> duration = "long" = 3sec
> duration = "touch" = until the user touches the browser somewhere else?

I like this idea

> maybe we could even 'or' those together?
> 
> duration = "short|touch" for 1 sec or until the user touches fennec?
> duration = "short&touch" wait at least one second and then wait until the
> user touches fennec? (that would have best suited snorp yesterday I think).

I like this too

> I think we can do the first part easily. The second would be a separate bug.
> Ideas? Opinions?

Two bugs are better than one

Comment 17

[amoghbl1]

Attachment: temp_1018417.patch

OK, this is a completely temporary patch as I've had to change function calls in a couple of places in order to make it work. Also, could you tell me if this is fine so that I may submit the actual patch? :wesj , your thoughts?

Comment 18

[amoghbl1]

Attachment: temp_1018417.patch

Comment 19

[:Margaret Leibovic]

Comment on attachment 8462875 [details] [diff] [review]
temp_1018417.patch

FYI, you should mark patches as obsolete when they're no longer the most up-to-date version.

Comment 20

[:Margaret Leibovic]

Comment on attachment 8462892 [details] [diff] [review]
temp_1018417.patch

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

This is going in the right direction, but I'd like to see us use constants for the short/long values to make this API more robust.

::: mobile/android/base/GeckoApp.java
@@ +608,5 @@
>                  final String label = button.optString("label", "");
>                  final String icon = button.optString("icon", "");
>                  final String id = button.optString("id", "");
> +                showButtonToast(msg, duration, label, icon, id);
> +            } else 

You should keep the braces around this else statement.

@@ +790,4 @@
>          BitmapUtils.getDrawable(GeckoApp.this, buttonIcon, new BitmapUtils.BitmapLoader() {
>              @Override
>              public void onBitmapFound(final Drawable d) {
> +                getButtonToast().show(false, message, duration ,buttonText, d, new ButtonToast.ToastListener() {

We should convert the duration string that comes from Gecko into a constant to pass to the ButtonToast API, similar to what we do in showNormalToast.

::: mobile/android/base/widget/ButtonToast.java
@@ +122,5 @@
>          mButton.setCompoundDrawablesWithIntrinsicBounds(t.buttonDrawable, null, null, null);
>  
>          mHideHandler.removeCallbacks(mHideRunnable);
> +        // Check if the toast duration is short or long.
> +        if (t.duration.equals("short"))

Instead of doing string comparisons, I think we should declare static integer constants for short and long, similar to Toast.LENGTH_LONG and Toast.LENGTH_SHORT:
http://developer.android.com/reference/android/widget/Toast.html#LENGTH_LONG

I also think we should consider making LENGTH_SHORT the default, since that's the case for Android Toasts.

Then consumers would just pass those values into the ButtonToast API.

@@ +125,5 @@
> +        // Check if the toast duration is short or long.
> +        if (t.duration.equals("short"))
> +            mHideHandler.postDelayed(mHideRunnable, 3000);
> +        else
> +            mHideHandler.postDelayed(mHideRunnable, TOAST_DEFAULT_DURATION);

Nit: Brace your if/else statements.

Comment 21

[amoghbl1]

Attachment: fix_1018417.patch

This is the new updated patch with all the changes suggested by :margaret .

Comment 22

[:Margaret Leibovic]

Comment on attachment 8464290 [details] [diff] [review]
fix_1018417.patch

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

This is looking good! I just have some more comments for you to address before we land this.

::: mobile/android/base/BrowserApp.java
@@ +2551,5 @@
>                  } else {
>                      tab.addBookmark();
>                      getButtonToast().show(false,
>                          getResources().getString(R.string.bookmark_added),
> +                        ButtonToast.TOAST_SHORT ,

Nit: Get rid of the space before the comma.

::: mobile/android/base/GeckoApp.java
@@ +613,3 @@
>                  showNormalToast(msg, duration);
>              }
> +            

Looks like you accidentally added some trailing whitespace here. Please make sure your editor is set to avoid this.

@@ +785,5 @@
>          return mToast;
>      }
>  
> +    void showButtonToast(final String message, final String duration,
> +                         final String buttonText,final String buttonIcon,

Nit: Please add a space after the comma.

@@ +793,5 @@
>              public void onBitmapFound(final Drawable d) {
> +                int toastDuration = ButtonToast.TOAST_SHORT;
> +                if (duration.equals("long")) {
> +                    toastDuration = ButtonToast.TOAST_LONG;
> +                }

You should just make this a final variable you declare one using a ternary operator.

final int toastDuration = duration.equals("long") ? ButtonToast.TOAST_LONG : ButtonToast.TOAST_SHORT;

@@ +794,5 @@
> +                int toastDuration = ButtonToast.TOAST_SHORT;
> +                if (duration.equals("long")) {
> +                    toastDuration = ButtonToast.TOAST_LONG;
> +                }
> +                

(Some more whitespace here, and other places. You should inspect the patch when you upload it to make sure you didn't accidentally include whitespace changes.)

@@ +795,5 @@
> +                if (duration.equals("long")) {
> +                    toastDuration = ButtonToast.TOAST_LONG;
> +                }
> +                
> +                getButtonToast().show(false, message, toastDuration ,buttonText, d, new ButtonToast.ToastListener() {

Nit: Please make sure to put the space after the comma, not before it (as you would when writing a list in a sentence).

::: mobile/android/base/home/HomeFragment.java
@@ +203,5 @@
>              final String buttonMessage = getResources().getString(R.string.switch_button_message);
>              final GeckoApp geckoApp = (GeckoApp) context;
>              geckoApp.getButtonToast().show(false,
>                      message,
> +                    duration,

Instead of declaring a local variable, just pass ButtonToast.TOAST_SHORT here directly.

::: mobile/android/base/widget/ButtonToast.java
@@ +32,5 @@
>      @SuppressWarnings("unused")
>      private final static String LOGTAG = "GeckoButtonToast";
>  
> +    public static int TOAST_SHORT = 3000;
> +    public static int TOAST_LONG = 5000;

Let's be consistent with Android's Toast and rename this LENGTH_SHORT and LENGTH_LONG.

http://developer.android.com/reference/android/widget/Toast.html#LENGTH_LONG

These can also be declared final.

Comment 23

[amoghbl1]

Attachment: fix_1018417_v2.patch

Thanks for pointing those out :margaret, made the necessairy changes.

Comment 24

[:Margaret Leibovic]

Comment on attachment 8464970 [details] [diff] [review]
fix_1018417_v2.patch

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

It looks like there are a whole bunch of extraneous changes to organize imports and add @Override tags. While this type of clean-up is welcome, it should happen in a different patch. Could you split these changes apart and file a new bug for the clean-up bits? Thanks!

Comment 25

[amoghbl1]

Oh, sorry, that wasn't supposed to happen! New ide settings did that I guess! Will fix them and update in a minute...

Comment 26

[amoghbl1]

Attachment: fix_1018417_v3.patch

Comment 27

[:Margaret Leibovic]

Comment on attachment 8465270 [details] [diff] [review]
fix_1018417_v3.patch

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

Excellent! I can land this patch for you.

Comment 28

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/246fab00de4a

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/246fab00de4a