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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sriram Ramasubramanian [:sriram]]

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.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP

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).

Comment 2

[Sriram Ramasubramanian [:sriram]]

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.

Comment 3

[Sriram Ramasubramanian [:sriram]]

I could use View.isEnabled() here: https://bugzilla.mozilla.org/attachment.cgi?id=758384&action=diff#a/mobile/android/base/menu/MenuItemDefault.java_sec2
#very-late-realization.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

(Out of laundry and soooo cleaaan!)

Comment 5

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

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.

Comment 6

[Sriram Ramasubramanian [:sriram]]

There's an NPE! :O
https://tbpl.mozilla.org/?tree=Try&rev=ad1b4a345e58

Will post additional patches over this one.

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch: Test

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.

Comment 8

[Sriram Ramasubramanian [:sriram]]

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).

Comment 9

[Wesley Johnston (:wesj)]

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

Comment 10

[Sriram Ramasubramanian [:sriram]]

(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.

Comment 11

[Sriram Ramasubramanian [:sriram]]

Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98539fbbbf39
https://hg.mozilla.org/integration/mozilla-inbound/rev/9619f1a5d3c2

Green on try: https://tbpl.mozilla.org/?tree=Try&rev=8fae6e1fb85e

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/98539fbbbf39
https://hg.mozilla.org/mozilla-central/rev/9619f1a5d3c2