942862 - Factor out display UI into a separate View

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Lucas Rocha (:lucasr)]

Factor out the part of the toolbar that deals with:
- Throbber
- Lock icon
- Title
- Page actions
- Stop

Comment 1

[Lucas Rocha (:lucasr)]

Attachment: Remove unused member of BrowserToolbar (r=wesj)

Comment 2

[Lucas Rocha (:lucasr)]

Attachment: Factor out URL display UI into ToolbarDisplayLayout (r=wesj)

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8339996 [details] [diff] [review]
Factor out URL display UI into ToolbarDisplayLayout (r=wesj)

Splits the toolbar's 'URL display' UI into a separate view. This is just a 'mechanical' change which means I haven't made any functional changes to the code.

This refactoring makes it very clear that we need to redefine how we manage the state of the toolbar. The internal API is pretty vague and under-defined. This patch is just an initial effort to untangle the code a bit (together with bug 871522 and bug 938205) in preparation for more fundamental changes in upcoming patches. 

So, bear with me while I get things ready for the actual changes :-)

Comment 4

[Lucas Rocha (:lucasr)]

FYI: The new internal API to manage the state of the toolbar is being worked on in bug 944533.

Comment 5

[Lucas Rocha (:lucasr)]

Review ping?

Comment 6

[Wesley Johnston (:wesj)]

Comment on attachment 8339996 [details] [diff] [review]
Factor out URL display UI into ToolbarDisplayLayout (r=wesj)

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

Couple ideas. If they're off or done somewhere else, happy to hear and r+.

::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +472,5 @@
>                      if (showProgress && tab.getState() == Tab.STATE_LOADING) {
>                          setProgressVisibility(true);
>                      }
>                      setSecurityMode(tab.getSecurityMode());
> +                    setPageActionVisibility(mUrlDisplayLayout.isShowingProgress());

This seems a bit strange to still have in here (same with setProgressVisibility()?) Can we just make mUrlDisplayLayout a tab listener?

@@ +618,5 @@
>                                      mActivity.getString(R.string.one_tab));
>      }
>  
>      public void setProgressVisibility(boolean visible) {
> +        mUrlDisplayLayout.setProgressVisibility(visible);

I think we should kill this from in here. i.e. I'm not totally sure I understand how this is being driven though. Maybe doing to much is just renaming BrowserToolbar...

@@ +623,4 @@
>      }
>  
>      public void setPageActionVisibility(boolean isLoading) {
> +        mUrlDisplayLayout.setPageActionVisibility(isLoading, mSwitchingTabs);

Same with this...

@@ +1116,5 @@
>                      mUrlBarRightEdge.setVisibility(View.INVISIBLE);
>                  }
>  
>                  PropertyAnimator buttonsAnimator = new PropertyAnimator(300);
> +                mUrlDisplayLayout.prepareStopEditingAnimation(buttonsAnimator);

Seems like mUrlDisplayLayout should just attach its own Animation listener?

@@ +1195,5 @@
>  
>                      layoutParams = (ViewGroup.MarginLayoutParams) mUrlEditLayout.getLayoutParams();
>                      layoutParams.leftMargin = mUrlBarViewOffset;
>  
> +                    mUrlDisplayLayout.finishForwardAnimation();

This should probably be called regardless of the animation direction. i.e. this guy shouldn't be so aware of what is animating in the urlbar. But if we add the listener in the urlbar itself, it can smartly not add the listener it in the !enabled case.

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
@@ +51,5 @@
> +    private ImageButton mSiteSecurity;
> +    private PageActionLayout mPageActionLayout;
> +    private Animation mProgressSpinner;
> +
> +    private boolean mSpinnerVisible;

This seems like an odd name. Can we use mProgressVisible?

@@ +331,5 @@
> +            // the animation starts, so we shift these items to the right so that they don't
> +            // appear to move initially.
> +            ViewHelper.setTranslationX(mTitle, width);
> +            ViewHelper.setTranslationX(mFavicon, width);
> +            ViewHelper.setTranslationX(mSiteSecurity, width);

Separate bug, but these repeated calls could probably be wrapped in something prettier. i.e.

prepareTranslate(PropertyAnimator anim, Integer start, int end, View views...) {
  for (View v : views) {
    anim.attach(v, PropertyAnimation.Property.TRANSLATION_X, end);
    if (start != null)
      ViewHelper.setTranslationX(v, start);
  }
}

Comment 7

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #6)
> Comment on attachment 8339996 [details] [diff] [review]
> Factor out URL display UI into ToolbarDisplayLayout (r=wesj)
> 
> Review of attachment 8339996 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Couple ideas. If they're off or done somewhere else, happy to hear and r+.
> 
> ::: mobile/android/base/toolbar/BrowserToolbar.java
> @@ +472,5 @@
> >                      if (showProgress && tab.getState() == Tab.STATE_LOADING) {
> >                          setProgressVisibility(true);
> >                      }
> >                      setSecurityMode(tab.getSecurityMode());
> > +                    setPageActionVisibility(mUrlDisplayLayout.isShowingProgress());
> 
> This seems a bit strange to still have in here (same with
> setProgressVisibility()?) Can we just make mUrlDisplayLayout a tab listener?

The biggest design goal in bug 944533 is to make ToolbarDisplayLayout as dummy as possible with a single entry-point to update its state. From a wider "toolbar subsystem" perspective, the idea is that BrowserToolbar is the backbone where we listen to events and update the state of each component accordingly. This is all to make the toolbar code easier to follow by centralizing certain responsibilities to fewer components.  

> @@ +618,5 @@
> >                                      mActivity.getString(R.string.one_tab));
> >      }
> >  
> >      public void setProgressVisibility(boolean visible) {
> > +        mUrlDisplayLayout.setProgressVisibility(visible);
> 
> I think we should kill this from in here. i.e. I'm not totally sure I
> understand how this is being driven though. Maybe doing to much is just
> renaming BrowserToolbar...

This is gone with the patches in bug 944533.

> @@ +623,4 @@
> >      }
> >  
> >      public void setPageActionVisibility(boolean isLoading) {
> > +        mUrlDisplayLayout.setPageActionVisibility(isLoading, mSwitchingTabs);
> 
> Same with this...

Ditto.

> @@ +1116,5 @@
> >                      mUrlBarRightEdge.setVisibility(View.INVISIBLE);
> >                  }
> >  
> >                  PropertyAnimator buttonsAnimator = new PropertyAnimator(300);
> > +                mUrlDisplayLayout.prepareStopEditingAnimation(buttonsAnimator);
> 
> Seems like mUrlDisplayLayout should just attach its own Animation listener?

Good point. The intention here is to follow a consistent pattern of prepare/finish calls everywhere. I do intent (in the mid-term?) to turn all this into a little transition framework for us. 

> @@ +1195,5 @@
> >  
> >                      layoutParams = (ViewGroup.MarginLayoutParams) mUrlEditLayout.getLayoutParams();
> >                      layoutParams.leftMargin = mUrlBarViewOffset;
> >  
> > +                    mUrlDisplayLayout.finishForwardAnimation();
> 
> This should probably be called regardless of the animation direction. i.e.
> this guy shouldn't be so aware of what is animating in the urlbar. But if we
> add the listener in the urlbar itself, it can smartly not add the listener
> it in the !enabled case.

Nice catch. Calling finishForwardAnimation() unconditionally now.

> ::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
> @@ +51,5 @@
> > +    private ImageButton mSiteSecurity;
> > +    private PageActionLayout mPageActionLayout;
> > +    private Animation mProgressSpinner;
> > +
> > +    private boolean mSpinnerVisible;
> 
> This seems like an odd name. Can we use mProgressVisible?

This is gone in bug 944533.

> @@ +331,5 @@
> > +            // the animation starts, so we shift these items to the right so that they don't
> > +            // appear to move initially.
> > +            ViewHelper.setTranslationX(mTitle, width);
> > +            ViewHelper.setTranslationX(mFavicon, width);
> > +            ViewHelper.setTranslationX(mSiteSecurity, width);
> 
> Separate bug, but these repeated calls could probably be wrapped in
> something prettier. i.e.
> 
> prepareTranslate(PropertyAnimator anim, Integer start, int end, View
> views...) {
>   for (View v : views) {
>     anim.attach(v, PropertyAnimation.Property.TRANSLATION_X, end);
>     if (start != null)
>       ViewHelper.setTranslationX(v, start);
>   }
> }

Good point. Filed bug 957090 to track this.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8339996 [details] [diff] [review]
Factor out URL display UI into ToolbarDisplayLayout (r=wesj)

Requesting review again after clarifications.

Comment 9

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/779f90afa8d6
https://hg.mozilla.org/integration/fx-team/rev/486a6b3af95b

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

I think you added mobile/android/base/widget/GeckoActionProvider.java.orig by mistake.

Comment 11

[Lucas Rocha (:lucasr)]

(In reply to Tim Taubert [:ttaubert] from comment #10)
> I think you added mobile/android/base/widget/GeckoActionProvider.java.orig
> by mistake.

Oops. Nice catch, thanks. I'll do an extra push to remove it.

Comment 12

[Carsten Book [:Tomcat]]

hi,

had to backout this change as part of https://tbpl.mozilla.org/?tree=Fx-Team&rev=e89b407be9c5 because of build bustages on android and windows like https://tbpl.mozilla.org/php/getParsedLog.php?id=32753197&tree=Fx-Team

Comment 13

[Lucas Rocha (:lucasr)]

These patches require a clobber build. Touched CLOBBER file and re-landed:

https://hg.mozilla.org/integration/fx-team/rev/7463f220cbcc
https://hg.mozilla.org/integration/fx-team/rev/afbc1bece02d

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7463f220cbcc
https://hg.mozilla.org/mozilla-central/rev/afbc1bece02d