762727 - Refresh ActionBar on rotate

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sriram Ramasubramanian [:sriram]]

To accommodate the new design of sliding url-br, the action-bar had to be killed (as the action-bar and the content portion has two different parents).

On rotation, the action-bar should use smaller sizes like before.

Note: If we don't want resources on 40dp on rotation, this is a good chance to kill it and leve both portrait and landscape have same size.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP

With the new tabs UI, we don't use ActionBar anymore. Hence BrowserToolbar is just a part of the entire "content" portion of the app.

While rotating, the new layout isn't automatically loaded as we override onConfigurationChanged() to avoid the activity from getting killed. However, android doesn't update the view with newer resources and layout size unless we remove-inflate-attach the BrowserToolbar again. I tried using refreshLayout(), forceLayout(), invalidate(), refreshDrawableState() on every possible view. This doesn't seem to work.

Looking at some android code, they too remove and re-attach ActionBar views on rotation.

If this is the approach to go, and if we are sure that we won't end up getting ActionBar again, we can use "org.mozilla.gecko.BrowserToolbar". The layout inflation will take care of the height and doing the "from()" part.

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (1/2): BrowserToolbar

This patch removes, inflates a new and re-attaches the BrowserToolbar to the view.
I tried changing BrowserToolbar to be the layout. It worked fine, but was super disruptive with custom menu. Custom menu needs to know who can handle action bar items. Earlier mBrowserToolbar variable never changed -- only its member values did. Now on rotation, if mBrowserToolbar changes -- as it's the layout -- the menu gets confused, and we need to inform menu again, recreate menu on rotation. This is an expensive task. Hence this patch preserves the old "mBrowserToolbar.from(actionBar)", and adds the actionBar on rotation. This works the same way we did with Android's actionbar.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (2/2): TabsPanel

This patch removes, re-creates, attach a toolbar for TabsPanel. The toolbar works in the same fashion as BrowserToolbar.

One hack: TabsListContainer's forceLayout() call doesn't redraw the layout before we call show(). There is a minor lag, making the old size propagate to BrowserApp for animation. To avoid this, the height is taken as 50% and then dispatched to the BrowserApp. By the time the rotation is complete, the list comes up, the TabsListContainer has the new layout based on the height of the device in that orientation.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 634553 [details] [diff] [review]
Patch (2/2): TabsPanel


>     public void show(Panel panel) {

>+
>+            // TabsListContainer takes time to resize on rotation.
>+            // It's better to add 50% of the screen-size and dispatch it as height.
>+            DisplayMetrics metrics = new DisplayMetrics();
>+            GeckoApp.mAppContext.getWindowManager().getDefaultDisplay().getMetrics(metrics);
>+            int listHeight = (int) (0.5 * metrics.heightPixels);
>+
>+            int height = actionBarHeight + listHeight; 

This 50% trick seems kinda hacky to me. Can you tell me more about why we need this?

Comment 5

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Comment on attachment 634553 [details] [diff] [review]
> Patch (2/2): TabsPanel
> 
> 
> >     public void show(Panel panel) {
> 
> >+
> >+            // TabsListContainer takes time to resize on rotation.
> >+            // It's better to add 50% of the screen-size and dispatch it as height.
> >+            DisplayMetrics metrics = new DisplayMetrics();
> >+            GeckoApp.mAppContext.getWindowManager().getDefaultDisplay().getMetrics(metrics);
> >+            int listHeight = (int) (0.5 * metrics.heightPixels);
> >+
> >+            int height = actionBarHeight + listHeight; 
> 
> This 50% trick seems kinda hacky to me. Can you tell me more about why we
> need this?

On rotation, we do a mTabsPanel.refresh(). In refresh(), we call mListContainer.forceLayout(). This basically does a layout pass, to resize the ListContainer to 50% of the screensize (based on orientation of the device).

Though all the logic is fine.. Android has it own timing to do layout pass -- which we cannot control. As per our implemtation, we do a forceLayout(), then re-attach the toolbar, and then call show() to populate the list. At show(), we want to get the height of the "resized TabsListContainer". However, the resize is not done yet. So, we may get a portrait height on landscape and vice versa. This is why, the size we calculate there is the "intended size" of the TabsListContainer -- which is going to happen in the nearest future -- like 100ms.

Comment 6

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5d9cd0ad9ea1
http://hg.mozilla.org/integration/mozilla-inbound/rev/7fa9e53935cb

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5d9cd0ad9ea1
https://hg.mozilla.org/mozilla-central/rev/7fa9e53935cb

Comment 8

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 634551 [details] [diff] [review]
Patch (1/2): BrowserToolbar

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined:
The URL bar will not resize to landscape size on Galaxy Nexus (and other ICS phones).
Testing completed (on m-c, etc.): Landed on m-c on 06/20
Risk to taking this patch (and alternatives if risky): No risk. This is required to have the action-bar properties.
String or UUID changes made by this patch: None.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 634553 [details] [diff] [review]
Patch (2/2): TabsPanel

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined:
1. The tabs-ui's action-bar will not resize for landscape mode.
2. URL bar will be lost when device is rotated from portrait to landscape with tabs-ui open.
Testing completed (on m-c, etc.): Landed on m-c on 06/20
Risk to taking this patch (and alternatives if risky): No risk. This is required to have the action-bar properties.
String or UUID changes made by this patch: None.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Built and tested on a phone, 7" GB tablet and a honeycomb 10" tablet

https://hg.mozilla.org/releases/mozilla-aurora/rev/441868d931ef
https://hg.mozilla.org/releases/mozilla-aurora/rev/f659b07077c4