889620 - [FIG] Transition for creating a new tab from the tab tray

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Ian Barlow (:ibarlow)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=885363#c2 for a detailed description. 

The transition is the second sequence in the files below:

Keynotehttp://cl.ly/2Y1t3y2w0W41
.MOV: http://cl.ly/0j402j2I0k2h
.GIF: http://cl.ly/image/1d3N102N201f

Comment 1

[Lucas Rocha (:lucasr)]

Putting this as a blocker (although we'll still have to discuss the content/scope of this bug)

Comment 2

[Lucas Rocha (:lucasr)]

We'll have to do some exploration here. The listview part of the animation might be too complex to do in v1. So, my initial impression is that we should probably simplify this transition a bit. For instance, we could first close the tabs tray then flip the about:home UI on top of current web content. Reusing the work that will be done in bug 889619.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

Sorry for the bit of a delay here. This needs to be split up, but does a pretty nice set of animation for us.

We can't show webpage content flipping backwards yet. kats tells me what we need TextureView for that, but turning it on showed a lot of bugs. We can flip about:home and the tabs though. The hard part here is the tabs tray bit, because we don't actually want to create the new tabs until the animation is complete (otherwise, you see a flash of white as the old tab disappears).

To do that this (for now) I made the TabsPanel and TabsTray both aware of whats going on. i.e. the tabs panel creates a lazily loaded tab when newTab is pressed and tells the tray to animate it when showing it. It then also has to tell BrowserApp to animate about:home into place (since we're not creating a real tab here, we need some way to tell BrowserApp to do something).

When the animation is over, TabsTray tells TabsPanel (who tells BrowserApp), to hide the tray, and selects the tab so that Gecko can start the real load.

That all feels a bit disjoint. Happily take feedback on a better solution. Build at:
http://people.mozilla.com/~wjohnston/fig.apk

Also, I broke the tab counter. I'm going to try and pull all those changes out into a separate set of patches anyway, since they're mostly unrelated.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch

qrefed

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch 1/5

Splitting this up. This bit just refactors Rotate3D a little bit. It does a couple things

1.) Reorders the translations and rotation steps in the transform. We now rotate and then translate. We have to do some sort of y-translation here to match the mockups. The results were really confusing with the old order (the position we were rotating around moved as we rotated).
2.) Exposes an x and y translation parameter by exposing some public members to the animation. We need y for this, but it seemed silly to not also add x.
3.) Kills off the reverse parameter which was redundent to the fade-in parameter.
4.) Simplifies some logic.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch 2/5

This adds a flip animation which just wraps a rotation and an alpha transition. We're doing these in multiple places now. It made sense to try and put them all together.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch 3/5

This animates showing the HomePager. I know you have a separate patch that does this a bit differently. This is a bit of a different path, and I don't think will interfere too much?

Comment 8

[Wesley Johnston (:wesj)]

Attachment: Patch 4/5

Animate new-tabs.

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch 5/5

This is the strangest part of this. It delays actually loading the new tab until the animation has finished (to avoid a flash of white in the tab area).

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 780831 [details] [diff] [review]
Patch 1/5

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

Looks good overall but I'm not keen on this public members. I'd prefer to have a setter or setters for them. Also, I think I need more background on the bit about inverting time.

::: mobile/android/base/animation/Rotate3DAnimation.java
@@ +37,5 @@
> +    private float mDeltaY = 0.0f;
> +
> +    public float deltaX = 0.0f;
> +    public float deltaY = 0.0f;
> +    public float deltaZ = 0.0f;

Hmm, why public? Add a setDelta(x, y, z)?

@@ +90,2 @@
>          camera.rotateX(degrees);
> +        float time = interpolatedTime;

Why creating a new variable? Any reason to leave interpolatedTime untouched?

@@ +90,5 @@
>          camera.rotateX(degrees);
> +        float time = interpolatedTime;
> +        // If we're moving to zero degrees, invert the time to move towards zero translation
> +        if (mToDegrees == 0)
> +            time = (1.0f - interpolatedTime);

What exactly is this accomplishing? Not clear from the comment. Is this about your point 1 (i.e. translating after rotation)?

Comment 11

[Wesley Johnston (:wesj)]

(In reply to Lucas Rocha (:lucasr) from comment #10)
> Comment on attachment 780831 [details] [diff] [review]
> Patch 1/5
> 
> Review of attachment 780831 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good overall but I'm not keen on this public members. I'd prefer to
> have a setter or setters for them. Also, I think I need more background on
> the bit about inverting time.

The getters and setters bit was basically some stuff ckitching ran into at:

http://developer.android.com/training/articles/perf-tips.html#GettersSetters

Namely, they can be slow (and I hate them). But this is intentionally not in performance critical paths, so its probably not worth worrying about. I'll change it.

> What exactly is this accomplishing? Not clear from the comment. Is this
> about your point 1 (i.e. translating after rotation)?

No. For the counter animation, I added a shift in the z-direction as the counter moves (although I need to double check that animation now, since I reversed the orders of these transforms and changed things). If we were bringing the counter in (to zero degrees rotation), I'd transform from zdelta to 0. If I was sending the counter away, I'd move from 0 to zdelta. This just controls that direction. Instead of letting people pass that in, I thought I'd just remove a (confusing) parameter, and auto-calc it for you.

Alternatively, we could let you pass in startZ and endZ. Or we could add a new translate3D animation, and add it to the AnimationSet.

Comment 12

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #11)
> (In reply to Lucas Rocha (:lucasr) from comment #10)
> > Comment on attachment 780831 [details] [diff] [review]
> > Patch 1/5
> > 
> > Review of attachment 780831 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good overall but I'm not keen on this public members. I'd prefer to
> > have a setter or setters for them. Also, I think I need more background on
> > the bit about inverting time.
> 
> The getters and setters bit was basically some stuff ckitching ran into at:
> 
> http://developer.android.com/training/articles/perf-tips.html#GettersSetters
> 
> Namely, they can be slow (and I hate them). But this is intentionally not in
> performance critical paths, so its probably not worth worrying about. I'll
> change it.

Yes, these tips apply to performance-critical code paths. Also, strictly speaking, the delta values should not change after the animation starts. With a setter you'd be able to catch these cases and throw an error accordingly. 

> > What exactly is this accomplishing? Not clear from the comment. Is this
> > about your point 1 (i.e. translating after rotation)?
> 
> No. For the counter animation, I added a shift in the z-direction as the
> counter moves (although I need to double check that animation now, since I
> reversed the orders of these transforms and changed things). If we were
> bringing the counter in (to zero degrees rotation), I'd transform from
> zdelta to 0. If I was sending the counter away, I'd move from 0 to zdelta.
> This just controls that direction. Instead of letting people pass that in, I
> thought I'd just remove a (confusing) parameter, and auto-calc it for you.
> 
> Alternatively, we could let you pass in startZ and endZ. Or we could add a
> new translate3D animation, and add it to the AnimationSet.

That sounds like too much magic to me. But I don't feel strongly about it, as long as it's well documented.

Comment 13

[Lucas Rocha (:lucasr)]

Comment on attachment 780832 [details] [diff] [review]
Patch 2/5

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

I'm really not fond of these public properties. Either add all or something of them to the constructor or use the Builder pattern. Animations are meant to be immutable once created.

::: mobile/android/base/animation/FlipAnimation.java
@@ +25,5 @@
> +    public int duration = 500;
> +    public float deltaX = 0;
> +    public float deltaY = 0;
> +    public float deltaZ = 0;
> +    public int angle = 45;

This is very unsafe, considering that you're doing this in an animation object. There's a reason why you can only set animation properties on their constructor. It's because they are, by design, meant to be immutable once they are created. Providing public properties that define how the animation will run goes against this fundamental invariant of animations in Android.

@@ +34,5 @@
> +    private boolean mFadeIn = true;
> +    private boolean flipForward = true;
> +
> +    public static final String FORWARD = "FORWARD";
> +    public static final String BACKWARD = "BACKWARD";

I'd go with an enum here.

@@ +43,5 @@
> +        mFadeIn = fadeIn;
> +        flipForward = FORWARD.equals(flipDir);
> +    }
> +
> +    public FlipAnimation build() {

The problem with this approach is that it doesn't enforce a correct usage of the API. I guess your intent here is that the animation should only be started if the has been 'built' properly. The proper way to implement this kind of API would be use something like the Builder pattern[1] which is usually be applied when you want to avoid a constructor for an (immutable) object with too many arguments while some or most of the arguments are optional.

I can probably apply the same idea to the previous patch on the Rotate3DAnimation too. Or you can simply add the delta arguments to your constructor.

[1] http://en.wikipedia.org/wiki/Builder_pattern

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 780833 [details] [diff] [review]
Patch 3/5

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

The patch itself looks fine but it touches a few things that has changed quite a bit in latest fig (after my patches for bug 889621). For instance, we'll have to figure out a way to animate differently depending on the case (tap on URL bar or new tab).

Comment 15

[Lucas Rocha (:lucasr)]

Comment on attachment 780834 [details] [diff] [review]
Patch 4/5

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

I like the approach for animating the new tab in the list. But don't you have to first open a gap in the list before flipping the new tab view? From the patch, it seems like the gap would snap on screen and then you'd flip the tab view in. Giving r- to clarify this part.

Also, I guess we'll have to setAnimation(null) on that resetTransforms() methods that you added in bug 895549?

With that said, I'm not entirely convinced about the animation from an UX perspective. Based on the animated mockups, it does seem like there's too much going on on the screen. A couple of alternatives comes to mind:
- Open gap in the list, fade in new tab item in the list, close tabs tray (about:home could fade in simultaneously in the content area)
- Close tabs tray, flip tab counter and about:home simultaneously (same animation than when a new tab created from the menu, bug 889619)

::: mobile/android/base/TabsPanel.java
@@ +39,5 @@
>          public void setTabsPanel(TabsPanel panel);
>          public void show();
>          public void hide();
>          public boolean shouldExpand();
> +        public void addInserting(int id);

addInsertingTab() for clarity? Also, id -> tabId?

@@ +129,5 @@
>             mActivity.addTab();
>          else
>             mActivity.addPrivateTab();
>  
>          mActivity.autoHideTabs();

Won't this start closing the tabs tray before the flipping animation starts?

::: mobile/android/base/TabsTray.java
@@ +277,5 @@
>                  row = (TabRow) convertView.getTag();
>              }
>  
>              Tab tab = mTabs.get(position);
>              assignValues(row, tab);

nit: add empty lines before and after this if.

@@ +293,5 @@
>  
> +    public void animateOpen(final View view) {
> +        FlipAnimation anim = new FlipAnimation(getContext(), FlipAnimation.BACKWARD, true);
> +        anim.deltaY = -0.5f;
> +        anim.build();

nit: add empty line here.

@@ +305,5 @@
> +                Tabs.getInstance().selectTab(row.id);
> +            }
> +            public void onAnimationRepeat(Animation animation) { }
> +            public void onAnimationStart(Animation animation) { }
> +        });

nit: add empty line here.

Comment 16

[Lucas Rocha (:lucasr)]

Comment on attachment 780836 [details] [diff] [review]
Patch 5/5

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

Looks like a sane approach. But needs rebasing and the suggested addTab change.

::: mobile/android/base/BrowserApp.java
@@ +1355,5 @@
>              mBrowserSearch.filter(searchTerm, handler);
>          }
>      }
>  
> +    public void animateShowHomePager(HomePager.Page page) {

This API is a bit different now.

::: mobile/android/base/TabsPanel.java
@@ +130,2 @@
>          else
> +            tab = Tabs.getInstance().loadUrl("about:home", Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_PRIVATE | Tabs.LOADURL_DELAY_LOAD);

I'd prefer to keep addTab() as central point to handle new tab. Maybe just add an extraFlags argument to addTab?

@@ +134,5 @@
>              // Add this tab to the inserting list so it will be animated
>              mTabsContainer.getCurrentPanelView().addInserting(tab.getId());
> +            // We also need to animate showing the java about:home. The real about:home won't
> +            // be created until the animation finishes, so that there's no flash of white from the new tab
> +            ((BrowserApp)mActivity).animateShowHomePager(tab.getAboutHomePage());

Isn't mActivity a BrowserApp already? No need for the casting I guess.

Does this mean that we'll show about:home but the current title (for whatever page you're current displaying) will remain the same until we actually switch to that tab, right?

Comment 17

[Ian Barlow (:ibarlow)]

In response to what Lucas suggested in comment 15, I'm open to the first suggestion, "Open gap in the list, fade in new tab item in the list, close tabs tray ", but not so much the second one. 

We want to communicate that something is happening in the tab tray when a new tab is created from there. It could be a fade or a zoom instead of a flip, but it should be something a user can see before the tray closes.

Comment 18

[Wesley Johnston (:wesj)]

(In reply to Lucas Rocha (:lucasr) from comment #15)
> I like the approach for animating the new tab in the list. But don't you
> have to first open a gap in the list before flipping the new tab view? From
> the patch, it seems like the gap would snap on screen and then you'd flip
> the tab view in. Giving r- to clarify this part.

Right now new tabs are added at the bottom of the list, so there's no need to open a gap. Working on adding them to the top.

Adding a tab and opening a gap without having a flash of content drawn at the wrong size (at the point we're adding the animation here, the view hasn't even been layed out yet, so we're racing with the AdapterView) turns out to be a bit difficult. I found a clever way to do it, but I'm fighting z-ordering right now.

We also wants about:home to open over about:home, which is also a bit difficult. I'm playing with that as well. I'll throw up a WIP and maybe someone will have some advice.

Comment 19

[Wesley Johnston (:wesj)]

Attachment: WIP Patch 6/5

This animates the views in from the top by overriding drawChild in TabsPanel. If the view we're drawing is animating, we move the canvas' current transform up a bit so that things "slide" in. Unfortunately, we still draw the new tab under the old one.

I also starting playing with caching about:home, drawing it underneith about:home and then transitioning about:home on the bitmap. Calling setBackground seems to hang my phone...

Comment 20

[Wesley Johnston (:wesj)]

Attachment: WIP Patch 6/5

This needs a bunch of cleanup, but manages to get things in an ok state:

1.) I'm fading in the new tab (at the top of the list now), since I can't figure out a non "do all the drawing ourselves" way to fix the z-order.

2.) I managed to get about:home to flip over about:home by reflecting it onto an ImageView and animating that. Its a bit ugly, but its the best solution I could come up with.

There's a build with this at:
http://people.mozilla.com/~wjohnston/flip.apk

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch 1/6 - Cleanup the rotate animation

I removed all of the translation code in here, and am adding back a Translate3D animation in the next patch. Seems better to just keep things separate.

Comment 22

[Wesley Johnston (:wesj)]

Attachment: Patch 2/6 - Add a translate3d animation

This adds the new animation. Technically we don't need either of these patches for this bug, but the cleanup is good.

Comment 23

[Wesley Johnston (:wesj)]

Attachment: Patch 3/6 - Animate showing the home pager

This adds a flip animation for showing the home pager. I wanted to use normal Animation objects here (since Rotate3D is one), so I set this up as a "default", and left the PropertyAnimator stuff in as a separate path.

In the long run, I think maybe we could pass in an enum value for animate instead of a boolean. Then the home pager could define the animations is supports (and we can move its AlphaAnimation into a normal animation... maybe)?

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Patch 4/6 - Animate adding new tabs

This is pretty similar to the old patch except its using an AlphaAnimation. I have another patch coming that does the work adding these to the top of the list.

Comment 25

[Wesley Johnston (:wesj)]

Attachment: Patch 5/5 - Open new tabs in the background until the animation finishes

This opens the new tab in a background tab, and then switches to it once the new tab animation is finished.

Comment 26

[Wesley Johnston (:wesj)]

Attachment: Patch 6/6 - Add new tabs to the top of the list

This adds new tabs to the top of the list. We animate their height in using a little canvas.transform trick. I played with animating the height of the row, but because we're setting up the animation in our adapter.getView() call, any layoutParameters we set are overwritten when the view is actually added to the TwoWayView. This seemed like a nice way to get around that.

ViewGroups do support a layoutAnimator, but since we only want these to happen to certain views, they don't work in a straightforward fashion. We could try using one and setting something in the TabRow tag that told the animation to only run on certain rows...

Comment 27

[Wesley Johnston (:wesj)]

Attachment: Patch 7/6 - Animate about:home even if its already showing

I can't count apparently.

This animates about:home over itself, by setting a picture of about:home as the background of its container.

Comment 28

[Lucas Rocha (:lucasr)]

Comment on attachment 782225 [details] [diff] [review]
Patch 1/6 - Cleanup the rotate animation

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

Nice.

::: mobile/android/base/TabCounter.java
@@ +105,5 @@
>                                           float zEnd, boolean reverse) {
>          final Context context = getContext();
>          AnimationSet set = new AnimationSet(context, null);
> +        Rotate3DAnimation rot = new Rotate3DAnimation(startAngle, endAngle, CENTER_X, CENTER_Y);
> +        //rot.deltaZ = zEnd;

Leftover?

Comment 29

[Lucas Rocha (:lucasr)]

Comment on attachment 782226 [details] [diff] [review]
Patch 2/6 - Add a translate3d animation

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

Cool.

Comment 30

[Lucas Rocha (:lucasr)]

Comment on attachment 782227 [details] [diff] [review]
Patch 3/6 - Animate showing the home pager

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

The new APIs in BrowserApp and HomePager look a bit confusing to me. What about something like:

In HomePager:
show(FragmentManager, Page)
showWithFlipAnimation(FragmentManager, Page)
showWithAnimator(FragmentManager, Page, PropertyAnimator)

Then have matching APIs in BrowserApp for each of these cases.

::: mobile/android/base/home/HomePager.java
@@ +111,5 @@
>          mLoaded = true;
>          TabsAdapter adapter = new TabsAdapter(fm);
>  
> +        if (page == null) {
> +            page = Page.BOOKMARKS;

When does this happen? Looks like a workaround for a bug.

::: mobile/android/base/resources/values/integers.xml
@@ +6,5 @@
>  <resources>
>  
>      <integer name="number_of_top_sites">6</integer>
>      <integer name="number_of_top_sites_cols">2</integer>
> +    <integer name="default_animation_length">300</integer>

It would be nice to use this same constant everywhere it's applicable. Please file a follow-up.

Comment 31

[Lucas Rocha (:lucasr)]

Comment on attachment 782228 [details] [diff] [review]
Patch 4/6 - Animate adding new tabs

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

Looks fine. Needs review from ibarlow (if it hasn't been reviewed yet).

::: mobile/android/base/TabsTray.java
@@ +181,5 @@
>                          return;
>  
>                      TabRow row = (TabRow) view.getTag();
>                      assignValues(row, tab);
> +

Unrelated?

@@ +278,5 @@
>                  row = (TabRow) convertView.getTag();
>              }
>  
>              Tab tab = mTabs.get(position);
>              assignValues(row, tab);

nit: add empty lines before and after this 'if'.

@@ +293,5 @@
>      }
>  
> +    public void animateOpen(final View view) {
> +        AlphaAnimation anim = new AlphaAnimation(0.0f, 1.0f);
> +        anim.setDuration(mContext.getResources().getInteger(R.integer.default_animation_length));

I guess this means the gap for the new gap simply snaps on screen?

Comment 32

[Wesley Johnston (:wesj)]

(In reply to Lucas Rocha (:lucasr) from comment #30)
> show(FragmentManager, Page)
> showWithFlipAnimation(FragmentManager, Page)
> showWithAnimator(FragmentManager, Page, PropertyAnimator)

What I personally would like is to just have

public void show(Fragmentmanager, Page, TransitionType);

where AnimationType is an enum {
   FLIP,
   FADE,
   FUTURE_AWESOME_TRANSITION,
   NONE
}

It felt like this bug was growing beyond its scope though, so I left things as is for now. 

That requires removing the PropertyAnimator (which isn't really needed here I think?) The nice thing about the PropertyAnimator is that you can attach other animations (or listeners) to it, so maybe we'd be better to push Rotate3D Animation into it somehow and always using one of them instead though.

Comment 33

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #32)
> (In reply to Lucas Rocha (:lucasr) from comment #30)
> > show(FragmentManager, Page)
> > showWithFlipAnimation(FragmentManager, Page)
> > showWithAnimator(FragmentManager, Page, PropertyAnimator)
> 
> What I personally would like is to just have
> 
> public void show(Fragmentmanager, Page, TransitionType);
> 
> where AnimationType is an enum {
>    FLIP,
>    FADE,
>    FUTURE_AWESOME_TRANSITION,
>    NONE
> }

Yep, that would be the ideal API.

> It felt like this bug was growing beyond its scope though, so I left things
> as is for now. 

Fair enough. But I'd say this is actually a core aspect of this bug.

> That requires removing the PropertyAnimator (which isn't really needed here
> I think?) The nice thing about the PropertyAnimator is that you can attach
> other animations (or listeners) to it, so maybe we'd be better to push
> Rotate3D Animation into it somehow and always using one of them instead
> though.

Passing the animator is quite important so that the different components of the editing mode transition (toolbar + homepager) animate using a single property animator.

Yeah, I'd love to consolidate all our animation bits around the same API. This would make it easier to handle cases like this bug.

Comment 34

[Lucas Rocha (:lucasr)]

Comment on attachment 782230 [details] [diff] [review]
Patch 6/6 - Add new tabs to the top of the list

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

Looks generally good but the patch summary does really match what the patch does. For instance, how does the animation part relate to adding new tabs to top?

Just want questions answered before giving r+.

::: mobile/android/base/TabsPanel.java
@@ +139,5 @@
>              // be created until the animation finishes, so that there's no flash of white from the new tab
>              ((BrowserApp)mActivity).showHomePager(tab.getAboutHomePage(), true);
> +
> +            // Select the new tab after a short delay, to ensure its scrolled into view and the tabs animation shows
> +            postDelayed(new Runnable() {

Is the 100ms delay just magic number that that seems to work well? Does it have a special meaning?

@@ +141,5 @@
> +
> +            // Select the new tab after a short delay, to ensure its scrolled into view and the tabs animation shows
> +            postDelayed(new Runnable() {
> +                public void run() {
> +                    ((TwoWayView)mTabsContainer.getCurrentPanelView()).setSelection(0);

Is it actually safe to cast to TwoWayView here?

::: mobile/android/base/TabsTray.java
@@ +297,5 @@
>  
>      public void animateOpen(final View view) {
> +        TabRowAnimationWrapper wrapper = new TabRowAnimationWrapper(true);
> +        wrapper.addAnimation(new AlphaAnimation(0.0f, 1.0f));
> +        wrapper.setDuration(mContext.getResources().getInteger(R.integer.default_animation_length));

Does using TabRowAnimationWrapper change the animation in any visible way?

::: mobile/android/base/animation/TabRowAnimationWrapper.java
@@ +18,5 @@
> +/**
> + * An animation that rotates the view on the Y axis between two specified angles.
> + * This animation also adds a translation on the Z axis (depth) to improve the effect.
> + */
> +public class TabRowAnimationWrapper extends AnimationSet {

TabRowAnimationSet instead?

@@ +41,5 @@
> +        return super.getTransformation(time, t);
> +    }
> +
> +    @Override
> +    public boolean willChangeTransformationMatrix() { return true; }

nit: indent this as usual.

@@ +54,5 @@
> +    // This implementation just shifts the canvas transformation so that the view slides in.
> +    public void doAnimation(boolean isVertical, View view, Canvas canvas) {
> +        float t = (currentTime - 1.0f);
> +        canvas.translate(isVertical ? 0 : view.getWidth()*t,
> +                         isVertical ? view.getHeight()*t : 0.0f);

Where is this used? Why is this not implemented as a translation animation?

Comment 35

[Lucas Rocha (:lucasr)]

Comment on attachment 782231 [details] [diff] [review]
Patch 7/6 - Animate about:home even if its already showing

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

Maybe this code should all be in HomePager somehow? This is where having completely different code paths for each HomePager animation starts hurting us. Giving r+ simply because I can't think of a better way to do it from the top of my mind. But I really think we should move towards consolidating these animations into the same code path somehow.

::: mobile/android/base/BrowserApp.java
@@ +1359,5 @@
>      }
>  
>      public void showHomePager(HomePager.Page page, boolean animate) {
> +        if (mHomePager.isVisible() && animate) {
> +            updateContainerBackgroundForAnimation();

Won't this cause about:home to reload even when it doesn't have to?

@@ +1396,5 @@
>          mHomePager.setupAnimator(animator);
> +        animator.addPropertyAnimationListener(new PropertyAnimator.PropertyAnimationListener() {
> +            public void onPropertyAnimationStart() { }
> +            public void onPropertyAnimationEnd() {
> +                mHomePagerContainer.setBackgroundResource(0);

I'm slightly uncomfortable with these setBackgroundResource() calls in different spots. Very prone to breaking easily. Ideally both animations would use same code path somehow. Maybe updateContainerBackgroundForAnimation() should be a private method in HomePager?

Comment 36

[Lucas Rocha (:lucasr)]

Comment on attachment 782229 [details] [diff] [review]
Patch 5/5 - Open new tabs in the background until the animation finishes

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

Nice trick.

::: mobile/android/base/BrowserApp.java
@@ +303,5 @@
>                      tab.doStop();
>                      return true;
>  
>                  case KeyEvent.KEYCODE_T:
> +                    addTab(0);

USe Tabs.LOADURL_NONE for clarity.

@@ +1851,5 @@
>                  }
>                  GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("DesktopMode:Change", args.toString()));
>                  return true;
>              case R.id.new_tab:
> +                addTab(0);

USe Tabs.LOADURL_NONE for clarity.

@@ +1856,3 @@
>                  return true;
>              case R.id.new_private_tab:
> +                addPrivateTab(0);

Ditto.

::: mobile/android/base/BrowserToolbar.java
@@ +852,5 @@
>          return false;
>      }
>  
>      private void addTab() {
> +        mActivity.addTab(0);

Same.

::: mobile/android/base/TabsTray.java
@@ +298,5 @@
>  
>          anim.setAnimationListener(new Animation.AnimationListener() {
>              public void onAnimationEnd(Animation animation) {
>                  // After we animate adding a tab, close the panel
>                  autoHidePanel();

nit: add empty line here.

Comment 37

[Lucas Rocha (:lucasr)]

Before I forget: when I suggested fading the new tabs tray item instead of flipping in comment #15, I also expected the about:home content to fade instead of flip. I think the animations in tabs tray and content area should be same. Ian?

Comment 38

[Ian Barlow (:ibarlow)]

That's not how I envisioned it, but I can see your point. Worth trying.

Comment 39

[Ian Barlow (:ibarlow)]

Just pinging to see how things are going here. Would be great to get this work, as well as the transition in bug 889619 wrapped up and landed before our test day next week.

Comment 40

[Wesley Johnston (:wesj)]

Yeah. I made most of the changes I think lucas wanted, but got distracted with some other work. I'll bump this up in my queue.

Comment 41

[Wesley Johnston (:wesj)]

Attachment: Patch 3/6

Apologies for the delay again. I went with an enum here, so now BrowserToolbar and HomePager both have something like:

show(FragmentManager, Page, AnimationType, PropertyAnimator);

where AnimationType is an enum { FLIP, ANIMATOR, NONE }

Maybe Animator should be "Fade", since HomePager adds a Fade Animation to it? but its still a bit confusing in that case if a PropertyAnimator is needed or not, so I thought ANIMATOR would be clearer to callees. I also fixed this so we don't attach a new adapter if about:home is already visible (alternatively, we could just check if it has an adapter or not...)

Comment 42

[Wesley Johnston (:wesj)]

Attachment: Patch 4/6

Carrying the r+. No changes here, just unbitrotting.

Comment 43

[Wesley Johnston (:wesj)]

Attachment: Patch 5/6

Carrying r+

Comment 44

[Wesley Johnston (:wesj)]

Attachment: Patch 6/6

> Where is this used? Why is this not implemented as a translation animation?
I left out a key piece of this last time when qcrecord splitting it up I think. This adds back the bit that actually calls into the animator. Hope its clearer now?

I don't think a translate animation gets us what we want (all of the tabs list sliding down). We need to either animate height, or do a nicer trick like this that doesn't actually cost us layout calculations.

Comment 45

[Wesley Johnston (:wesj)]

Attachment: Path 7/6

Carrying r+. Unbitrotting.

Comment 46

[Wesley Johnston (:wesj)]

Review Ping!

Also, I thought I should mention that I left the flip animation in for the about:home page. If we do an alpha transition there's sometimes no change when opening a new tab (if you're already showing about:home). I've grown to love that transition, and it makes it obvious-er that something happened to the user.

Comment 47

[Lucas Rocha (:lucasr)]

Comment on attachment 789225 [details] [diff] [review]
Patch 3/6

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

Looks generally good. But I think the API could be a bit more robust.

::: mobile/android/base/BrowserApp.java
@@ +1356,5 @@
>                  }
>              }
>          }
>  
> +        final PropertyAnimator animator = new PropertyAnimator(getContext().getResources().getInteger(R.integer.default_animation_length));

nit: freaking long line, no? :-)

@@ +1400,5 @@
>              mBrowserSearch.filter(searchTerm, handler);
>          }
>      }
>  
> +    public void showHomePager(HomePager.Page page, HomePager.AnimationType type, PropertyAnimator animator) {

Add equivalent flavours of the new show* methods in HomePager in BrowserApp and use them according to each case.

::: mobile/android/base/home/HomePager.java
@@ +73,5 @@
>          setOffscreenPageLimit(2);
>      }
>  
> +    private Animation getFlipAnimation() {
> +        if (mEnterAnimation != null)

nit: add {}'s

@@ +95,5 @@
> +        });
> +        return mEnterAnimation;
> +    }
> +
> +    public void show(FragmentManager fm, Page page, AnimationType type, PropertyAnimator animator) {

This API is a bit too hand-wavy. The fact that the animator argument only applies to a specific animation type is not cool. Given that each type of animation is implemented in a different way, I'd prefer something more controlled with specific methods for type.

show(FragmentManager fm) - no animation
showWithFlipAnimation(FragmentManager fm) - use FlipAnimation internally
showWithAnimator(FragmentManager fm, PropertyAnimator animator) - show using arbitrary animator

Factor our the current show() method into something that can be reused in these three new methods.

@@ +100,5 @@
>          mLoaded = true;
>          TabsAdapter adapter = new TabsAdapter(fm);
>  
> +        if (page == null) {
> +            page = Page.BOOKMARKS;

Is this really necessary? Looks like something that will cover an actual bug. Page should never really be null.

::: mobile/android/base/resources/values/integers.xml
@@ +6,5 @@
>  <resources>
>  
>      <integer name="number_of_top_sites">6</integer>
>      <integer name="number_of_top_sites_cols">2</integer>
> +    <integer name="default_animation_length">300</integer>

Please file a follow-up to use this same integer consistency across all animations.

Comment 48

[Lucas Rocha (:lucasr)]

Comment on attachment 789230 [details] [diff] [review]
Patch 6/6

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

Looks nice, just needs some more cleanups. Please provide a build to ibarlow for UX review (if you haven't yet).

::: mobile/android/base/TabsPanel.java
@@ +142,5 @@
> +
> +            // Select the new tab after a short delay, to ensure its scrolled into view and the tabs animation shows
> +            postDelayed(new Runnable() {
> +                public void run() {
> +                    final PanelView v = mTabsContainer.getCurrentPanelView();

This is racy. What if the current panel changes between the beginning of this call and when this runnable is actually called? You should probably assign getCurrentPanelView() to a final variable at the top of this method and use it everywhere here.

@@ +143,5 @@
> +            // Select the new tab after a short delay, to ensure its scrolled into view and the tabs animation shows
> +            postDelayed(new Runnable() {
> +                public void run() {
> +                    final PanelView v = mTabsContainer.getCurrentPanelView();
> +                    if (v instanceof TwoWayView) {

Maybe you should simply cast to TwoWayView directly here. This code should never run in the remote tabs panel.

@@ +147,5 @@
> +                    if (v instanceof TwoWayView) {
> +                        ((TwoWayView)v).setSelection(0);
> +                    }
> +                }
> +            }, 0);

AFAIK, this is equivalent to simply using post() i.e. internally, it will call postDelayed(..., 0);

::: mobile/android/base/TabsTray.java
@@ +314,5 @@
>          return (getOrientation().compareTo(TwoWayView.Orientation.VERTICAL) == 0);
>      }
>  
>      public void animateOpen(final View view) {
> +        TabRowAnimationSet set = new TabRowAnimationSet(true);

final

@@ +336,5 @@
> +    }
> +
> +    @Override
> +    public boolean drawChild(Canvas canvas, View child, long drawingTime) {
> +        boolean res = super.drawChild(canvas, child, drawingTime);

nit: add empty line here.

@@ +337,5 @@
> +
> +    @Override
> +    public boolean drawChild(Canvas canvas, View child, long drawingTime) {
> +        boolean res = super.drawChild(canvas, child, drawingTime);
> +        Animation anim = child.getAnimation();

final

@@ +339,5 @@
> +    public boolean drawChild(Canvas canvas, View child, long drawingTime) {
> +        boolean res = super.drawChild(canvas, child, drawingTime);
> +        Animation anim = child.getAnimation();
> +        if (anim instanceof TabRowAnimationSet) {
> +            ((TabRowAnimationSet)anim).doAnimation(isVertical(), child, canvas);

nit: I kinda hate these embedded casting. They don't make code that much shorter and are much harder to read. I'd go with:

final TabRowAnimationSet tabRowAnim = (TabRowAnimationSet) anim;
tabRowAnim.doAnimation(...)

@@ +340,5 @@
> +        boolean res = super.drawChild(canvas, child, drawingTime);
> +        Animation anim = child.getAnimation();
> +        if (anim instanceof TabRowAnimationSet) {
> +            ((TabRowAnimationSet)anim).doAnimation(isVertical(), child, canvas);
> +        }

nit: add empty line here.

::: mobile/android/base/animation/TabRowAnimationSet.java
@@ +19,5 @@
> + * An animation that rotates the view on the Y axis between two specified angles.
> + * This animation also adds a translation on the Z axis (depth) to improve the effect.
> + */
> +public class TabRowAnimationSet extends AnimationSet {
> +    private float currentTime = 1.0f;

currentTime -> mCurrentTime

@@ +29,5 @@
> +    // applyTransformation() isn't called for AnimationSets, but getTransformation() is.
> +    // Override it here to cache off the interpolated time so that we can refer to it in doAnimation()
> +    @Override
> +    public boolean getTransformation(long time, Transformation t) {
> +        if (getStartTime() == -1) {

assign getStartTime to final at the top and use it everywhere here.

@@ +32,5 @@
> +    public boolean getTransformation(long time, Transformation t) {
> +        if (getStartTime() == -1) {
> +            currentTime = 0.0f;
> +        } else {
> +            currentTime = (time -getStartTime()) / ((float)getDuration());

nit: space between (float) and getDuration()

@@ +35,5 @@
> +        } else {
> +            currentTime = (time -getStartTime()) / ((float)getDuration());
> +        }
> +
> +        Interpolator interp = getInterpolator();

final

@@ +54,5 @@
> +
> +    // This should be called during the drawChild call for a viewgroup, in addition to the typical child.draw call.
> +    // This implementation just shifts the canvas transformation so that the view slides in.
> +    public void doAnimation(boolean isVertical, View view, Canvas canvas) {
> +        float t = (currentTime - 1.0f);

final

Comment 49

[Wesley Johnston (:wesj)]

(In reply to Lucas Rocha (:lucasr) from comment #47)
> show(FragmentManager fm) - no animation
> showWithFlipAnimation(FragmentManager fm) - use FlipAnimation internally
> showWithAnimator(FragmentManager fm, PropertyAnimator animator) - show using
> arbitrary animator

This feels way to close to having api's like

showWithStarWipeAndPurpleBarsAndHaveAUnicornRunAlongTheBottom(FragmentManager fm);

I think the right way to do this is to probably make the PropertyAnimator not completely oblivious to Android's normal animation frameworks. i.e.

1.) it doesn't need its own special listener interface but can just reuse or extend Animation.AnimationListener which would mean we could use the same listener for both animations here

2.) You should be able to post a normal Andriod Animation object to it, and have that run in sync with animation of other properties (in fact, that seems like a better way to do these alpha animations than us rolling them by hand, and might fix our GB problems?)

Those are what I meant by scope creep in this bug, but I hate the above API's alot. Fixing these bugs seems like the only way to get us to something like:

Animation show(FragmentManager, AnimationType type);
and
animator.add(homePager, homePager.show(fm, AnimationType.FADE));

Comment 50

[Wesley Johnston (:wesj)]

Attachment: Patch 1.5/7

This adds rotateX abilities to the PropertyAnimator. I tried for awhile to set up the PropertyAnimator so that it could take something like:

  animator.attachAnimation(View view, Animation anim);

but gave up after hitting far to many walls. This is simple. Restrictive, but simple.

Comment 51

[Wesley Johnston (:wesj)]

Attachment: Patch 3/7

Uses the new PropertyAnimator stuff.

I keep leaving the if (page == null) check, because I feel like this needs a way to say "use whatever you feel like" when showing about:home. i.e. a default. At least, I wound up needing it here.

I guess we could override the method with one that doesn't have the Page parameter, but I don't really feel like taking a null argument is too un-javaish (Android does it all over...).

Comment 52

[Wesley Johnston (:wesj)]

Attachment: Patch 6/7

I think I've got your changes addressed here.

Comment 53

[Wesley Johnston (:wesj)]

Attachment: Patch 6/7

Missed a few changes.

Comment 54

[Wesley Johnston (:wesj)]

Attachment: Patch 7/7

Updates this to use the animator now. Since its changed, asking for r?, but the changes are fairly minor.

Comment 55

[Lucas Rocha (:lucasr)]

Comment on attachment 792249 [details] [diff] [review]
Patch 3/7

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

Looks good overall but still needs some cleanups. Make sure you're not breaking the current transition to enter editing mode (when tapping the URL bar).

::: mobile/android/base/BrowserApp.java
@@ +1388,5 @@
>          if (url == null) {
>              throw new IllegalArgumentException("Cannot handle null URLs in enterEditingMode");
>          }
>  
> +        final PropertyAnimator animator = new PropertyAnimator(2000);//getContext().getResources().getInteger(R.integer.default_animation_length));

Leftover? :-) Also, now I wonder if we should actually use Android's config_longAnimTime, config_mediumAnimTime, and config_shortAnimTime instead?

@@ +1393,4 @@
>          animator.setUseHardwareLayer(false);
>  
>          mBrowserToolbar.startEditing(url, animator);
> +        showHomePager(HomePager.Page.HISTORY, HomePager.AnimationType.FLIP, animator);

This should actually be AnimationType.FADE.

@@ -1432,5 @@
>              mBrowserSearch.filter(searchTerm, handler);
>          }
>      }
>  
> -    private void showHomePager(HomePager.Page page) {

I'd prefer to keep this as the short for showHomePager(page, AnimationType.NONE, null)

::: mobile/android/base/home/HomePager.java
@@ +120,5 @@
>          mLoaded = true;
>          TabsAdapter adapter = new TabsAdapter(fm);
>  
> +        if (page == null) {
> +            page = Page.BOOKMARKS;

As I said, if the given page is undefined, this is a bug somewhere, not a valid case. Having a auto-default page would be nice but in this case I'd prefer to make sure we are very explicit about the expected behaviour on each HomePager.show() call.

@@ +126,2 @@
>  
> +        // We can animate showing about:home, even if about:home is already showing. Don't refresh the adapter if we don't have to

nit: break this comment into fewer cols.

@@ +129,5 @@
> +            // Add the pages to the adapter in order.
> +            adapter.addTab(Page.HISTORY, HistoryPage.class, null, getContext().getString(R.string.home_history_title));
> +            adapter.addTab(Page.BOOKMARKS, BookmarksPage.class, null, getContext().getString(R.string.bookmarks_title));
> +            adapter.addTab(Page.READING_LIST, ReadingListPage.class, null, getContext().getString(R.string.reading_list));
> +    

nit: remove trailing whitespace here.

@@ +137,5 @@
>          setVisibility(VISIBLE);
>  
> +        switch(type) {
> +            case FLIP:
> +                // startAnimation(getFlipAnimation( && Build.VERSION.SDK_INT >= 11));

I wonder if we want to actually enable these fancy animations on pre-Honeycomb devices. Seems like you need to rebase your patch. The latest code disables at least the fade animation on tablets and pre-HC devices.

@@ +141,5 @@
> +                // startAnimation(getFlipAnimation( && Build.VERSION.SDK_INT >= 11));
> +                Rotate3DAnimation.RotationDetails details = new Rotate3DAnimation.RotationDetails(0.5f, 0.5f);
> +                ViewHelper.setRotationX(this, -45.0f, details);
> +                ViewHelper.setAlpha(this, 0.0f);
> +                ViewHelper.setTranslationY(this, 0.5f*getHeight());

nit: add empty line here.

Comment 56

[Lucas Rocha (:lucasr)]

Comment on attachment 792245 [details] [diff] [review]
Patch 1.5/7

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

I'm a bit confused by this patch. I thought the idea was to remove Rotate3DAnimation completely and replace it with a PropertyAnimator based animation?

Comment 57

[Wesley Johnston (:wesj)]

(In reply to Lucas Rocha (:lucasr) from comment #56)
> I'm a bit confused by this patch. I thought the idea was to remove
> Rotate3DAnimation completely and replace it with a PropertyAnimator based
> animation?

Nope. In fact, my original plan was to make the PropertyAnimator take normal animations. i.e.

PropertyAnimation.attach(View, Animation);

but because of the way that the animator works on HC+, it turns out to be a lot of work. I did this as a shortcut to try and end scope creep on this bug.

Comment 58

[Wesley Johnston (:wesj)]

Attachment: Patch 3/7

Rebased. Sorry about the cruft last time. Serves me right for writing something Firday and submitting it monday morning.

Comment 59

[Wesley Johnston (:wesj)]

Attachment: Patch 1.5/7

Rebased

Comment 60

[Wesley Johnston (:wesj)]

Attachment: Patch 7/7

Comment 61

[Lucas Rocha (:lucasr)]

Comment on attachment 792963 [details] [diff] [review]
Patch 7/7

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

::: mobile/android/base/BrowserApp.java
@@ +1459,5 @@
> +            if (type == HomePager.AnimationType.NONE)
> +                return;
> +
> +            if (animator != null) {
> +                updateContainerBackgroundForAnimation();

nit: add empty line here.

::: mobile/android/base/TabsTray.java
@@ +322,5 @@
>          set.setAnimationListener(new Animation.AnimationListener() {
>              public void onAnimationEnd(Animation animation) {
>                  // After we animate adding a tab, close the panel
>                  autoHidePanel();
> +

unrelated?

Comment 62

[Lucas Rocha (:lucasr)]

Comment on attachment 792962 [details] [diff] [review]
Patch 1.5/7

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

It feels to me that these pivots are not a property of an animation. They are a property of the target view. So this 'details' argument in the APIs feel very unnatural. Here what I suggest:

Add pivotX and pivotY getters/setters to AnimatorProxy (on pre-HC they will be stored in the AnimatorProxy's Animation object). Add corresponding setters in ViewHelper too. Call setPivotX/setPivotY before/after your animations where needed. Remove all the 'details' bits from the API.

::: mobile/android/base/animation/AnimatorProxy.java
@@ +227,5 @@
>              m.postTranslate(mTranslationX, mTranslationY);
> +            if (mRotationDetails == null) {
> +                mRotationDetails = new Rotate3DAnimation.RotationDetails();
> +            }
> +            Rotate3DAnimation.rotateX(m, mRotationX,

This inter-dependency between Rotate3DAnimation and AnimatorProxy is a bit unfortunate...

Comment 63

[Lucas Rocha (:lucasr)]

Comment on attachment 792961 [details] [diff] [review]
Patch 3/7

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

Looks good overall. Just needs to account for the API changes I suggested in my previous review.

::: mobile/android/base/BrowserApp.java
@@ +473,5 @@
>              updateSideBarState();
>          }
>  
>          mFindInPageBar = (FindInPageBar) findViewById(R.id.find_in_page);
> +        mMediumDuration = getResources().getInteger(android.R.integer.config_mediumAnimTime);

We should probably store these values in something like AnimationUtils.MEDIUM_ANIM_TIME. But we can discuss this in bug 906749.

@@ +1452,5 @@
> +    public void showHomePager(HomePager.AnimationType type, PropertyAnimator animator) {
> +        showHomePager(HomePager.Page.BOOKMARKS, type, animator);
> +    }
> +
> +    public void showHomePager(HomePager.Page page, HomePager.AnimationType type, PropertyAnimator animator) {

nit: It's not very clear why you're changing/adding showHomePager in this patch given that you don't really use all the signatures here. I guess it counts as "add API in preparation for a next patch" but still...

::: mobile/android/base/home/HomePager.java
@@ +146,4 @@
>                  getContext().getString(R.string.reading_list_title));
>  
> +            setAdapter(adapter);
> +        }

nit: add empty line here.

@@ +152,3 @@
>          adapter.setCanLoadHint(!shouldAnimate);
>  
> +        if (!shouldAnimate || animator == null || Build.VERSION.SDK_INT < 11) {

nit: I personally don't like early returns in the middle of a method as they are easy to miss. I don't feel strongly about this one though.

@@ +170,4 @@
>  
> +        switch(type) {
> +            case FLIP:
> +                Rotate3DAnimation.RotationDetails details = new Rotate3DAnimation.RotationDetails(0.5f, 0.5f);

Replace this with ViewHelper.setPivotX() and ViewHelper.setPivotY() (as per my review of the other patch)

Comment 64

[Lucas Rocha (:lucasr)]

Comment on attachment 792255 [details] [diff] [review]
Patch 7/7

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

Why do I feel like I have already reviewed this?

Comment 65

[Wesley Johnston (:wesj)]

(In reply to Lucas Rocha (:lucasr) from comment #62)
> Comment on attachment 792962 [details] [diff] [review]
> Patch 1.5/7
> 
> Review of attachment 792962 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It feels to me that these pivots are not a property of an animation. They
> are a property of the target view. So this 'details' argument in the APIs
> feel very unnatural. Here what I suggest:

Maybe its the mathematician in me, but pivots really are a property of the transform being applied. RotateX is an ambiguous statement on its own, unless you give a point to transform around. Attaching the pivot in a different place just makes the calls harder to parse (and would be confusing to people if two PropertyAnimations on the same view were applied in different places).

The PropertyAnimator already has problems where it just ignores order of operations for these more complex types of transforms (we're basically forcing all translation to happen before rotations right now). I don't really want to make it worse.

Comment 66

[Wesley Johnston (:wesj)]

Not to mention that attaching the pivot to the transform also allows you to do fancy things like have two different animations occurring around separate "pivot" points simultaneously.

Comment 67

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #65)
> (In reply to Lucas Rocha (:lucasr) from comment #62)
> > Comment on attachment 792962 [details] [diff] [review]
> > Patch 1.5/7
> > 
> > Review of attachment 792962 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > It feels to me that these pivots are not a property of an animation. They
> > are a property of the target view. So this 'details' argument in the APIs
> > feel very unnatural. Here what I suggest:
> 
> Maybe its the mathematician in me, but pivots really are a property of the
> transform being applied. RotateX is an ambiguous statement on its own,
> unless you give a point to transform around. Attaching the pivot in a
> different place just makes the calls harder to parse (and would be confusing
> to people if two PropertyAnimations on the same view were applied in
> different places).

Well, on the other hand, you might want to animate pivots while rotating too. Pivots became a View property (just like rotation*, translation*, etc) in post-HC.

> The PropertyAnimator already has problems where it just ignores order of
> operations for these more complex types of transforms (we're basically
> forcing all translation to happen before rotations right now). I don't
> really want to make it worse.

I don't follow. Where in PropertyAnimator do we force translation first?

Comment 68

[Wesley Johnston (:wesj)]

(In reply to Wesley Johnston (:wesj) from comment #65)
> (In reply to Lucas Rocha (:lucasr) from comment #62)
> > Comment on attachment 792962 [details] [diff] [review]
> > Patch 1.5/7
> > 
> > Review of attachment 792962 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > It feels to me that these pivots are not a property of an animation. They
> > are a property of the target view. So this 'details' argument in the APIs
> > feel very unnatural. Here what I suggest:
> 
> Maybe its the mathematician in me, but pivots really are a property of the
> transform being applied. RotateX is an ambiguous statement on its own,
> unless you give a point to transform around. Attaching the pivot in a
> different place just makes the calls harder to parse (and would be confusing
> to people if two PropertyAnimations on the same view were applied in
> different places).
> 
> The PropertyAnimator already has problems where it just ignores order of
> operations for these more complex types of transforms (we're basically
> forcing all translation to happen before rotations right now). I don't
> really want to make it worse.

(In reply to Lucas Rocha (:lucasr) from comment #67)
> (In reply to Wesley Johnston (:wesj) from comment #65)
> > (In reply to Lucas Rocha (:lucasr) from comment #62)
> > > Comment on attachment 792962 [details] [diff] [review]
> > > Patch 1.5/7
> > > 
> > > Review of attachment 792962 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > It feels to me that these pivots are not a property of an animation. They
> > > are a property of the target view. So this 'details' argument in the APIs
> > > feel very unnatural. Here what I suggest:
> > 
> > Maybe its the mathematician in me, but pivots really are a property of the
> > transform being applied. RotateX is an ambiguous statement on its own,
> > unless you give a point to transform around. Attaching the pivot in a
> > different place just makes the calls harder to parse (and would be confusing
> > to people if two PropertyAnimations on the same view were applied in
> > different places).
> 
> Well, on the other hand, you might want to animate pivots while rotating
> too. Pivots became a View property (just like rotation*, translation*, etc)
> in post-HC.

True. Still doable in either case (although if you set up a rotation animation expecting a certain pivot, and someone else decides to do a different rotation with a different pivot at the same time, you're probably in for a confusing ride). Since "pivots" mean essentially nothing outside the context of a transform, I wonder if you'd be better to just include start and end properties in a details object if you wanted to support that.

> > The PropertyAnimator already has problems where it just ignores order of
> > operations for these more complex types of transforms (we're basically
> > forcing all translation to happen before rotations right now). I don't
> > really want to make it worse.
> 
> I don't follow. Where in PropertyAnimator do we force translation first?

The pre-honeycomb version doesn't have any concept of order in its transformMatrix function:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/animation/AnimatorProxy.java#212

We need to either enforce something like that (i.e. we probably need things like setTranslationX to modify a matrix of their own rather than storing properties individually).

Comment 69

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #68)
> True. Still doable in either case (although if you set up a rotation
> animation expecting a certain pivot, and someone else decides to do a
> different rotation with a different pivot at the same time, you're probably
> in for a confusing ride). Since "pivots" mean essentially nothing outside
> the context of a transform, I wonder if you'd be better to just include
> start and end properties in a details object if you wanted to support that.

View transforms (rotation, translation, ...) in pre-HC were directly tied to animations i.e. you couldn't really rotate a view outside the context of an animation. On post-HC though, transforms became properties of the View itself, including the pivots. This means that each view has only one pivot point. I can't think of a way how you'd be able to implement concurrent animations using completely different pivots i.e. changing the view pivot for one animation would necessarily affect the other transforms in the view.

My point here is that the 'details' argument is trying to artificially add an API layer that can be easily broken using the standard platform API. Passing a 'details' object to PropertyAnimator doesn't really stop anyone from setting pivot outside the animation. That's just how Android's API has been designed.

I can see the benefit of adding an API that allows you to set properties that only apply during an animation. But the API for this would probably have to be a bit more robust/generic than what's in the current patch—imo anyway.

Comment 70

[Wesley Johnston (:wesj)]

Ah, you're right. Calling something like:

view.setPivotX(50);
view.setRotationX(50):
view.setPivotX(0);
view.setRotationY(50);

doesn't work like you'd expect it to.

> I can't think of a way how you'd be able to implement concurrent animations using 
> completely different pivots i.e. changing the view pivot for one animation would 
> necessarily affect the other transforms in the view.

> My point here is that the 'details' argument is trying to artificially add an API layer 
> that can be easily broken using the standard platform API. Passing a 'details' object to 
> PropertyAnimator doesn't really stop anyone from setting pivot outside the animation. 
> That's just how Android's API has been designed.

I'm not sure how anything else fixes this problem either though. The API I put up tries pretty hard to keep the pivot point in the place it expects during the animation. Any other API would do the same thing and suffer the same problems. i.e. this isn't a problem with the 'details' argument. Its a problem with Android's implementation. And if Android ever fixed their API, it has a hope of working (or being adapted to work), while the alternative doesn't?

Right now, to do this correctly in a post Honeycomb phone, you'd still have to use the Animation framework, which does the job correctly by letting you stack transform matricies together into one final transform for the view.

Comment 71

[Lucas Rocha (:lucasr)]

Ok, it seems we don't really agree here ;-) I don't want us to block on this though. here's my suggestion. Add an attachWithPivot(view, property, value, pivotX, pivotX). This method should throw if the given property is not rotateX. I really don't like the generic "details" object being passed around with type safety. attachWithPivot is at least more explicit and does not involve doing random type castings.

Comment 72

[Ian Barlow (:ibarlow)]

Friendly ping to check on progress here, -- while it wasn't crucial for v1 this would be a nice transition to see sooner rather than later :)

Comment 73

[Wesley Johnston (:wesj)]

Attachment: Patch 1/7 - Cleanup Rotate3d

Lets try these again :)

Comment 74

[Wesley Johnston (:wesj)]

Attachment: Patch 2/7 - Add rotations to AnimatorProxy

This adds the animator proxy stuff we've been talking about. Still gets a bit strange because right now, the PropertyAnimator doesn't expose the proxy to callers, making it hard to set the pivot in there. I just exposed the proxy.

Alternatively, I could go all in and just expose the two different pivots as animatable values.

Comment 75

[Wesley Johnston (:wesj)]

Attachment: Patch 3/7 - Translate3D Animator

Comment 76

[Wesley Johnston (:wesj)]

Attachment: Patch 4/7 - Add a FLIP animation to the HomePager

Comment 77

[Wesley Johnston (:wesj)]

Attachment: Patch 5/7 - Fade in new Tabs

Comment 78

[Wesley Johnston (:wesj)]

Attachment: Patch 6/7 - Add new tabs to the top

Comment 79

[Wesley Johnston (:wesj)]

Attachment: Patch 7/7 - Flip up the home pager

Comment 80

[Wesley Johnston (:wesj)]

Ping?

Comment 81

[Wesley Johnston (:wesj)]

Build for ian: http://people.mozilla.com/~wjohnston/flip.apk

Comment 82

[Lucas Rocha (:lucasr)]

Comment on attachment 826191 [details] [diff] [review]
Patch 2/7 - Add rotations to AnimatorProxy

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

Looks good overall but I need these questions answered before giving r+.

::: mobile/android/base/animation/AnimatorProxy.java
@@ +439,5 @@
> +
> +        @Override
> +        public PointF getPivot() {
> +            return new PointF(view.getPivotX() / view.getWidth(),
> +                              view.getPivotY() / view.getHeight());

Doesn't getPivotX/getPivotY already gives you the position in the view? Why do you need to divide by width/height here?

::: mobile/android/base/animation/PropertyAnimator.java
@@ +72,5 @@
>      public void setUseHardwareLayer(boolean useHardwareLayer) {
>          mUseHardwareLayer = useHardwareLayer;
>      }
>  
> +    public ElementHolder attach(View view, Property property, float to) {

Hmmm, ElementHolder is private and should probably kept it so. Why do you to return it here?

I had sugested something like attach(View, Property, to, pivot). Wouldn't that cover the intended behaviour here?

::: mobile/android/base/animation/Rotate3DAnimation.java
@@ +24,5 @@
>      private final float mCenterX;
>      private final float mCenterY;
>  
> +    private static Camera sCamera = new Camera();
> +    private Matrix mMatrix = new Matrix();

Why did you need to init mMatrix here?

::: mobile/android/base/animation/ViewHelper.java
@@ +60,5 @@
>  		final AnimatorProxy proxy = AnimatorProxy.create(view);
>  		proxy.setHeight(height);
>  	}
> +
> +	public static void setRotationX(View view, float rotation, PointF pivot) {

Just wondering: why is the AnimatorProxy API different than ViewHelper? i.e. why here you set pivot with rotationX and in AnimatorProxy you have separate methods?

Comment 83

[Lucas Rocha (:lucasr)]

Comment on attachment 826193 [details] [diff] [review]
Patch 4/7 - Add a FLIP animation to the HomePager

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

I really like the general direction. Just not sure I agree with the part about exposing AnimatorProxy in the PropertyAnimator API.

::: mobile/android/base/BrowserApp.java
@@ +496,5 @@
>              updateSideBarState();
>          }
>  
>          mFindInPageBar = (FindInPageBar) findViewById(R.id.find_in_page);
> +        mMediumDuration = getResources().getInteger(android.R.integer.config_mediumAnimTime);

The the mediumAnimTime bits are kinda unrelated to the core intent of this patch. I'd put it in a separate patch. No big deal, just a little tip ;-)

@@ +1558,5 @@
>          }
>      }
>  
> +    public void showHomePager() {
> +        showHomePager(HomePager.Page.TOP_SITES);

We now unconditionally show TOP_SITES by default. We can probably remove the arguments to set the initial page. File a follow-up?

@@ +1569,5 @@
> +    public void showHomePager(HomePager.AnimationType type, PropertyAnimator animator) {
> +        showHomePager(HomePager.Page.TOP_SITES, type, animator);
> +    }
> +
> +    public void showHomePager(HomePager.Page page, HomePager.AnimationType type, PropertyAnimator animator) {

type -> animType for clarity?

::: mobile/android/base/animation/AnimatorProxy.java
@@ +17,5 @@
>  
>  import java.lang.ref.WeakReference;
>  import java.util.WeakHashMap;
>  
> +public class AnimatorProxy {

AnimatorProxy is an implementation detail in our little animation framework. I'd prefer to keep it private if possible.

::: mobile/android/base/home/HomePager.java
@@ +132,5 @@
>  
>          super.addView(child, index, params);
>      }
>  
> +    public void show(FragmentManager fm, Page page, AnimationType type, PropertyAnimator animator) {

type -> animType

@@ +137,5 @@
>          mLoaded = true;
>          final TabsAdapter adapter = new TabsAdapter(fm);
>  
>          // Only animate on post-HC devices, when a non-null animator is given
>          final boolean shouldAnimate = (animator != null && Build.VERSION.SDK_INT >= 11);

Update this to account for animType? i.e. && animType == NONE

@@ +147,2 @@
>          adapter.addTab(Page.BOOKMARKS, BookmarksPage.class, new Bundle(),
> +            getContext().getString(R.string.bookmarks_title));

What's this change about?

@@ +164,5 @@
>          setAdapter(adapter);
>  
>          setCurrentItem(adapter.getItemPosition(page), false);
>          setVisibility(VISIBLE);
> +        adapter.setCanLoadHint(!shouldAnimate);

Don't we call this above? Leftover?

@@ +169,2 @@
>  
> +        if (animator == null || Build.VERSION.SDK_INT < 11) {

Simply use !shouldAnimate?

@@ +188,5 @@
> +            case FLIP:
> +                PointF pivot = new PointF(0.5f, 0.5f);
> +                ViewHelper.setRotationX(this, -45.0f, pivot);
> +                ViewHelper.setAlpha(this, 0.0f);
> +                ViewHelper.setTranslationY(this, 0.5f*getHeight());

nit: space before/after *

@@ +194,5 @@
> +                AnimatorProxy proxy = animator.attach(this,
> +                                                      PropertyAnimator.Property.ROTATE_X,
> +                                                      0.0f);
> +                // Force this to rotate around its mid-point
> +                proxy.setPivot(pivot);

Not sure I follow. What do you need to set the pivot explicitly here? This could be part of the attachWithPivot() API that I suggested, no?

Comment 84

[Wesley Johnston (:wesj)]

Ian told me he wants to rethink this. Unassigning until we have new mockups.

Comment 85

[:Margaret Leibovic]

I'm going to close this because yuan closed bug 885363. antlam filed bug 1053390 about creating some new designs for this.