1154425 - Tapping the same link twice when Tab Queue is enabled should Open Now

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Overlays, defect)

Description

[Richard Newman [:rnewman]]

I didn't see the message at the bottom of the screen, forgot I had Tab Queue on, and ended up loading the same page four times.

Firstly, we shouldn't queue the same URL more than once!

Secondly, I think if we get the same link twice in a row, we should assume that the user wanted to open it right away, and do so.

Agree, Anthony?

Comment 1

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

(In reply to Richard Newman [:rnewman] from comment #0)
> I didn't see the message at the bottom of the screen, forgot I had Tab Queue
> on, and ended up loading the same page four times.
> 
> Firstly, we shouldn't queue the same URL more than once!

Agree!

> Secondly, I think if we get the same link twice in a row, we should assume
> that the user wanted to open it right away, and do so.
> 
> Agree, Anthony?

Almost agree. I think the Toast should come up, but say something different (i.e. "Tab already queued in Firefox"). And since, there is the [ ] Open ability on these "super toasts", users can just open. But, we should not add another notification since we shouldn't queue the same URL more than once.

Comment 2

[Richard Newman [:rnewman]]

That wouldn't help in the situation I described -- I tapped the URL multiple times because my hand was covering the teensy tiny toast, and because I wanted (and expected) it to open when I tapped. If you're covering the toast, you can't tell that you didn't just miss the link and needing to tap again.

Changing the text doesn't help if the user doesn't see the toast, and I think I'm expressing my intent pretty firmly by tapping the link more than once, no?

Am I making sense?

Comment 3

[Martyn Haigh (:mhaigh)]

Attachment: Part 1 - Move imports

A bit of cleanup - the imports were in the wrong place which bugged me.

Comment 4

[Martyn Haigh (:mhaigh)]

Attachment: Part 2 - Prep Service changes

Some slight prep to the service which will assist the main logic of this feature. We extract the openNow functionality to a separate function and allow the runnable to be executed without actually running the embedded code - this helps us as it means we can call stopSelfResult on the startId held within the runnable.

Comment 5

[Martyn Haigh (:mhaigh)]

Attachment: Part 3 - Add remove url method

Add a remove url method to the helper class - this scans through the stored urls and removes any instances of the passed in url.

Comment 6

[Martyn Haigh (:mhaigh)]

Attachment: Part 4 - Implement core logic change for double tap functionality

Here we implement the core logic of this feature.  We check if we pass the double tap requirements, both in terms of url content and time restraints and if so we go on to open the url immediately.  Because we allow a double tap to occur within X seconds of the previous tap, there's a chance the previously added url has already been processed, if so we need to remove the url from the list before we open the urls.

Comment 7

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

Comment on attachment 8596101 [details] [diff] [review]
Part 1 - Move imports

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

::: mobile/android/base/tabqueue/TabQueueService.java
@@ +8,5 @@
> +import org.mozilla.gecko.BrowserApp;
> +import org.mozilla.gecko.GeckoProfile;
> +import org.mozilla.gecko.GeckoSharedPrefs;
> +import org.mozilla.gecko.R;
> +import org.mozilla.gecko.mozglue.ContextUtils;

My intellij only gets alt-enter correct when the package name is alphabetized - i.e. org.mozilla is below java, which is below android.

Mind moving it below (or telling me how to configure intellij better)? :P

Comment 8

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

Comment on attachment 8596108 [details] [diff] [review]
Part 2 - Prep Service changes

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

::: mobile/android/base/tabqueue/TabQueueService.java
@@ +208,5 @@
>          public void run(final boolean shouldStopService) {
> +            run(shouldStopService, true);
> +        }
> +
> +        public void run(final boolean shouldStopService, final boolean shouldExecuteRunnable) {

For the record, I generally prefer larger patches that show why this change is necessary, rather than clicking through multiple patches trying to figure out where it is used.

i.e. I would have put this change in the 4th patch, or if you retroactivily divided these changes and it's hard to separate, just did the 2nd & 4th patches together.

Comment 9

[Martyn Haigh (:mhaigh)]

> My intellij only gets alt-enter correct when the package name is
> alphabetized - i.e. org.mozilla is below java, which is below android.
> 
> Mind moving it below (or telling me how to configure intellij better)? :P

As discussed in IntelliJ open (Settings|Preferences)>Editor>Code Style>Java and select the imports tab.  At the bottom of that panel, in the import layout, add the line org.mozilla.* and add a blank line underneath that.

Comment 10

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

Comment on attachment 8596109 [details] [diff] [review]
Part 3 - Add remove url method

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

::: mobile/android/base/tabqueue/TabQueueHelper.java
@@ +114,5 @@
> +     * @param urlToRemove URL to remove
> +     * @param filename    filename to remove URL from
> +     * @return the number of queued urls
> +     */
> +    public static int removeUrl(final Context context, final String urlToRemove, final String filename) {

nit: -> removeURLFromFile

@@ +120,5 @@
> +
> +        final GeckoProfile profile = GeckoProfile.get(context);
> +
> +        JSONArray jsonArray = profile.readJSONArrayFromFile(filename);
> +        JSONArray newArray = new JSONArray();

It'd be more efficient to iterate through the array and call remove on the urls to remove rather than construct an entirely new JSONArray object.

Followup? Mentorable? Or just do it now. :)

@@ +126,5 @@
> +        for (int i = 0; i < jsonArray.length(); i++) {
> +            try {
> +                url = jsonArray.getString(i);
> +            } catch (JSONException e) {
> +                url = "";

Log?

@@ +128,5 @@
> +                url = jsonArray.getString(i);
> +            } catch (JSONException e) {
> +                url = "";
> +            }
> +            if(!TextUtils.isEmpty(url) && !urlToRemove.equals(url)) {

Can't guarantee urlToRemove is non-null but we know url is b/c of TextUtils - I'd switch the object and argument.

Comment 11

[Martyn Haigh (:mhaigh)]

> @@ +120,5 @@
> > +
> > +        final GeckoProfile profile = GeckoProfile.get(context);
> > +
> > +        JSONArray jsonArray = profile.readJSONArrayFromFile(filename);
> > +        JSONArray newArray = new JSONArray();
> 
> It'd be more efficient to iterate through the array and call remove on the
> urls to remove rather than construct an entirely new JSONArray object.
> 
> Followup? Mentorable? Or just do it now. :)

I agree this is a bit janky but JSONArray.remove was only added in API level 19 :(

Comment 12

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

Comment on attachment 8596111 [details] [diff] [review]
Part 4 - Implement core logic change for double tap functionality

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

On the right track but I think we never remove URLs (see specific comments). Also, I think it can be a bit clearer in parts. If you think I'm wrong, say why and queue me another r?

::: mobile/android/base/tabqueue/TabQueueService.java
@@ +58,4 @@
>      private static final String LOGTAG = "Gecko" + TabQueueService.class.getSimpleName();
>  
>      private static final long TOAST_TIMEOUT = 3000;
> +    private static final long TOAST_DOUBLE_TAP_TIMEOUT = 6000;

nit: I like to include units in my constant names, so -> TOAST_DOUBLE_TAP_TIMEOUT_MILLIS

@@ +111,4 @@
>  
>      @Override
>      public int onStartCommand(final Intent intent, final int flags, final int startId) {
> +        // If this is a redelivery then lets bypass the entire double tap to open now code as that's a big can of worms.

Maybe mention we don't expect redeliveries to happen because we do this quickly?

@@ +128,5 @@
> +                // Background thread because we could do some file IO if we have to remove a url from the list.
> +                tabQueueHandler.post(new Runnable() {
> +                    @Override
> +                    public void run() {
> +                        if (stopServiceRunnable != null) {

This if condition could use more explanation - if the Runnable is null, what does it mean for the state?

I realize it reveals itself in the comments inside the branches, but it requires sleuthing - I think you could be clearer.

@@ +131,5 @@
> +                    public void run() {
> +                        if (stopServiceRunnable != null) {
> +                            // The only way we can tell the service not to redeliver the last intent again is to call
> +                            // stopSelfResult but the only pointer to the startId is kept in the runnable.
> +                            stopServiceRunnable.run(true, false);

This seems awkward because we're basically removing all functionality from the runnable in order to call stopSelf - why not make startId more accessible? Either store it elsewhere or use `final protected` in the runnable.

Also, shouldn't stopServiceRunnable be nulled if we ever want to reach the bottom branch?

@@ +141,5 @@
> +                        openNow(safeIntent.getUnsafe(), startId);
> +                    }
> +                });
> +
> +                return START_REDELIVER_INTENT;

Should we null the SharedPrefs values? Then again, maybe we shouldn't care about the very few users I'd expect to double tap, hit back, and then tap again.

Comment 13

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

(In reply to Martyn Haigh (:mhaigh) from comment #11)
> I agree this is a bit janky but JSONArray.remove was only added in API level
> 19 :(

lolwut.

Add a comment. :)

Comment 14

[Martyn Haigh (:mhaigh)]

Attachment: Tapping the same link twice when Tab Queue is enabled should Open Now

Revisited the core logic, implemented a getStartId in the runnable and removed the janky "don't run" logic.

Also renamed the bool used in the runnable from shouldStopService to shouldRemoveView as that's what it was doing.

Other nits addressed

Comment 15

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

Comment on attachment 8596268 [details] [diff] [review]
Tapping the same link twice when Tab Queue is  enabled should Open Now

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

네~!

Comment 16

[Martyn Haigh (:mhaigh)]

https://hg.mozilla.org/integration/fx-team/rev/1baa62b2cc57

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1baa62b2cc57

Comment 18

[Teodora Vermesan (:TeoVermesan)]

Scenario 1:
Tapping a link external to Firefox will generate in the android notification bar "Nightly - 1 tab waiting" and tapping once again on the same link will open it in Firefox in a new tab.
But tapping the "Nightly - 1 tab waiting" notification from the android notification bar does nothing. Is it expected? 

Scenario 2:
Tapping a link external to Firefox will generate in the android notification bar "Nightly - 1 tab waiting" and tapping once again on the same link will open it in Firefox in a new tab.
Close Nightly and tap the notification bar.
=> About:home is displayed and the link is not loaded in a new tab
=> The "Nightly - 1 tab waiting" notification is still displayed in the android notification bar
=> Tapping it, does nothing
Is this also expected?

I also filled Bug 1159296 suggesting that it would be helpful to inform users with a notification when the same tab is already queued. If you don't consider it a priority or a necessity, please feel free to close it.

Comment 19

[Martyn Haigh (:mhaigh)]

I've filed bug 1159718 to track removing the notification when quick open is activated.