Lucas Rocha (:lucasr) Reporter
	Description • 10 years ago

Because the tabs panel will use a grid in the new tablet UI and a list in the old tablet and phone UIs. Create an interface that defines the constract between tabspanel and the view.

	Lucas Rocha (:lucasr) Reporter
	Comment 3 • 10 years ago

Comment on attachment 8475973 [details] [diff] [review]
Part 1 - rename TabsTray to TabsTrayList in prep for interface

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

Looks good but didn't we agree to name this TabsListLayout instead? Where the interface would be called TabsLayout.

	Lucas Rocha (:lucasr) Reporter
	Comment 4 • 10 years ago

Comment on attachment 8475983 [details] [diff] [review]
part 2 create and implement interface

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

Looking good. Just make sure we're using the terminology we've agreed on.

::: mobile/android/base/tabs/TabsTray.java
@@ +7,5 @@
> +
> +import android.view.View;
> +import org.mozilla.gecko.tabs.TabsPanel;
> +
> +interface TabsTray extends TabsPanel.PanelView{

I'd prefer having the TabsTray concrete implementations to implement both TabsTray and TabsPanel.PanelView interface instead (the current TabsTrayList already implements CloseAllPanelView anyway). To avoid coupling the two interfaces unnecessarily. Also, didn't we agree on naming this interface as TabsLayout?

	Martyn Haigh (:mhaigh) Assignee
	Comment 6 • 10 years ago

Terminology updated and added dependency on TabsPanel back to the TabsListLayout class

	Lucas Rocha (:lucasr) Reporter
	Comment 7 • 10 years ago

Comment on attachment 8476759 [details] [diff] [review]
0001-bug-1046209-part-2-create-and-implement-interface.patch

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

Looks good with the suggested changes.

::: mobile/android/base/tabs/TabsLayout.java
@@ +9,5 @@
> +import org.mozilla.gecko.tabs.TabsPanel;
> +
> +interface TabsLayout extends TabsPanel.PanelView {
> +    public void setEmptyView(View view);
> +    public void closeAll();

Hmm, isn't closeAll() from CloseAllPanelView? Remove it and make TabsLayout extend from TabsPanel.CloseAllPanelView instead.

::: mobile/android/base/tabs/TabsListLayout.java
@@ +40,5 @@
>  
>  class TabsListLayout extends TwoWayView
> +                     implements TabsLayout,
> +                                TabsPanel.PanelView,
> +                                TabsPanel.CloseAllPanelView {

TabsLayout is a PanelView & CloseAllPanelView. You only need a "implements TabsLayout" here.

	Martyn Haigh (:mhaigh) Assignee
	Comment 8 • 10 years ago

Have reworked the interface as noted.

	Martyn Haigh (:mhaigh) Assignee
	Comment 9 • 10 years ago

Fixed missing space

	Lucas Rocha (:lucasr) Reporter
	Comment 10 • 10 years ago

Comment on attachment 8479012 [details] [diff] [review]
Part 2 - create and implement interface

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

Looks good with the suggested change.

::: mobile/android/base/tabs/TabsLayout.java
@@ +7,5 @@
> +
> +import android.view.View;
> +import org.mozilla.gecko.tabs.TabsPanel;
> +
> +interface TabsLayout extends TabsPanel.CloseAllPanelView {

Hmm... given how simple this interface turned out to be and the fact that is directly coupled with CloseAllPanelView now, maybe we should just move its definition to where PanelView and CloseAllPanelView are defined instead i.e. move it to TabsPanel. Sorry for the churn!

	Martyn Haigh (:mhaigh) Assignee
	Comment 11 • 10 years ago

Reworked patch to simplified - this one just renames TabsTray to TabsListLayout

	Lucas Rocha (:lucasr) Reporter
	Comment 12 • 10 years ago

Comment on attachment 8479843 [details] [diff] [review]
Part 1 - rename TabsTray to TabsListLayout

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

Looks good but I think you actually meant to rename it to TabsListLayout? :-)

	Martyn Haigh (:mhaigh) Assignee
	Comment 13 • 10 years ago

silly mistake fixed :-/

	Martyn Haigh (:mhaigh) Assignee
	Comment 15 • 10 years ago

Named interface incorrectly - this fixes

	Martyn Haigh (:mhaigh) Assignee
	Comment 16 • 10 years ago

Missed refactoring of a variable name.

Having one of those days.  Sorry for the churn.

	Martyn Haigh (:mhaigh) Assignee
	Comment 17 • 10 years ago

can this be checked in pls?  This needs to be checked in prior to 1056169. cheers

	Lucas Rocha (:lucasr) Reporter
	Comment 18 • 10 years ago

https://hg.mozilla.org/projects/larch/rev/31cbf57e972a
https://hg.mozilla.org/projects/larch/rev/ce1c303ee66a

	Carsten Book [:Tomcat]
	Comment 19 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/31cbf57e972a
https://hg.mozilla.org/mozilla-central/rev/ce1c303ee66a