Closed Bug 762727 Opened 9 years ago Closed 9 years ago

Refresh ActionBar on rotate

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox15 verified, firefox16 verified, firefox17 verified)

VERIFIED FIXED
Firefox 16
Tracking Status
firefox15 --- verified
firefox16 --- verified
firefox17 --- verified

People

(Reporter: sriram, Assigned: sriram)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Blocks: 739407
Attached patch WIP (obsolete) — Splinter Review
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.
Assignee: nobody → sriram
Attachment #634194 - Flags: feedback?(mark.finkle)
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.
Attachment #634194 - Attachment is obsolete: true
Attachment #634194 - Flags: feedback?(mark.finkle)
Attachment #634551 - Flags: review?(mark.finkle)
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.
Attachment #634553 - Flags: review?(mark.finkle)
Attachment #634551 - Flags: review?(mark.finkle) → review+
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?
Attachment #634553 - Flags: review?(mark.finkle) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/5d9cd0ad9ea1
https://hg.mozilla.org/mozilla-central/rev/7fa9e53935cb
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
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.
Attachment #634551 - Flags: approval-mozilla-aurora?
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.
Attachment #634553 - Flags: approval-mozilla-aurora?
Attachment #634551 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #634553 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.