1159296 - Add toast notification when trying to add the same link to the tab queue

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Overlays, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Steps to reproduce: 
1. Go to gmail app and tap on a link 
2. Wait a few seconds => "Nightly - 1 tab waiting" notification is displayed in the android notification bar 
3. Tap once again on the same link from gmail app 

Actual results:
- The link is loaded in a new tab 

Expected results:
- A notification should be displayed announcing that the same tab is already queued

Comment 1

[Teodora Vermesan (:TeoVermesan)]

Also tapping the "Nightly - 1 tab waiting" notification from the android notification bar does nothing

Comment 2

[Teodora Vermesan (:TeoVermesan)]

Also I saw in Bug 1154425 that Anthony suggested a notification: for example: "Tab already queued in Firefox" to inform users.

Comment 3

[Martyn Haigh (:mhaigh)]

Are you suggesting that we display the toast instead of opening the url?

There are pro's and con's for each method:

Double tap to open:
Pro: power feature; very useful
Con: non discoverable; perhaps the user double tapped because they forgot the feature was enabled and didn't see the toast

Double tap to show toast:
Pro: Doesn't break the tab queue functionality
Con: If the user double tapped because they didn't see the toast initially, another toast isn't going to help; more of a hassle to quickly open a tab

Comment 4

[Teodora Vermesan (:TeoVermesan)]

Yes, I suggest that a toast notification should be displayed instead of opening the dupe URL.
The toast notification should inform the user that the tab is already opened in the browser (for example: "Tab already queued in Firefox").

Since users will know about Tab Queue from the prompt (which informs them to turn on or to ignore the feature), they should already know that the tab will open in background.

Comment 5

[Martyn Haigh (:mhaigh)]

As per discussion with antlam - if tab already queued and not inside the X second window for double tap to open, display super toast with text "Tab already queued in Fx | Open Now" with the same functionality as the standard TQ toast

Comment 6

[Martyn Haigh (:mhaigh)]

Attachment: Add toast notification when trying to add the same link to the tab queue

Basic functionality is that if the tab has already been queued, we display a toast with the words "Tab already queued" with the same "Open Now" action.  

The difference in functionality is that:
- if there's no user interaction then the tab doesn't get queued again.
- if the "Open Now" button is pressed we have to remove the url from the tab queue data as it gets passed in via the intent.
- if the double tap to open feature is used, again we have to remove the url from the tab queue data for the same reason.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #4)
> Since users will know about Tab Queue from the prompt

I'm not sure we can make this assumption because some users may not remember - we removed the paint flashing preference (bug 1154504) because some users would turn it on by accident and they would have a bad experience.

(In reply to Martyn Haigh (:mhaigh) from comment #5)
> As per discussion with antlam - if tab already queued and not inside the X
> second window for double tap to open, display super toast with text "Tab
> already queued in Fx | Open Now" with the same functionality as the standard
> TQ toast

This is starting to sound complicated - we can try this out, but we need to make sure the user experience is intuitive - having two different outcomes for the same action might seem arbitrary from a user perspective. Maybe we need to do some user studies?

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8602836 [details] [diff] [review]
Add toast notification when trying to add the same link to the tab queue

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

Want to take a step back. If you disagree, feel free to reflag me for review.

::: mobile/android/base/tabqueue/TabQueueHelper.java
@@ +270,5 @@
>  
>          editor.apply();
>      }
> +
> +    public static boolean isUrlAlreadyQueued(final String url, final GeckoProfile profile, final String filename) {

nit: args can be more specific - which profile is this? This is the tabQueueFileName, right?

@@ +271,5 @@
>          editor.apply();
>      }
> +
> +    public static boolean isUrlAlreadyQueued(final String url, final GeckoProfile profile, final String filename) {
> +        JSONArray jsonArray = profile.readJSONArrayFromFile(filename);

nit: final, here and below.

@@ +277,5 @@
> +            String jsonUrl;
> +            try {
> +                jsonUrl = jsonArray.getString(i);
> +            } catch (JSONException e) {
> +                Log.w(LOGTAG, "Error parsing Tab Queue data.");

nit: Include the index number

@@ +280,5 @@
> +            } catch (JSONException e) {
> +                Log.w(LOGTAG, "Error parsing Tab Queue data.");
> +                continue;
> +            }
> +            if(TextUtils.equals(jsonUrl, url)) {

nit: `if (`

::: mobile/android/base/tabqueue/TabQueueService.java
@@ +98,3 @@
>  
> +        messageView = (TextView) toastLayout.findViewById(R.id.toast_message);
> +        messageView.setText(tabQueuedMessage);

We set this in onStartCommand - this line is unnecessary.

@@ +114,5 @@
>          toastLayoutParams.gravity = Gravity.BOTTOM | Gravity.CENTER_HORIZONTAL;
>      }
>  
>      @Override
>      public int onStartCommand(final Intent intent, final int flags, final int startId) {

I'm having difficulty following this - shall we take a step back?

Given the multiple states we can enter onStartCommand in, I think it'd make sense to do some branching. For example (some of this may be incorrect):

if (isFastDoubleTap) {
  openLinkNow();
  clearQueuedLinks();
} else if (isSlowDoubleTap) {
  displayAlreadyQueuedToast();
} else {
  queueLink();
  displayQueuedToast();
}

Seems much easier to read.

Another issue I'm having is understanding the state associated with the stopServiceRunnable. When you make comparisons to the state, consider storing the state in a temporary variable to give that state a name (and make it easier to make sense of). e.g.

boolean isToastShowing = stopServiceRunnable != null; // or whatever
if (isSomeState && isToastShowing) { /* Do stuff */ }

@@ +140,5 @@
>                      @Override
>                      public void run() {
> +                        // If there is no runnable around then the url has already been added to the list, so we'll
> +                        // need to remove it before proceeding or that url will open multiple times.
> +                        if(isUrlAlreadyQueued || stopServiceRunnable == null) {

When will isUrlAlreadyQueued be false but stopServiceRunnable == null true?

I don't think the latter is necessary.

@@ +166,5 @@
>                                      .putLong(GeckoPreferences.PREFS_TAB_QUEUE_LAST_TIME, System.currentTimeMillis())
>                                      .apply();
>          }
>  
> +        if(isUrlAlreadyQueued) {

nit: `if (`

@@ +186,3 @@
>              @Override
> +            public void onRun(boolean isUrlAlreadyQueued) {
> +                if(!isUrlAlreadyQueued) {

nit: `if (`

Comment 9

[Martyn Haigh (:mhaigh)]

Attachment: Add toast notification when trying to add the same link to the tab queue

So approach is the same, but I've cleaned up and grouped some code to make it more readable.  The reason I've kept the approach the same is that I think it actually works fairly well, once you get past the inherent complexity. 


> > +    public static boolean isUrlAlreadyQueued(final String url, final GeckoProfile profile, final String filename) {
> 
> nit: args can be more specific - which profile is this? 

I think this is the default profile being used at the moment (warning: I don't know a lot about our profile code!) - can you explain why this is important as I've just used the default so far without issue?

> This is the tabQueueFileName, right?

I think this is implied by the context.


> > +    public static boolean isUrlAlreadyQueued(final String url, final GeckoProfile profile, final String filename) {
> > +        JSONArray jsonArray = profile.readJSONArrayFromFile(filename);
> 
> nit: final, here and below.

This is just a WIP patch, but I've gone through the TabQueueHelper file and corrected all missing finals, there were a few (who ever reviewed those patches!?)

> > +                Log.w(LOGTAG, "Error parsing Tab Queue data.");
> 
> nit: Include the index number

Ah - missed this, but will address in final patch.

> 
> ::: mobile/android/base/tabqueue/TabQueueService.java
> @@ +98,3 @@
> >  
> > +        messageView = (TextView) toastLayout.findViewById(R.id.toast_message);
> > +        messageView.setText(tabQueuedMessage);
> 
> We set this in onStartCommand - this line is unnecessary.

Missed this also, will address in final patch.


> 
> I'm having difficulty following this - shall we take a step back?
> 
> Given the multiple states we can enter onStartCommand in, I think it'd make
> sense to do some branching. For example (some of this may be incorrect):
> 
> if (isFastDoubleTap) {
>   openLinkNow();
>   clearQueuedLinks();
> } else if (isSlowDoubleTap) {
>   displayAlreadyQueuedToast();
> } else {
>   queueLink();
>   displayQueuedToast();
> }
> 
> Seems much easier to read.

It does seem easier to read, but because of the flow of the code it's not quite as simple as this :(
I've tried to remove as much of the code from the onStartCommand function to make the flow a little easier to read.  I've also taken your suggestion about state and tried to make that a bit more transparent.
I've renames the StopServiceRunnable to just TabQueueRunnable as the StopService was confusing, given that this runnable has several responsibilities.  Actually thinking about it, it should maybe be called a TabQueueItemRunnable, as it's responsible for holding the info and performing bits of functionality for an individual item for the tab queue.

Anyway, a bit of feedback would be great.  If you still think it's confusing then perhaps I can think of a new way of doing it.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8604805 [details] [diff] [review]
Add toast notification when trying to add the same link to the tab queue

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

Sorry but I'm a bit too brain dead to finish this up right now - I'll either get it later tonight or first thing tomorrow morning.

::: mobile/android/base/tabqueue/TabQueueService.java
@@ +137,2 @@
>  
> +            if (isFastDoubleTap) {

nit: I think these double taps can be named better - is the DoubleTapTimeLimit a FastDoubleTapTimeLimit? I can't figure out if the slow double-tap also has a time limit or not.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8604805 [details] [diff] [review]
Add toast notification when trying to add the same link to the tab queue

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

As discussed over IRC, Martyn will look into further revising this patch.

Comment 12

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

NI-ing here to sync up on this tomorrow - unclear what ended up happening here :)

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

This is more a feature/improvement than a bug. That's why I'm setting it to block v2.

Comment 14

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.