Closed Bug 1046209 Opened 5 years ago Closed 5 years ago

Abstract the type of views used in list of tabs

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: lucasr, Assigned: mhaigh)

References

Details

(Whiteboard: [fixed-larch])

Attachments

(2 files, 9 obsolete files)

12.60 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
4.23 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → mhaigh
Attachment #8475983 - Flags: review?(lucasr.at.mozilla)
Blocks: 1056169
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.
Attachment #8475973 - Flags: feedback+
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?
Attachment #8475983 - Flags: review?(lucasr.at.mozilla) → feedback+
Attachment #8475973 - Attachment is obsolete: true
Terminology updated and added dependency on TabsPanel back to the TabsListLayout class
Attachment #8475983 - Attachment is obsolete: true
Attachment #8476759 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #8476759 - Flags: review?(lucasr.at.mozilla) → review+
Have reworked the interface as noted.
Attachment #8476759 - Attachment is obsolete: true
Attachment #8478999 - Flags: review?(lucasr.at.mozilla)
Fixed missing space
Attachment #8478999 - Attachment is obsolete: true
Attachment #8478999 - Flags: review?(lucasr.at.mozilla)
Attachment #8479012 - Flags: review?(lucasr.at.mozilla)
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!
Attachment #8479012 - Flags: review?(lucasr.at.mozilla) → review+
Reworked patch to simplified - this one just renames TabsTray to TabsListLayout
Attachment #8476734 - Attachment is obsolete: true
Attachment #8479843 - Flags: feedback?(lucasr.at.mozilla)
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? :-)
Attachment #8479843 - Flags: feedback?(lucasr.at.mozilla) → feedback+
silly mistake fixed :-/
Attachment #8479843 - Attachment is obsolete: true
Attachment #8479012 - Attachment is obsolete: true
Named interface incorrectly - this fixes
Attachment #8479926 - Attachment is obsolete: true
Missed refactoring of a variable name.

Having one of those days.  Sorry for the churn.
Attachment #8479930 - Attachment is obsolete: true
can this be checked in pls?  This needs to be checked in prior to 1056169. cheers
Flags: needinfo?(lucasr.at.mozilla)
Attachment #8479922 - Flags: review+
Attachment #8479933 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/31cbf57e972a
https://hg.mozilla.org/mozilla-central/rev/ce1c303ee66a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.