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

RESOLVED INACTIVE

Status

()

Firefox for Android
Overlays
RESOLVED INACTIVE
3 years ago
3 days ago

People

(Reporter: TeoVermesan, Unassigned)

Tracking

40 Branch
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

3 years ago
Also tapping the "Nightly - 1 tab waiting" notification from the android notification bar does nothing
(Reporter)

Updated

3 years ago
Blocks: 1112185
(Reporter)

Comment 2

3 years ago
Also I saw in Bug 1154425 that Anthony suggested a notification: for example: "Tab already queued in Firefox" to inform users.
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
Flags: needinfo?(teodora.vermesan)

Updated

3 years ago
Assignee: nobody → mhaigh
(Reporter)

Comment 4

3 years ago
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.
Flags: needinfo?(teodora.vermesan)
Status: NEW → ASSIGNED
Component: General → Overlays
Hardware: ARM → All
Summary: Add toast notification when trying to add the same link to the tabqueue → Add toast notification when trying to add the same link to the tab queue
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
Created attachment 8602836 [details] [diff] [review]
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.
Attachment #8602836 - Flags: review?(michael.l.comella)
(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 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 (`
Attachment #8602836 - Flags: review?(michael.l.comella) → feedback+
Created attachment 8604805 [details] [diff] [review]
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.
Attachment #8602836 - Attachment is obsolete: true
Attachment #8604805 - Flags: feedback?(michael.l.comella)
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 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.
Attachment #8604805 - Flags: feedback?(michael.l.comella)

Updated

3 years ago
Assignee: mhaigh → nobody
Status: ASSIGNED → NEW
NI-ing here to sync up on this tomorrow - unclear what ended up happening here :)
Flags: needinfo?(mhaigh)
This is more a feature/improvement than a bug. That's why I'm setting it to block v2.
Blocks: 1158260
No longer blocks: 1112185
status-firefox40: affected → ---
Flags: needinfo?(martyn.haigh+bugzilla)

Comment 14

3 days ago
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Last Resolved: 3 days ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.