Closed Bug 1046203 Opened 5 years ago Closed 5 years ago

Change BrowserToolbar to use alternative layout when isNewTablet()

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 35

People

(Reporter: lucasr, Assigned: mcomella)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [fixed-larch])

Attachments

(7 files, 15 obsolete files)

13.42 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
4.65 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
43.28 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
15.68 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
43.38 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
17.01 KB, patch
mcomella
: review+
Details | Diff | Splinter Review
7.68 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
I don't expect we'll need any major change in the code itself (at least initially). We might end up needing more fundamental changes in the code *if* the behavior of the toolbar in the new tablet UI changes a bit more drastically.
Passing the ball to mcomella. We have discussed a few ways to approach this.
Assignee: nobody → michael.l.comella
Depends on: 1048903
Depends on: 1048907
Comment on attachment 8467801 [details] [diff] [review]
Use different layout in toolbar when new tablet UI is enabled (r=mcomella)

Here's the hack I have to apply a different toolbar layout when the new tablet UI is enabled. This should give you an idea about some of the spots where we need to adapt the UI. Forward button is kinda broken.
Forgot to mention one important aspect of this patch: I'm prefixing all resources related to the new tablet UI with NewTablet/new_tablet. This will make it a lot easier to see where we're changing the UI for the new tablet UX (i.e. dxr-friendly) and easier to override the old tablet UI with the new resources once we're ready to ship.
It seems you are missing your changes to attrs.xml:

/home/mcomella/dev/trees/larch/mobile/android/base/resources/layout-large-v11/new_tablet_browser_toolbar.xml:75: error: No resource identifier found for attribute 'layout' in package 'org.mozilla.fennec_mcomella'

I'll fix it locally and carry on.
Status: NEW → ASSIGNED
Ah, I see - patches are in the blocking bugs.
I decided that the proper way to do this would be to have an abstract
BrowserToolbar base class, with a subclass for each device type. However, to do
this, we need to stub our views so BrowserToolbarStub can be declared in the
xml. I figured this wasn't worth the time, so I opted to take the middle ground
and call device-specific code via a BrowserToolbarDevice interface from the
BrowserToolbar code.

Note that the BrowserToolbarDevice interface may not be final - it may change
as I do the other device types.
Attachment #8474026 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 8474026 [details] [diff] [review]
Part 1: Abstract BrowserToolbar phone layout

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

Looks like a pretty good start. I like the fact that you started small here.

Some questions for further consideration:
- How could we define this interface in such a way that can avoid null checks for elements that don't exist in all UI flavours? For example, back/forward buttons and actionBarButtons are only used on tablets, translating edge is only needed on phones, etc.
- We're most likely replacing the dynamic tab counter with a simpler tabs button in the new tablet UI. How can we define this interface in a way that we can vary this?

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +171,5 @@
>      private static final Interpolator buttonsInterpolator = new AccelerateInterpolator();
>  
>      private static final int FORWARD_ANIMATION_DURATION = 450;
>  
> +    private final BrowserToolbarDevice device;

nit: 'Device' doesn't seem to match what you're doing here. BrowserToolbarUI? BrowserToolbarImpl?
Attachment #8474026 - Flags: feedback?(lucasr.at.mozilla) → feedback+
From your initial patch (thus not sure if you should review this).
Attachment #8474831 - Flags: review?(lucasr.at.mozilla)
(In reply to Lucas Rocha (:lucasr) from comment #8)
> - How could we define this interface in such a way that can avoid null
> checks for elements that don't exist in all UI flavours? For example,
> back/forward buttons and actionBarButtons are only used on tablets,
> translating edge is only needed on phones, etc.

I'm making each phone/tablet implementation inherit from a shared base class
that takes care of this initialization. However, because we need to do this
after Inflation, I moved inflation into the BrowserToolbarUI implementors,
which is a little gross because the inflation is hidden, but works better than
"postInflationInitialize()" and "getLayoutID()" methods in the interface.

> - We're most likely replacing the dynamic tab counter with a simpler tabs
> button in the new tablet UI. How can we define this interface in a way that
> we can vary this?

As in we need to be able to update dynamically on tablet, but not on phone?
I think I will just utilize BrowserToolbarUI, but I will do this after I
complete the tablet patch so I can better understand the whole picture and
avoid doing too much at once.

> nit: 'Device' doesn't seem to match what you're doing here.
> BrowserToolbarUI? BrowserToolbarImpl?

I went with BrowserToolbarUI since all of the split code should be exclusively
UI related.
Attachment #8474839 - Flags: review?(lucasr.at.mozilla)
Attachment #8474026 - Attachment is obsolete: true
Attachment #8467801 - Attachment is obsolete: true
Did some renaming.
Attachment #8474875 - Flags: review?(lucasr.at.mozilla)
Attachment #8474839 - Attachment is obsolete: true
Attachment #8474839 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8474831 [details] [diff] [review]
Part 0: Add basic new_tablet BrowserToolbar resources

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

Looks good without the shadow view.

::: mobile/android/base/resources/layout-large-v11/new_tablet_browser_toolbar.xml
@@ +35,5 @@
> +                                          android:layout_centerVertical="true"
> +                                          android:padding="13dp"
> +                                          android:src="@drawable/ic_menu_back"
> +                                          android:contentDescription="@string/back"
> +                                          android:background="@drawable/url_bar_nav_button"/>

With the exception of the back/forward buttons, I think the new toolbar layout will be a lot more linear with no overlapping children. It we could change the new tablet toolbar to be a simpler LinearLayout instead. No need to do this in this patch though. Please file a follow-up to investigate.

@@ +102,5 @@
> +               android:layout_width="match_parent"
> +               android:layout_height="2dp"
> +               android:layout_alignParentBottom="true"
> +               android:background="@color/url_bar_shadow"
> +               android:contentDescription="@null"/>

I've just merged m-c into larch. This view can go away.

@@ +112,5 @@
> +            style="@style/UrlBar.ImageButton.Icon"
> +            android:layout_alignParentRight="true"
> +            android:src="@drawable/close_edit_mode_selector"
> +            android:paddingLeft="2dp"
> +            android:paddingRight="2dp"

Not specific to this patch but I wonder if this padding is actually necessary. Please file a follow-up to remove it from all *browser_toolbar.xml if it's not.
Attachment #8474831 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8474875 [details] [diff] [review]
Part 1: Abstract BrowserToolbar phone layout

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

To be honest, I'm slightly mixed about this approach after seeing how the BrowserToolbarUI implementations interact with BrowserToolbar because the responsibilities are kinda of vague i.e. BrowserToolbar delegates only "part" of the UI implementation to BrowserToolbarUI which is a bit weird. And the view getters look a bit leaky for something that is supposed to just be composed together.

Just as an exercise, let's see at how this would look if we took the subclassing path:

1. BrowserToolbar would be abstract and have abstract methods that match your current BrowserToolbarUI (startEditing, stopEditing, getFocusOrder).

2. BrowserToolbar would have protected constructor like BrowserToolbar(Context context, int layoutId). Subclasses would just call super(context, R.layout.THEIR_LAYOUT_ID) in their public *BrowserToolbar(Context context) constructor. This protected constructor will go away once the new tablet UI actually replaces the current one in Nightly i.e. we'll always use R.layout.browser_toolbar.

3. The child views references would become protected members instead of private.

4. The BrowserToolbar subclasses would look almost the same as your current BrowserToolbarUI implementations. You'd have a BrowserToolbarPhoneBase and BrowserToolbarTabletBase inheriting from BrowserToolbar. And then the concrete implementation subclasses.

5. You'd create a simple stub that would look like:

// Not tested at all btw :-)
public class BrowserToolbarStub extends View {
    public BrowserToolbatStub(Context context, AttributeSet attrs, int defStyle) {
        super(context, attrs, defStyle);
    }

    @Override
    protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
        // Always empty.
        setMeasuredDimension(0, 0);
    }

    @Override
    public void draw(Canvas canvas) {
        // Draw nothing.
    }

    // maybe init() or create() or ...
    public BrowserToolbar addAndReturn() {
        final BrowserToolbar toolbar;
        if (NewTabletUI.isEnabled()) {
            toolbar = new NewTabletBrowserToolbar(getContext());
        } else if (HardwardUtils.isTablet()) {
            toolbar = new TabletBrowserToolbar(getContext());
        } else if (Build.VERSION.SDK_INT < 11) {
            toolbar = new GBPhoneBrowserToolbar(getContext());
        } else {
            toolbar = new PhoneBrowserToolbar(getContext());
        }

        final ViewGroup parent = (ViewGroup) getParent();

        final int index = parent.indexOfChild(this);
        parent.removeViewInLayout(this);

        final LayoutParams lp = getLayoutParams();
        if (lp != null) {
            parent.addView(toolbar, index, lp);
        } else {
            parent.addView(toolbar, index);
        }

        toolbar.setClickable(true);
        toolbar.setFocusable(true);
        toolbar.setBackgroundDrawable(getBackground());

        return toolbar;
    }
}

Or can simply use stock ViewStub and setLayoutResource() dynamically following the same logic. This would involve creating a separate XML file for each toolbar flavour which is kinda annoying but I'd be fine with that too. Then use the stub in gecko_app.xml and call addAndReturn() is BrowserApp's constructor.

Subclassing feels slightly saner to me here because it becomes clear that each subclass is a variation of the same type of UI component.

Thoughts?

ps: In terms of patch structure, I'd first implement the variations between tablet and phones. Then make the new tablet UI changes on top, in a separate patch. Not a big deal though.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +142,5 @@
>      private ThemedImageButton menuButton;
>      private ThemedImageView menuIcon;
>      private LinearLayout actionItemBar;
>      private MenuPopup menuPopup;
> +    private final List<View> focusOrder;

No change?

@@ +188,5 @@
> +        if (HardwareUtils.isTablet()) {
> +            final int layoutID = (isNewTablet) ?
> +                    R.layout.new_tablet_browser_toolbar : R.layout.browser_toolbar;
> +            LayoutInflater.from(context).inflate(layoutID, this);
> +            toolbarUI = null;

If you plan to work on the BrowserToolbarUI implementation for tablets in a follow-up, there's no need to include  BrowserToolbarTabletBase in this patch.

@@ +649,5 @@
>              }
>          }
>      }
>  
> +    protected void updateTabCountAndAnimate(final int count) {

The way java treats scoping for inner protected interfaces is so *not* intuitive :-)

@@ +835,5 @@
>      public void setOnStopEditingListener(OnStopEditingListener listener) {
>          stopEditingListener = listener;
>      }
>  
> +    protected void showUrlEditLayout() {

Same question about protected vs package access.

::: mobile/android/base/toolbar/BrowserToolbarTabletBase.java
@@ +1,1 @@
> +package org.mozilla.gecko.toolbar;

No need to include this in this patch as you're not actually implementing tablet UI just yet.

::: mobile/android/base/toolbar/BrowserToolbarUIPhone.java
@@ +55,5 @@
> +    }
> +
> +    @Override
> +    public void startEditing(final PropertyAnimator animator) {
> +        if (toolbar.isAnimatingEntry()) {

Is there any value in leaving the notion of isAnimatingEntry() in BrowserToolbar? It seems very phone-specific now.
Attachment #8474875 - Flags: review?(lucasr.at.mozilla) → feedback+
Actually, a cleaner approach would be to dynamically resolve the class name from the layout file using a LayoutInflater.Factory instead of using ViewStubs.

BrowserApp is a FragmentActivity which is already a LayoutInflater.Factory itself. You can just override this method and resolve org.mozilla.gecko.toolbar.BrowserToolbar with something like:

View onCreateView(String name, Context context, AttributeSet attrs) {
   final View view;
   if (name.equals(BrowserToolbar.class.getName())) {
       view = BrowserToolbar.create(context, attrs);
   } else {
       view = super.onCreateView(name, context, attrs);
   }

   return v;
}
(In reply to Lucas Rocha (:lucasr) from comment #13)
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +142,5 @@
> >      private ThemedImageButton menuButton;
> >      private ThemedImageView menuIcon;
> >      private LinearLayout actionItemBar;
> >      private MenuPopup menuPopup;
> > +    private final List<View> focusOrder;
> 
> No change?

What do you mean here?

> @@ +649,5 @@
> > +    protected void updateTabCountAndAnimate(final int count) {
> 
> The way java treats scoping for inner protected interfaces is so *not*
> intuitive :-)
> 
> @@ +835,5 @@
> > +    protected void showUrlEditLayout() {
> 
> Same question about protected vs package access.

I don't understand what you mean here - what is the issue?
Comment on attachment 8477099 [details] [diff] [review]
Part 1: Implement LayoutInflater.Factory in BrowserApp to inflate device specific BrowserToolbars

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

Nice. Double-check if we actually need to explicitly set factory.

::: mobile/android/base/BrowserApp.java
@@ +470,5 @@
>      }
>  
>      @Override
>      public void onCreate(Bundle savedInstanceState) {
> +        LayoutInflater.from(getContext()).setFactory(this); // Must be called before super.onCreate.

Are you sure we need this? FragmentActivity does that for us. See:
https://github.com/android/platform_frameworks_support/blob/master/v4/java/android/support/v4/app/FragmentActivity.java#L201
Attachment #8477099 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Michael Comella (:mcomella) from comment #15)
> (In reply to Lucas Rocha (:lucasr) from comment #13)
> > ::: mobile/android/base/toolbar/BrowserToolbar.java
> > @@ +142,5 @@
> > >      private ThemedImageButton menuButton;
> > >      private ThemedImageView menuIcon;
> > >      private LinearLayout actionItemBar;
> > >      private MenuPopup menuPopup;
> > > +    private final List<View> focusOrder;
> > 
> > No change?
> 
> What do you mean here?

Splinter fail apparently. It displayed this as if you were removing and adding same code.

> > @@ +649,5 @@
> > > +    protected void updateTabCountAndAnimate(final int count) {
> > 
> > The way java treats scoping for inner protected interfaces is so *not*
> > intuitive :-)
> > 
> > @@ +835,5 @@
> > > +    protected void showUrlEditLayout() {
> > 
> > Same question about protected vs package access.
> 
> I don't understand what you mean here - what is the issue?

Ignore it. I got confused with the scopes of the interfaces and other classes.
Comment 17 is correct - we don't need to set it explicitly. Updated this patch accordingly.
Attachment #8477099 - Attachment is obsolete: true
I could have split this up better so that I created the boilerplate files in one patch, then made the phone implementation in another, but it was too much work.
Attachment #8477701 - Flags: review?(lucasr.at.mozilla)
Addressed Lucas' comments on the removal of "isAnimatingEntry" from the base class by adding an abstract isAnimating method.
Attachment #8477713 - Flags: review?(lucasr.at.mozilla)
Attachment #8477701 - Attachment is obsolete: true
Attachment #8477701 - Flags: review?(lucasr.at.mozilla)
Attachment #8474875 - Attachment is obsolete: true
Attachment #8474831 - Attachment is obsolete: true
Attachment #8477625 - Attachment is obsolete: true
Attachment #8477713 - Attachment is obsolete: true
Attachment #8477713 - Flags: review?(lucasr.at.mozilla)
Moved the new phone-specific roundCorner* code into the phone classes.
Attachment #8477778 - Flags: review?(lucasr.at.mozilla)
Attachment #8477763 - Attachment is obsolete: true
Attachment #8477763 - Flags: review?(lucasr.at.mozilla)
Sorry for the churn - added licenses to the tops of all of the new files and class comments for tablets.
Attachment #8478498 - Flags: review?(lucasr.at.mozilla)
Attachment #8477778 - Attachment is obsolete: true
Attachment #8477778 - Flags: review?(lucasr.at.mozilla)
Attachment #8478480 - Attachment is obsolete: true
Attachment #8478480 - Flags: review?(lucasr.at.mozilla)
I changed a bit of functionality in actionItemBar, which is why I left it out
of part 3. The things I have so concern over:
  * Using the view ID to update the focus order on actionItemBar
  * Changing the behavior of addActionItem on phones, where it now returns
    false.
Attachment #8478512 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8478498 [details] [diff] [review]
Part 2: Add boilerplate BrowserToolbar subclasses and phone implementation

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

Looks like a pretty good start.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +171,5 @@
>  
> +    public abstract boolean isAnimating();
> +
> +    protected abstract void transitionForStartEditing(PropertyAnimator animator);
> +    protected abstract void transitionForStopEditing();

nit: These method names look a bit weird. Maybe triggerStartEditingTransition/triggerStopEditingTransition?

@@ +198,5 @@
>          // BrowserToolbar is attached to BrowserApp only.
>          activity = (BrowserApp) context;
>  
>          // Inflate the content.
> +        // TODO: Remove the branch when new tablet becomes old tablet.

Please add this to the "Enabling the new UI in Nightly" section in the tablet-refresh etherpad.

@@ +202,5 @@
> +        // TODO: Remove the branch when new tablet becomes old tablet.
> +        if (!isNewTablet) {
> +            LayoutInflater.from(context).inflate(R.layout.browser_toolbar, this);
> +        } else {
> +            LayoutInflater.from(context).inflate(R.layout.new_tablet_browser_toolbar, this);

It would be slightly nicer if the constructor took this as an argument instead. Not a big deal though. File a follow-up if you think this is worth doing.
Attachment #8478498 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8478502 [details] [diff] [review]
Part 3: Create BrowserToolbar tablet and new_tablet implementations

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

Awesome.

::: mobile/android/base/toolbar/BrowserToolbarTabletBase.java
@@ +40,5 @@
> +        backButton = (BackButton) findViewById(R.id.back);
> +        setButtonEnabled(backButton, false);
> +        forwardButton = (ForwardButton) findViewById(R.id.forward);
> +        setButtonEnabled(forwardButton, false);
> +        initButtonListeners();

Initialize these in onAttachedToWindow() for consistency with the BrowserToolbar parent class.

@@ +47,5 @@
> +        focusOrder.addAll(urlDisplayLayout.getFocusOrder());
> +        focusOrder.addAll(Arrays.asList(actionItemBar, menuButton));
> +    }
> +
> +    private void initButtonListeners() {

This should become your onAttachedToWindow().
Attachment #8478502 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8478512 [details] [diff] [review]
Part 4: Remove unused views and move actionItemBar to TabletBase

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

Less useless views, win.
Attachment #8478512 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8478585 [details] [diff] [review]
Part 5: Remove final isTablet invocations in BrowserToolbar

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

We'll probably have different implementations of transitionForTabsTray in the new tablet IO but this is fine for now.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +149,5 @@
>      protected abstract void updateNavigationButtons(Tab tab);
>  
>      protected abstract void transitionForStartEditing(PropertyAnimator animator);
>      protected abstract void transitionForStopEditing();
> +    public abstract void transitionForTabsTray(PropertyAnimator animator, boolean areTabsShown);

triggerTabsPanelTransition instead?
Attachment #8478585 - Flags: review?(lucasr.at.mozilla) → review+
I believe the try run in comment 31 has failures because the forward button is not currently implemented - I'll do a basic implementation in part 6.
(In reply to Lucas Rocha (:lucasr) from comment #32)
> It would be slightly nicer if the constructor took this as an argument
> instead. Not a big deal though. File a follow-up if you think this is worth
> doing.

Since we're only holding onto this until the new tablet is the default, I don't think it's a big deal.
(In reply to Lucas Rocha (:lucasr) from comment #33)
> ::: mobile/android/base/toolbar/BrowserToolbarTabletBase.java
> @@ +40,5 @@
> > +        backButton = (BackButton) findViewById(R.id.back);
> > +        setButtonEnabled(backButton, false);
> > +        forwardButton = (ForwardButton) findViewById(R.id.forward);
> > +        setButtonEnabled(forwardButton, false);
> > +        initButtonListeners();
> 
> Initialize these in onAttachedToWindow() for consistency with the
> BrowserToolbar parent class.

Why? What is the benefit? I can see this being useful for two things:
  1) Lazy initialization, though I'm skeptical about the benefits
  2) To free memory by nulling our listeners in onDetachedFromWindow. However, we don't do this so we end up doing redundant work and forcing GC every time this View is attached to the window.
Attachment #8478498 - Attachment is obsolete: true
Attachment #8478585 - Attachment is obsolete: true
Comment on attachment 8479325 [details] [diff] [review]
Part 6: Copy-pasta rough BrowserToolbarNewTablet forward button behavior

Looking at the failures in comment 42 locally, the forward button may be overlapped by the back button, so the back button gets hit instead - I'll update my patch shortly.
Attachment #8479325 - Flags: review?(lucasr.at.mozilla)
Copy-pasta with a hint of increasing the animation with from width * .5 to width * .75. try run in comment 44.
Attachment #8479325 - Attachment is obsolete: true
Comment on attachment 8480168 [details] [diff] [review]
Part 6: Copy-pasta rough BrowserToolbarNewTablet forward button behavior

Rubber stamp to help get tests passing. As I understand it, this will be refined in bug 1058322.
Attachment #8480168 - Flags: review+
For question in comment 38. I'm going to land, so we can file a followup if this is an issue.
Flags: needinfo?(lucasr.at.mozilla)
Whiteboard: [fixed-larch]
(In reply to Michael Comella (:mcomella) from comment #47)
> For question in comment 38. I'm going to land, so we can file a followup if
> this is an issue.

Not a big issue, it's just to keep some consistency across the toolbar classes.
Flags: needinfo?(lucasr.at.mozilla)
You need to log in before you can comment on or make changes to this bug.