Closed Bug 942862 Opened 6 years ago Closed 6 years ago

Factor out display UI into a separate View

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: lucasr, Assigned: lucasr)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Factor out the part of the toolbar that deals with:
- Throbber
- Lock icon
- Title
- Page actions
- Stop
Attachment #8339995 - Flags: review?(wjohnston)
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 :-)
Attachment #8339996 - Flags: review?(wjohnston)
Status: NEW → ASSIGNED
FYI: The new internal API to manage the state of the toolbar is being worked on in bug 944533.
Blocks: 944533
Depends on: 944537
Attachment #8339995 - Flags: review?(wjohnston) → review+
Review ping?
Flags: needinfo?(wjohnston)
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);
  }
}
Attachment #8339996 - Flags: review?(wjohnston) → review-
Flags: needinfo?(wjohnston)
Blocks: 957090
(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 on attachment 8339996 [details] [diff] [review]
Factor out URL display UI into ToolbarDisplayLayout (r=wesj)

Requesting review again after clarifications.
Attachment #8339996 - Flags: review- → review?(wjohnston)
Attachment #8339996 - Flags: review?(wjohnston) → review+
I think you added mobile/android/base/widget/GeckoActionProvider.java.orig by mistake.
(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.
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
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
https://hg.mozilla.org/mozilla-central/rev/7463f220cbcc
https://hg.mozilla.org/mozilla-central/rev/afbc1bece02d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.