757198 - Refactor TabsTray to work with new tablet UI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

This lays the groundwork for making an initial tabs UI for tablets.

Comment 1

[:Margaret Leibovic]

Attachment: Part 1 - Turn TabsTray into a PopupWindow

This patch just sorts out some of the basic plumbing converting TabsTray into a PopupWindow.

Comment 2

[:Margaret Leibovic]

Attachment: Part 2 - Make sure TabsAdapter properly observes tab changes

Now that we're not destroying/recreating an activity to show the tabs UI, we need to make sure we're updating it properly when tabs change.

I'm not sure how costly having a permanent observer is, so alternately, we could only observe changes while the popup is open (like we currently do), then just refresh the tabs when we open it (previously we weren't observing tab added events properly).

This patch also makes TabsAdapter into the OnTabsChangedListener instead of TabsTray, since TabsAdapter is really the one that wants to know about things changing, since it keeps track of the stuff that needs updating.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 625754 [details] [diff] [review]
Part 1 - Turn TabsTray into a PopupWindow

Looks OK to me as a first pass.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 625754 [details] [diff] [review]
Part 1 - Turn TabsTray into a PopupWindow

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

This looks good to me.
Few things:
- show() takes a parent as argument. I don't find a need for it there. If it's for the anchor to be used for showAsDropDown(), I would say, it's better to use it in instantiation of mTabsTray.
(And please do test it with rotation).

- In case of phones, the anchor isn't need. It should be positioned on top (0,0).

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 625754 [details] [diff] [review]
Part 1 - Turn TabsTray into a PopupWindow

Do we need to add some mTabsTray.hide() calls ?

Comment 6

[:Margaret Leibovic]

(In reply to Sriram Ramasubramanian [:sriram] from comment #4)

> - show() takes a parent as argument. I don't find a need for it there. If
> it's for the anchor to be used for showAsDropDown(), I would say, it's
> better to use it in instantiation of mTabsTray.
> (And please do test it with rotation).

Whoops, that got left in there from when I was trying to play around with different ways to show the popup.

> - In case of phones, the anchor isn't need. It should be positioned on top
> (0,0).

Yeah, I was trying to useShowAtLocation [1] with mMainLayout as the parent, but it wasn't showing up. I need to play around with that more.

[1] http://developer.android.com/reference/android/widget/PopupWindow.html#showAtLocation%28android.view.View,%20int,%20int,%20int%29

What I was going to say before I got mid-aired (three times):

Things still left to deal with (potentially could go in separate bugs):
-RemoteTabs is still an Activity, so we should probably make that into a PopupWindow as well
-Proper styling of these popup windows on phone/tablet (I haven't put any effort into that yet)

Comment 7

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #5)

> Do we need to add some mTabsTray.hide() calls ?

Oh, yeah, we'll likely need those (increasingly, I feel like we some sort of "popups, hide yourselves!" event). We get built-in tap outside hiding from the fact that it's a PopupWindow, but we probably also want to hide it if the user hits the back button, or if the app moves to the background.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 625757 [details] [diff] [review]
Part 2 - Make sure TabsAdapter properly observes tab changes

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

Making TabsAdapter listen for tab changes look good to me.

The refreshTabs() function seems to be tricky. It monitors for close and selected. How about other tab related messages?


The following line should be done in show() of TabsTray.
Tabs.registerOnTabsChangedListener(mTabsAdapter);

Similarly,
Tabs.unregisterOnTabsChangedListener(mTabsAdapter);
should be done in hide() (which makes sense to move dismiss() here).
This would make sure, TabsTray doesn't listen for unnecessary tab events when not being displayed.

Lastly,
please clear the list on hide. Thereby, the views won't take up memory.
Probably clearing up data in mTabsAdapter (or making it null to do GC) would be better.

Comment 9

[Aaron Train [:aaronmt]]

Any potential performance hit with this conversion? (I love the TabsTray).

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 625757 [details] [diff] [review]
Part 2 - Make sure TabsAdapter properly observes tab changes

Nice. I like the cleanup in this patch.

We might be able to add some simple tests for TabsTray too. Just checking for adding, selecting and removing tabs. Nothing too fancy.

Comment 11

[:Margaret Leibovic]

(In reply to Aaron Train [:aaronmt] from comment #9)
> Any potential performance hit with this conversion? (I love the TabsTray).

No, this shouldn't have any performance hit. In fact, it may speed up performance, since we won't be recreating everything every time the tabs tray is shown (however, as Sriram suggested in comment 8, we will try to clear up some of the views to save on memory).

Comment 12

[:Margaret Leibovic]

Updating the summary to better reflect what this bug is doing.

I have new patches that turn TabsTray into a LinearLayout so that we can stick it in a PopupWindow on phones, and in a sidebar on tablets.

Comment 13

[:Margaret Leibovic]

Attachment: Part 1 - Turn TabsTray into LinearLayout

This still doesn't look very pretty, but it puts a foundation in place, and it doesn't regress phones (when combined with the second patch I'll upload).

Comment 14

[:Margaret Leibovic]

Attachment: Part 2 - Update TabsAdapter

This patch does a better job listening for specific tabs events and cleaning up after itself with the tabs tray is hidden. It was working well when I tested it.

Comment 15

[:Margaret Leibovic]

(In reply to Margaret Leibovic [:margaret] from comment #13)
> Created attachment 626908 [details] [diff] [review]
> Part 1 - Turn TabsTray into LinearLayout

I should note, I borrowed the isTablet() method from Sriram's patch in bug 730775. That will probably land first, and then I can get rid of it in here :)

Comment 16

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 626908 [details] [diff] [review]
Part 1 - Turn TabsTray into LinearLayout

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

I am not happy with changing TabsTray to a LinearLayout.
When added to GeckoApp, this is going to make the UI super heavy.

Here is a better approach:
TabsTray and RemoteTabs are inflated "into" a Popup (say a TabsPopup?).
Now that I've pushed tablet UI, we can specify 2 different layouts for the TabsPopup (or the width of the TabsTray, RemoteTabs) in two different xml.
Please look into layout/, layout-xlarge/ (and also layout-sw600dp/) for the same.

Now that we have a TabsPopup with "wrap_content" / "fill_parent" as width (based on tablet or phone -- one time initialization),
the content flows in easily.

The last step is to position the popup. In phones, it's going to start from (0,0). In tablets, it's going to use an anchor (GeckoApp.mBrowserToolbar).

The extra last step, in tablets, shift the mGeckoLayout when popup is shown.
And one more, ;) , cleanup the contents of lists when hiding. (I think you are doing it).

To me this is a better approach. We can look into swiping gesture for tablets later (having the popup position in -300dp (width) and enable clipping for popup, and slide it in based on gesture).

Comment 17

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 626910 [details] [diff] [review]
Part 2 - Update TabsAdapter

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

I like the cleanup in this patch.
However, I don't like the onListShow() and onListHide() methods.
They can be inside show() and hide() of listview.

TabsAdapter can have a method clear(), that can cleanup all the stuff (or, make mTabsAdapter null and leave it to GC :D).
Let the Adapter control just the views and the data. The registering with Tabs can always be with the list.
Reason: what if mTabsAdapter changes? Or you end up creating a new one? Only list knows it and it can gracefully unregister and register it.

Comment 18

[:Margaret Leibovic]

Passing over to Sriram, as we discussed over email.

(In reply to Sriram Ramasubramanian [:sriram] from comment #16)

> I am not happy with changing TabsTray to a LinearLayout.
> When added to GeckoApp, this is going to make the UI super heavy.

What are the practical repercussions of this? Will this make the UI slower? I thought sticking a view in the main layout vs in a popup wouldn't have a performance impact.

> Here is a better approach:
> TabsTray and RemoteTabs are inflated "into" a Popup (say a TabsPopup?).
> Now that I've pushed tablet UI, we can specify 2 different layouts for the
> TabsPopup (or the width of the TabsTray, RemoteTabs) in two different xml.
> Please look into layout/, layout-xlarge/ (and also layout-sw600dp/) for the
> same.
...
> The extra last step, in tablets, shift the mGeckoLayout when popup is shown.
> And one more, ;) , cleanup the contents of lists when hiding. (I think you
> are doing it).

If the plan is to shift the content over when the tabs tray opens in a sidebar, it seems like it would be a lot less error-prone to just let the android layout take care of it, rather than making a popup and manually re-sizing the content view.

> We can look into swiping gesture for
> tablets later (having the popup position in -300dp (width) and enable
> clipping for popup, and slide it in based on gesture).

Yeah, I filed bug 758317 for this.

Comment 19

[:Margaret Leibovic]

It looks like the first patch in here got taken care of by bug 739407.

Sriram, are you planning on taking over to land the second patch, or should I take this bug back to finish that up?

Comment 20

[:Margaret Leibovic]

Attachment: Update TabsAdapter

We don't need that first patch anymore, but here's an updated version of my TabsAdapter cleanup patch.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 640409 [details] [diff] [review]
Update TabsAdapter

Good cleanup. We could use some tests for Tabs and TabsTray. File a followup?

Comment 22

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #21)
> Comment on attachment 640409 [details] [diff] [review]
> Update TabsAdapter
> 
> Good cleanup. We could use some tests for Tabs and TabsTray. File a followup?

Filed bug 772520.

Comment 23

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/90e6b6c68e63

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/90e6b6c68e63