Closed Bug 871651 Opened 9 years ago Closed 8 years ago

Janky animation to enter editing mode

Categories

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

All
Android
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

(Whiteboard: [fixed-fig])

Attachments

(6 files)

This is because we're inflating about:home during the animation. We should probably add hooks to only start the animation once the target homepager page is inflated.
Assignee: nobody → lucasr.at.mozilla
Priority: -- → P1
This is going to covered when fixing bug 889621. Closing this one as dup.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 889621
Re-opening. I've just implemented the new animation in bug 889621. Even though it's better than the original one in fig, it still drops quite a few frames. Needs more tuning.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Depends on: 904840
Comment on attachment 790722 [details] [diff] [review]
(1/6) Avoid starting animations during or just before a layout round

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

LGTM. Please add the braces.

::: mobile/android/base/animation/PropertyAnimator.java
@@ +152,5 @@
>  
> +        // Get ViewTreeObserver from any of the participant views
> +        // in the animation.
> +        final ViewTreeObserver treeObserver;
> +        if (mElementsList.size() > 0)

{

@@ +154,5 @@
> +        // in the animation.
> +        final ViewTreeObserver treeObserver;
> +        if (mElementsList.size() > 0)
> +            treeObserver = mElementsList.get(0).view.getViewTreeObserver();
> +        else

} .. {

@@ +155,5 @@
> +        final ViewTreeObserver treeObserver;
> +        if (mElementsList.size() > 0)
> +            treeObserver = mElementsList.get(0).view.getViewTreeObserver();
> +        else
> +            treeObserver = null;

}
Attachment #790722 - Flags: review?(sriram) → review+
Comment on attachment 790723 [details] [diff] [review]
(2/6) Use hardware layers to animate about:home

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

::: mobile/android/base/home/HomePager.java
@@ +138,5 @@
> +                public void onPropertyAnimationEnd() {
> +                    setLayerType(View.LAYER_TYPE_NONE, null);
> +                }
> +            });
> +

This is all fine. But I have this question. Why do we need to reset the layer type? If we have hardware layer enabled at Application level -- which will descend to activity -- to its views (by inheriting), why not we always have hardware layers everywhere?
Attachment #790723 - Flags: review?(sriram) → review+
Attachment #790724 - Flags: review?(sriram) → review+
Attachment #790726 - Flags: review?(sriram) → review+
Attachment #790727 - Flags: review?(sriram) → review+
Comment on attachment 790725 [details] [diff] [review]
(4/6) Only load pages after editing mode animation ends

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

::: mobile/android/base/home/HomePager.java
@@ +273,5 @@
> +            }
> +
> +            // Enable/disable loading on all existing pages
> +            for (Fragment page : mPages.values()) {
> +                HomeFragment homePage = (HomeFragment) page;

final.

Infact, its a one liner. Please make it " ((HomeFragment) page).seCanLoadHint(canLoadHint);
Attachment #790725 - Flags: review?(sriram) → review+
(In reply to Sriram Ramasubramanian [:sriram] from comment #9)
> Comment on attachment 790722 [details] [diff] [review]
> (1/6) Avoid starting animations during or just before a layout round
> 
> Review of attachment 790722 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM. Please add the braces.
> 
> ::: mobile/android/base/animation/PropertyAnimator.java
> @@ +152,5 @@
> >  
> > +        // Get ViewTreeObserver from any of the participant views
> > +        // in the animation.
> > +        final ViewTreeObserver treeObserver;
> > +        if (mElementsList.size() > 0)
> 
> {
> 
> @@ +154,5 @@
> > +        // in the animation.
> > +        final ViewTreeObserver treeObserver;
> > +        if (mElementsList.size() > 0)
> > +            treeObserver = mElementsList.get(0).view.getViewTreeObserver();
> > +        else
> 
> } .. {
> 
> @@ +155,5 @@
> > +        final ViewTreeObserver treeObserver;
> > +        if (mElementsList.size() > 0)
> > +            treeObserver = mElementsList.get(0).view.getViewTreeObserver();
> > +        else
> > +            treeObserver = null;
> 
> }

Done.
(In reply to Sriram Ramasubramanian [:sriram] from comment #10)
> Comment on attachment 790723 [details] [diff] [review]
> (2/6) Use hardware layers to animate about:home
> 
> Review of attachment 790723 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomePager.java
> @@ +138,5 @@
> > +                public void onPropertyAnimationEnd() {
> > +                    setLayerType(View.LAYER_TYPE_NONE, null);
> > +                }
> > +            });
> > +
> 
> This is all fine. But I have this question. Why do we need to reset the
> layer type? If we have hardware layer enabled at Application level -- which
> will descend to activity -- to its views (by inheriting), why not we always
> have hardware layers everywhere?

What we have enabled for the entire application is hw acceleration (not layers). Hw acceleration defines the underlying drawing model for the UI. When you enable hw acceleration in your app (which is on by default since API >= 14) all views will use the DisplayList-based drawing model.

Layers, on the other hand, are about "flattening" drawing operations. A hw layer will paint the view into an off-screen GL texture and use that texture for drawing the view until the layer is destroyed. A sw layer will paint the view into an off-screen bitmap instead of a GL texture. Layers are meant to be used when you want your view's drawing to be very efficient. Animations are the main use case but they also used for smoother scrolling e.g. in ListView and GridView.

Hw layers take extra GPU memory space (for the GL texture). Sw layers take extra RAM memory (for the cached bitmap). This is why it's a good idea to destroy the hw/sw layer once they are not needed anymore.
(In reply to Sriram Ramasubramanian [:sriram] from comment #11)
> Comment on attachment 790725 [details] [diff] [review]
> (4/6) Only load pages after editing mode animation ends
> 
> Review of attachment 790725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomePager.java
> @@ +273,5 @@
> > +            }
> > +
> > +            // Enable/disable loading on all existing pages
> > +            for (Fragment page : mPages.values()) {
> > +                HomeFragment homePage = (HomeFragment) page;
> 
> final.
> 
> Infact, its a one liner. Please make it " ((HomeFragment)
> page).seCanLoadHint(canLoadHint);

Added the final. I really dislike these embedded castings. They don't make code that much shorter and much harder to read.
Depends on: 1001242
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.