Inflate the custom menu's list only when about to be shown

RESOLVED FIXED in Firefox 24

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sriram, Assigned: sriram)

Tracking

unspecified
Firefox 24
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

The current implementation of custom menu inflates the layout and holds the view in memory, ready to be shown.
However, menu is a secondary UI and these views needn't be retained until shown. Hence "optimize" the list view to inflate the MenuItems only when needed.
Assignee

Updated

6 years ago
Assignee: nobody → sriram
Posted patch WIP (obsolete) — Splinter Review
This patch cleans up the custom menu (Oh ya! It's refactor and re-write season after all ;) )

Major changes:
1. ListView's adapter creates views on demand. Earlier we use to create views when we inflate the menu (I know! I know! We shouldn't have done it. But one year back, in crunch mode, I didn't have an option). Now the adapter holds a List of GeckoMenuItems. These are like "data" about the menu items. When it's time to show the menu, the ListView uses the adapter to show the items. Meanwhile, any update happening on the MenuItem, from BrowserApp, is done on the "data" and not on the actual view yet.
2. ListView(Menu) handles the click events of the menu items and forwards it to MenuItem to see if it has any OnMenuItemClickListener. If not, ListView forwards it to any Callback listener (usually activity).
   -- Quick share works a bit differently. If a row has clickable/focusable view in it, ListView wouldn't perform a click event on it. This is a problem for us. Hence, I'm tunneling the click event on the TextView to the ListView.
3. A good approach with styles is to create them as an attribute and define them at theme level. I'm starting that approach with this. (There's actually no big difference between R.style.MenuItemDefault and R.attr.menuItemDefaultStyle, but the latter is a good approach from exposing point of view -- a repacked firefox for a distribution can change these values).


ToDo:
1. onItemChanged() should pass in the MenuItem that was changed to the GeckoMenu. GeckoMenu can take a call to inform the ActionBarPresenter or update its own adapter based on the kind of MenuItem (normal vs action-item).
2. Currently action-bar-items aren't updated with changes in this patch. Only the ones catered by the adapter are. That needs to be fixed.
(Basically action-item update should alone be fixed).
Attachment #758384 - Flags: feedback?(wjohnston)
Attachment #758384 - Flags: feedback?(mark.finkle)
The GeckoMenuItem.Layout interface is not needed anymore. We don't use "generic" Layout to update them like earlier (Note: This was needed for doing updates like setTitle() as we used to hold the views). I am planning on killing it in the actual patch -- but a part of me tells me to have it, as it tells "what a view that is backed by MenuItem should support".
I am open for suggestions.
Posted patch PatchSplinter Review
(Out of laundry and soooo cleaaan!)
Attachment #758384 - Attachment is obsolete: true
Attachment #758384 - Flags: feedback?(wjohnston)
Attachment #758384 - Flags: feedback?(mark.finkle)
Attachment #758847 - Flags: review?(wjohnston)
Attachment #758847 - Flags: review?(mark.finkle)
Comment on attachment 758847 [details] [diff] [review]
Patch

General plan seems fine. I didn't notice anything too scary, but I want a Try server run.
Attachment #758847 - Flags: review?(mark.finkle) → review+
There's an NPE! :O
https://tbpl.mozilla.org/?tree=Try&rev=ad1b4a345e58

Will post additional patches over this one.
Posted patch Patch: TestSplinter Review
My mistake! Last time I changed testShareLink, I copied code from above. They don't pass in the ListView as the "parent" argument while getting a view through getView(). My cleanup used context from this "parent" -- which was now null. Fixed it now. While at it, fixed the extra unnecessary argument "context" for the MenuItemsAdapter.
Attachment #759822 - Flags: review?(mark.finkle)
Try passed with the test patch: https://tbpl.mozilla.org/?tree=Try&rev=8fae6e1fb85e
(Hit parent to look at the parent. Try server didn't give me old link).
Attachment #759822 - Flags: review?(mark.finkle) → review+
Comment on attachment 758847 [details] [diff] [review]
Patch

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

Looks like good cleanup to me.

::: mobile/android/base/menu/GeckoMenuItem.java
@@ +31,4 @@
>      private int mId;
>      private int mOrder;
> +    private View mActionView;
> +    private boolean mActionItem = false;

false is default right. Why are we initializaing some things here but not others?

@@ +291,5 @@
> +            MenuItemActionBar actionView = new MenuItemActionBar(mMenu.getContext(), null);
> +            actionView.initialize(this);
> +            mActionView = actionView;
> +
> +            mActionItem = (actionEnum > 0);         

trailing whitespace

::: mobile/android/base/menu/MenuItemActionView.java
@@ +51,5 @@
> +    public void initialize(GeckoMenuItem item) {
> +        if (item == null)
> +            return;
> +
> +        setTitle(item.getTitle());        

whitespace
Attachment #758847 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #9)
> Comment on attachment 758847 [details] [diff] [review]
> Patch
> 
> Review of attachment 758847 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like good cleanup to me.
> 
> ::: mobile/android/base/menu/GeckoMenuItem.java
> @@ +31,4 @@
> >      private int mId;
> >      private int mOrder;
> > +    private View mActionView;
> > +    private boolean mActionItem = false;
> 
> false is default right. Why are we initializaing some things here but not
> others?

I just wanted to initialize the boolean items so that by reading the code we can find the default values. Other objects are null by default.
https://hg.mozilla.org/mozilla-central/rev/98539fbbbf39
https://hg.mozilla.org/mozilla-central/rev/9619f1a5d3c2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
You need to log in before you can comment on or make changes to this bug.