ButtonToast doesn't do anything with duration parameter

RESOLVED FIXED in Firefox 34

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Margaret, Assigned: amoghbl1, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 34
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec+)

Details

(Whiteboard: [lang=java][lang=js])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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.
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.
(Reporter)

Comment 2

4 years ago
(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
(Reporter)

Comment 3

4 years ago
(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.
Flags: needinfo?(alam)
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.
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.
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.
Flags: needinfo?(alam)
(Reporter)

Comment 7

4 years ago
(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() :)
(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.
(Reporter)

Comment 9

4 years ago
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.
(Reporter)

Updated

4 years ago
Blocks: 701725, 997288
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.
(Reporter)

Comment 11

4 years ago
(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.
tracking-fennec: ? → +
(Reporter)

Comment 12

4 years ago
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.
Whiteboard: [mentor=margaret][lang=java][lang=js]
Can you clarify? "Fixing that toast API" is vague.
(Reporter)

Comment 14

4 years ago
(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
Mentor: margaret.leibovic@gmail.com
Whiteboard: [mentor=margaret][lang=java][lang=js] → [lang=java][lang=js]
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?
Assignee: nobody → amoghbl1
(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
(Assignee)

Comment 17

4 years ago
Created attachment 8462875 [details] [diff] [review]
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?
Attachment #8462875 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 18

4 years ago
Created attachment 8462892 [details] [diff] [review]
temp_1018417.patch
Attachment #8462892 - Flags: review?(wjohnston)
Attachment #8462892 - Flags: review?(margaret.leibovic)
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Reporter)

Updated

4 years ago
Blocks: 1044296
(Reporter)

Comment 19

4 years ago
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.
Attachment #8462875 - Attachment is obsolete: true
Attachment #8462875 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 20

4 years ago
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.
Attachment #8462892 - Flags: review?(margaret.leibovic) → feedback+
(Assignee)

Comment 21

4 years ago
Created attachment 8464290 [details] [diff] [review]
fix_1018417.patch

This is the new updated patch with all the changes suggested by :margaret .
Attachment #8462892 - Attachment is obsolete: true
Attachment #8462892 - Flags: review?(wjohnston)
Attachment #8464290 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 22

4 years ago
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.
Attachment #8464290 - Flags: review?(margaret.leibovic) → feedback+
(Assignee)

Comment 23

4 years ago
Created attachment 8464970 [details] [diff] [review]
fix_1018417_v2.patch

Thanks for pointing those out :margaret, made the necessairy changes.
Attachment #8464290 - Attachment is obsolete: true
Attachment #8464970 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 24

4 years ago
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!
Attachment #8464970 - Flags: review?(margaret.leibovic) → review-
(Assignee)

Comment 25

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

Comment 26

4 years ago
Created attachment 8465270 [details] [diff] [review]
fix_1018417_v3.patch
Attachment #8464970 - Attachment is obsolete: true
Attachment #8465270 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 27

4 years ago
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.
Attachment #8465270 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/246fab00de4a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
You need to log in before you can comment on or make changes to this bug.