Closed Bug 1026715 Opened 10 years ago Closed 10 years ago

Add "Open all" item to recent tabs lists

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 33

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file, 1 obsolete file)

Splitting this off from bug 1004850.
I really want to fix bug 732752, but in the meantime, this just uses the same onNewTabs logic, although I modified it to use a list rather than an array, so that I could dynamically generate the list of urls to open.

This openAllTabs logic feels kinda hacky, so I'm open to suggestions about how to make it better, but these lists shouldn't be too big, so I don't know how much it matters to try to be efficient here.
Attachment #8441716 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 8441716 [details] [diff] [review]
WIP - Add "Open all" item to recent tabs lists

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

Looks good overall, just needs to improve the way you figure out the type of tabs to be opened.

::: mobile/android/base/home/RecentTabsPanel.java
@@ +128,5 @@
>                      return;
>                  }
>  
> +                final int type = c.getInt(c.getColumnIndexOrThrow(RecentTabs.TYPE));
> +                if (type == RecentTabs.TYPE_OPEN_ALL) {

Maybe have TYPE_OPEN_ALL_CLOSED/TYPE_OPEN_ALL_LAST_TIME row types so that you can tell them apart here and use TYPE_LAST_TIME or TYPE_CLOSED in openTabsWithType?

@@ +244,5 @@
>              }
>          });
>      }
>  
> +    private void openAllTabs(int position) {

Wouldn't it simpler to simply take a 'type' int as argument and open all tabs for the given type?

@@ +249,2 @@
>          final Cursor c = mAdapter.getCursor();
> +        if (c == null || !c.moveToPosition(position - 1)) {

What's this 'position - 1' about? I'd expect this method to just position directly.

@@ +251,4 @@
>              return;
>          }
>  
> +        final int type = c.getInt(c.getColumnIndexOrThrow(RecentTabs.TYPE));

This is a bit fragile. The type should probable be the argument of openAllTabs. Maybe rename it to openTabsWithType?

@@ +254,5 @@
> +        final int type = c.getInt(c.getColumnIndexOrThrow(RecentTabs.TYPE));
> +        Log.i(LOGTAG, "************ openAllTabs: type: " + type);
> +
> +        final ArrayList<String> urls = new ArrayList<String>();
> +        c.moveToFirst();

What do you move to 'position - 1' then call moveToFirst() here?
Attachment #8441716 - Flags: feedback?(lucasr.at.mozilla) → feedback+
(In reply to Lucas Rocha (:lucasr) from comment #2)

> @@ +254,5 @@
> > +        final int type = c.getInt(c.getColumnIndexOrThrow(RecentTabs.TYPE));
> > +        Log.i(LOGTAG, "************ openAllTabs: type: " + type);
> > +
> > +        final ArrayList<String> urls = new ArrayList<String>();
> > +        c.moveToFirst();
> 
> What do you move to 'position - 1' then call moveToFirst() here?

This is a hack to figure out the type that we want to open (looking at the type of the row above the open all button). Pretty bad, I know.

I'll work with the suggestion to have explicit OPEN_ALL_CLOSED and OPEN_ALL_LAST_TIME row types, and then we won't need this.
Also flagging bnicholson in case he wants to review this before lucasr is back from PTO :)
Attachment #8441716 - Attachment is obsolete: true
Attachment #8442416 - Flags: review?(lucasr.at.mozilla)
Attachment #8442416 - Flags: review?(bnicholson)
FYI: This depends on the patches in bug 1004850, but that should be ready to land soon, I'm just verifying that it's green on try.
Comment on attachment 8442416 [details] [diff] [review]
Add "Open all" item to recent tabs lists

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

Looks good. I assume UX folks have given their thumbs up on the design here?

::: mobile/android/base/home/RecentTabsPanel.java
@@ +144,3 @@
>                  Telemetry.sendUIEvent(TelemetryContract.Event.LOAD_URL);
>  
> +                final ArrayList<String> urls = new ArrayList<String>();

final List<String>...

@@ +308,5 @@
>                      }
>                  }
> +
> +                // Add an "Open all" button if more than 2 tabs were added to the list.
> +                if (length > 1) {

> 2, no?
Attachment #8442416 - Flags: review?(lucasr.at.mozilla)
Attachment #8442416 - Flags: review?(bnicholson)
Attachment #8442416 - Flags: review+
(In reply to Lucas Rocha (:lucasr) from comment #6)
> Comment on attachment 8442416 [details] [diff] [review]
> Add "Open all" item to recent tabs lists
> 
> Review of attachment 8442416 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. I assume UX folks have given their thumbs up on the design here?

This was the design ibarlow initially proposed in bug 1004850, but I'll check with him today to see if there are any polish tweaks we need to make.

> @@ +308,5 @@
> >                      }
> >                  }
> > +
> > +                // Add an "Open all" button if more than 2 tabs were added to the list.
> > +                if (length > 1) {
> 
> > 2, no?

I should update the comment because that's confusing. It should be "if 2 or more tabs were added". So as long as there are more than 2 ( > 1) tabs, we'll show the button.
https://hg.mozilla.org/mozilla-central/rev/d825d00cceee
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
If I open one tab and close it, the "Open all" option isn't displayed but if I open two tabs and close them, the option is present and restores them, so verified fixed on:
Device: Samsung Galaxy Nexus (Android 4.2)
Build: Firefox for Android 33.0a1 (2014-06-26)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: