Closed Bug 899393 Opened 7 years ago Closed 6 years ago

[fig] Switch-to-tab is broken

Categories

(Firefox for Android :: General, defect, P1)

ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: Margaret, Assigned: Margaret)

References

(Depends on 1 open bug)

Details

(Whiteboard: fixed-fig)

Attachments

(1 file, 4 obsolete files)

Tapping an item with "Switch to tab" just opens the URL in the current tab, it doesn't switch to it.
Priority: -- → P1
Assignee: nobody → margaret.leibovic
Attached patch simple approach (obsolete) — Splinter Review
I initially tried porting our existing implementation over to fig, creating an onSwitchToTab method on OnUrlOpenListener, but that quickly turned into a big mess because we have a lot of instances of these listeners floating around, and we call onUrlOpen() all over the place. We'd either need to add switch-to-tab logic in all these places (pretty gross) or create a more central method for opening a URL, similar to the current AwesomeBarTab.sendToListener. I was hopeful we could just make a method like that on HomeFragment, but there are multiple smaller views that don't know about HomeFragment, which hold onto a reference to the listener themselves.

Long story short, this patch just makes it so that we always switch to an open tab if it's there when trying to load a URL. I think we're fine with this behavior, but it does mean that non-switch-to-tab UI things (like the thumbnails) will also have switch-to-tab behavior.

I decided to just modify Tabs.hasUrl to return a tab id, instead of making two similar methods that just return different values.
Attachment #783430 - Flags: review?(wjohnston)
Comment on attachment 783430 [details] [diff] [review]
simple approach

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

Drive-by.

::: mobile/android/base/Tabs.java
@@ +590,2 @@
>       */
> +    public int getIdForUrl(String url) {

getTabIdForUrl() for clarity?
Flags: needinfo?(ibarlow)
Comment on attachment 783430 [details] [diff] [review]
simple approach

I decided this is a bad solution, I don't think it's acceptable to switch to a tab when tapping a thumbnail, because the UI doesn't show that that's what we're going to do.
Attachment #783430 - Flags: review?(wjohnston) → review-
Oops, flipped the needinfo? flag without actually finishing a comment. Here are the issues here:

This launch point can actually be a tab now (as opposed to the awesomescreen), so what should happen when you choose a switch-to-tab item? Do we keep that about:home tab open? On IRC we talked about only showing the switch-to-tab UI in editing mode, but you can still get into a state where you enter editing mode on an about:home tab. Do we close it then? We probably wouldn't want to do that if that tab has history.

Basically, two main questions:
1) When do we show "Switch to tab" on items?
2) What do we do after the user taps on that?
Another bug with the patch here: if you're in editing mode and choose a switch-to-tab option for the currently open tab, we don't dismiss editing mode.
Attached patch more robust patch (obsolete) — Splinter Review
This doesn't deal with closing new about:home tabs if you choose a switch-to-tab option, but at least switch-to-tab actually works with this.

This approach is more verbose than my last one, but I like that it's more explicit about when we're switching to a tab vs opening a URL.

Some of the logic in onSwitchToTab is redundant with logic in openUrl(), but I wanted a solution that didn't involve passing more parameter to openUrl. I'm open to suggestions, though.
Attachment #783430 - Attachment is obsolete: true
Attachment #783983 - Flags: feedback?(wjohnston)
Attachment #783983 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 783983 [details] [diff] [review]
more robust patch

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

Hmm, the code duplication everywhere is a bit unfortunate for obvious reasons. What about adding a 'Flags' enum to OnUrlOpenListener and add an extra argument to onUrlOpen()? This way you'd be able to handle the switchToTab case in one spot (BrowserApp). If would be something like:

public interface OnUrlOpenListener {
    public enum Flags {
        ALLOW_SWITCH_TO_TAB
    }

    public void onUrlOpen(String url, EnumSet<Flags> flags);
}

Then you'd call onUrlOpen(url, EnumSet.of(OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB) in all spots except on the top sites grid, in which case you'd do onUrlOpen(url, EnumSet.noneOf(OnUrlOpenListener.Flags.class)). Yep, Java is that verbose :-) But at least this code is type-safe.

Alternatively, you could add a allowSwitchToTab boolean to onUrlOpen(). The drawback would be that boolean arguments are kinda sucky in terms of readability and they are not very future-proof. But, to be honest, I'd prefer even a boolean argument over replicating the tab switch code everywhere.
Attachment #783983 - Flags: feedback?(lucasr.at.mozilla) → feedback-
(In reply to Lucas Rocha (:lucasr) from comment #7)
> Comment on attachment 783983 [details] [diff] [review]
> more robust patch
> 
> Review of attachment 783983 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm, the code duplication everywhere is a bit unfortunate for obvious
> reasons. What about adding a 'Flags' enum to OnUrlOpenListener and add an
> extra argument to onUrlOpen()? This way you'd be able to handle the
> switchToTab case in one spot (BrowserApp). If would be something like:
> 
> public interface OnUrlOpenListener {
>     public enum Flags {
>         ALLOW_SWITCH_TO_TAB
>     }
> 
>     public void onUrlOpen(String url, EnumSet<Flags> flags);
> }
> 
> Then you'd call onUrlOpen(url,
> EnumSet.of(OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB) in all spots except
> on the top sites grid, in which case you'd do onUrlOpen(url,
> EnumSet.noneOf(OnUrlOpenListener.Flags.class)). Yep, Java is that verbose
> :-) But at least this code is type-safe.
> 
> Alternatively, you could add a allowSwitchToTab boolean to onUrlOpen(). The
> drawback would be that boolean arguments are kinda sucky in terms of
> readability and they are not very future-proof. But, to be honest, I'd
> prefer even a boolean argument over replicating the tab switch code
> everywhere.

Thanks for the feedback. I agree the code duplication sucks, so I like this flag suggestion. I'm contemplating updating BrowserApp.openUrl to replace the newTab boolean parameter with a flag, so that I could reuse that method and avoid duplicating the code that updates the browser toolbar.
Attached patch WIP v2 (obsolete) — Splinter Review
This patch uses lucasr's idea for OnUrlOpenListener flags. I like that this lets us avoid messing around with Tabs in all the various views.

It also fixes some incorrect logic around trying to switch to the currently selected tab.

I still want to think about this a bit more before asking for review, and maybe find a way to deal with closing a new tab if you immediately choose switch-to-tab from that. But maybe that's out of scope for this bug.
Attachment #783983 - Attachment is obsolete: true
Attachment #783983 - Flags: feedback?(wjohnston)
Attachment #784529 - Flags: feedback?(wjohnston)
Attachment #784529 - Flags: feedback?(lucasr.at.mozilla)
Attached patch WIP v2 (obsolete) — Splinter Review
Oops, I totally uploaded the wrong patch.
Attachment #784529 - Attachment is obsolete: true
Attachment #784529 - Flags: feedback?(wjohnston)
Attachment #784529 - Flags: feedback?(lucasr.at.mozilla)
Attachment #784654 - Flags: feedback?(wjohnston)
Attachment #784654 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 784654 [details] [diff] [review]
WIP v2

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

Looks pretty good with the suggested cleanups. I'd handle the close-if-switch-to-tab case on new tabs in a separate bug.

::: mobile/android/base/BrowserApp.java
@@ +2052,5 @@
> +        if (flags.contains(HomePager.OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB)) {
> +            maybeSwitchToTab(url);
> +        } else {
> +            openUrl(url);
> +        }

This would probably look a bit cleaner if you did something like:

if (!maybeSwitchToTab(url, flags)) {
    openUrl(url);
}

Then maybeSwitchToTab would look like:

private boolean maybeSwitchToTab(String url, EnumSet<Flags> flags) {
    if (!flags.contains(HomePager.OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB)) {
        return false;
    }

    ...

    if (tabId < 0) {
        return false;
    }

    ...

    return true;
}

::: mobile/android/base/home/HomePager.java
@@ +41,5 @@
>  
>      public interface OnUrlOpenListener {
> +        public enum Flags {
> +            ALLOW_SWITCH_TO_TAB
> +        }

nit: add empty line here.
Attachment #784654 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Attached patch patchSplinter Review
This patch addresses Lucas's comments. I'll let Wes do the final review :)
Attachment #784654 - Attachment is obsolete: true
Attachment #784654 - Flags: feedback?(wjohnston)
Attachment #785186 - Flags: review?(wjohnston)
(In reply to :Margaret Leibovic from comment #1)
> Created attachment 783430 [details] [diff] [review]
> simple approach
> 
> I initially tried porting our existing implementation over to fig, creating
> an onSwitchToTab method on OnUrlOpenListener, but that quickly turned into a
> big mess because we have a lot of instances of these listeners floating
> around, and we call onUrlOpen() all over the place. We'd either need to add
> switch-to-tab logic in all these places (pretty gross) or create a more
> central method for opening a URL, similar to the current
> AwesomeBarTab.sendToListener. I was hopeful we could just make a method like
> that on HomeFragment, but there are multiple smaller views that don't know
> about HomeFragment, which hold onto a reference to the listener themselves.
> 
> Long story short, this patch just makes it so that we always switch to an
> open tab if it's there when trying to load a URL. I think we're fine with
> this behavior, but it does mean that non-switch-to-tab UI things (like the
> thumbnails) will also have switch-to-tab behavior.
>

That sounds like a reasonable approach. We might want to look at adding a little tab icon under thumbnails that are already open in tabs.
Flags: needinfo?(ibarlow)
(In reply to Ian Barlow (:ibarlow) from comment #13)
> (In reply to :Margaret Leibovic from comment #1)
> > Created attachment 783430 [details] [diff] [review]
> > simple approach
> > 
> > I initially tried porting our existing implementation over to fig, creating
> > an onSwitchToTab method on OnUrlOpenListener, but that quickly turned into a
> > big mess because we have a lot of instances of these listeners floating
> > around, and we call onUrlOpen() all over the place. We'd either need to add
> > switch-to-tab logic in all these places (pretty gross) or create a more
> > central method for opening a URL, similar to the current
> > AwesomeBarTab.sendToListener. I was hopeful we could just make a method like
> > that on HomeFragment, but there are multiple smaller views that don't know
> > about HomeFragment, which hold onto a reference to the listener themselves.
> > 
> > Long story short, this patch just makes it so that we always switch to an
> > open tab if it's there when trying to load a URL. I think we're fine with
> > this behavior, but it does mean that non-switch-to-tab UI things (like the
> > thumbnails) will also have switch-to-tab behavior.
> >
> 
> That sounds like a reasonable approach. We might want to look at adding a
> little tab icon under thumbnails that are already open in tabs.

Sorry I might not have made the updated situation clear in the comments... the most recent version of my patch only opens things in new tabs if they have the "Switch to tab" label. I think this is better because it won't surprise users.
Ok, cool.
Comment on attachment 785186 [details] [diff] [review]
patch

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

::: mobile/android/base/BrowserApp.java
@@ +1229,5 @@
> +     *
> +     * @return true if we successfully switched to a tab, false otherwise.
> +     */
> +    private boolean maybeSwitchToTab(String url, EnumSet<HomePager.OnUrlOpenListener.Flags> flags) {
> +        if (!flags.contains(HomePager.OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB)) {

I don't really like naming this FLAGS. Also, having this namespaced so deeply is ugly. :(

@@ +2053,5 @@
>      }
>  
>      // HomePager.OnUrlOpenListener
>      @Override
> +    public void onUrlOpen(String url, EnumSet<HomePager.OnUrlOpenListener.Flags> flags) {

If we wanted to be nice, it seems like it would be good to allow null for flags.
Attachment #785186 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #16)
> Comment on attachment 785186 [details] [diff] [review]
> patch
> 
> Review of attachment 785186 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +1229,5 @@
> > +     *
> > +     * @return true if we successfully switched to a tab, false otherwise.
> > +     */
> > +    private boolean maybeSwitchToTab(String url, EnumSet<HomePager.OnUrlOpenListener.Flags> flags) {
> > +        if (!flags.contains(HomePager.OnUrlOpenListener.Flags.ALLOW_SWITCH_TO_TAB)) {
> 
> I don't really like naming this FLAGS. Also, having this namespaced so
> deeply is ugly. :(

Yeah, you can import OnUrlOpenListener and use OnUrlOpenListener.Flags instead.
 
> @@ +2053,5 @@
> >      }
> >  
> >      // HomePager.OnUrlOpenListener
> >      @Override
> > +    public void onUrlOpen(String url, EnumSet<HomePager.OnUrlOpenListener.Flags> flags) {
> 
> If we wanted to be nice, it seems like it would be good to allow null for
> flags.

The drawback of allowing null is readability. A null argument in a method call looks a little less obvious. Embrace Java's verbosity! :-)
Depends on: 904217
https://hg.mozilla.org/mozilla-central/rev/2b52431a8054
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.