869123 - Quick Share from Android menu

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: Mockup

Let's make sharing a little easier for our users by reserving a spot in the menu for their most commonly used sharing service. A mockup attached. It's super simple. 

By default we just show the "Share" row in the menu. 

Once you share something with a service, we pop that service's icon into the right side of the share menu row. The icon becomes a shortcut to use that sharing service. 

Whichever sharing service you use the most gets placed into that spot. 

This bug is not about managing what goes there, or deleting it, or anything like that -- those can be explored in follow-ups. It's just about putting your most commonly used sharing service one tap closer to you.

Comment 1

[Madhava Enros [:madhava]]

This is going to be a clever way to explore some saving-for-later taskflows - i.e. "sharing" to Evernote or Pocket.

Raises a question (not for v1!) of how do (a) adding to bookmarks, (b) saving to reading list, and (c) sharing to elsewhere interrelate.

Comment 2

[:Margaret Leibovic]

Sriram, does this depend on bug 825799?

Comment 3

[Sriram Ramasubramanian [:sriram]]

Duplicate of 869123

Comment 4

[Wesley Johnston (:wesj)]

That's this bug (so I guess its technically true?)

Comment 5

[Sriram Ramasubramanian [:sriram]]

(In reply to :Margaret Leibovic from comment #2)
> Sriram, does this depend on bug 825799?

Bug 825799 can be made as a duplicate of this.

Comment 6

[Caspy7]

Perhaps it's already planned, but I didn't see it stated, I would hope this uses frecency rather than simple frequency.  If I've been sharing to service A for a while, then it insults my honor and I don't like it anymore, I would want the new alternate (service B) to eclipse it after several days of use.
If I've been using service A for several months or a year, trying to overcome it - especially if I'm now using two alternates - would be frustrating (constantly reminding me what it said about my mother).

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 1: MenuItemDefault

It's better to set the properties inside MenuItemDefault, than doing in XML and "inflating" it. This makes MenuItemDefault re-usable.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 2: Menu package

This adds a (brand new) "menu" package.
Most things related to menu are moved to this package.
 - MenuItemDefault
 - MenuItemActionBar
 - MenuPanel
 - MenuPopup
 - GeckoMenu
 - GeckoMenuItem
 - GeckoSubMenu
 - GeckoMenuInflater

A "Divider" was stranded, and I moved it to "widget" package.

Comment 9

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 3: Quick Share button

This adds a quick share button. This is available on 14+, as ActionProviders are available only there. Going with android convention, I've added ActivityChooserModel and GeckoShareActionProvider to "widget" directory. The provider is added to MenuItem, which takes the responsibility of adding the icon.

Note: ActivityChooserModel had to be copied from Android. A few changes were made, marked with "Mozilla" comments. I'm not sure about these licenses. And I haven't added our license boilerplate there.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Unfortunately I couldn't store the share_history.xml inside the GeckoProfile. Android throws and exception if the file is specified with a path.

E/GeckoAppShell( 8521): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
E/GeckoAppShell( 8521): java.lang.IllegalArgumentException: File /data/data/org.mozilla.fennec_sriramramasubramanian/files/mozilla/itwyrnpo.default/share_history.xml contains a path separator
E/GeckoAppShell( 8521): 	at android.app.ContextImpl.makeFilename(ContextImpl.java:1966)
E/GeckoAppShell( 8521): 	at android.app.ContextImpl.openFileInput(ContextImpl.java:692)
E/GeckoAppShell( 8521): 	at android.content.ContextWrapper.openFileInput(ContextWrapper.java:167)
E/GeckoAppShell( 8521): 	at org.mozilla.gecko.widget.ActivityChooserModel.readHistoricalDataImpl(ActivityChooserModel.java:996)
E/GeckoAppShell( 8521): 	at org.mozilla.gecko.widget.ActivityChooserModel.readHistoricalDataIfNeeded(ActivityChooserModel.java:748)
E/GeckoAppShell( 8521): 	at org.mozilla.gecko.widget.ActivityChooserModel.ensureConsistentState(ActivityChooserModel.java:689)
E/GeckoAppShell( 8521): 	at org.mozilla.gecko.widget.ActivityChooserModel.getDefaultActivity(ActivityChooserModel.java:537)
E/GeckoAppShell( 8521): 	at org.mozilla.gecko.widget.GeckoShareActionProvider.onCreateActionView(GeckoShareActionProvider.java:57)

Comment 11

[Wesley Johnston (:wesj)]

Comment on attachment 749967 [details] [diff] [review]
Part 1: MenuItemDefault

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

::: mobile/android/base/MenuItemDefault.java
@@ +28,5 @@
>      private boolean mChecked = false;
>      private boolean mHasSubMenu = false;
>  
>      public MenuItemDefault(Context context, AttributeSet attrs) {
> +        super(context, attrs, R.style.MenuItemDefault);

It would be kinda nice to have a MenuItemDefault(context, attrs) instead call

MenuItemDefault(context, attrs, R.style.MenuItemDefault).

which could then pass the value on the super class. i.e. then we could at some point fix this up so that you could override the defaults below?

@@ +38,5 @@
> +        setMinimumHeight(height);
> +        setGravity(Gravity.CENTER_VERTICAL);
> +
> +        float density = res.getDisplayMetrics().density;
> +        int padding = (int) (10 * density);

Why isn't this in R.dimen

@@ +41,5 @@
> +        float density = res.getDisplayMetrics().density;
> +        int padding = (int) (10 * density);
> +        setPadding(padding, 0, padding, 0);
> +
> +        int drawablePadding = (int) (6 * density);

Or this?

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 749968 [details] [diff] [review]
Part 2: Menu package

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

::: mobile/android/base/menu/MenuPanel.java
@@ +32,5 @@
>      protected void onMeasure(int widthMeasureSpec, int heightMeasureSpec) {
>          super.onMeasure(widthMeasureSpec, heightMeasureSpec);
>  
>          // Restrict the height to 75% of the screen-height. heightPixels changes during rotation.
> +        DisplayMetrics metrics = getContext().getResources().getDisplayMetrics();

Yay!

Comment 13

[Wesley Johnston (:wesj)]

Comment on attachment 749973 [details] [diff] [review]
Part 3: Quick Share button

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

I hate bringing in this much code for something so simple, but overall it looks fine and I haven't found any way around it either. I do think we can simplify in some of these cases and limit the number of GeckoMenuItem, MenuItem, MenuItemDefault, etc. classes we're juggling. Not sure if you guys have looked into and rejected those ideas before or not, so holding my r+ to get your feedback on my feedback.

::: mobile/android/base/BrowserApp.java
@@ +1291,5 @@
> +                @Override
> +                public void onShareTargetSelected() {
> +                    closeOptionsMenu();
> +                }
> +            });

Instinctively, I think I expect this listener to be added to the view, not the provider. i.e. something like

MenuItemActionView item = (MenuItemActionView) mMenu;
item.setOnActionClickListener(View.OnClickListener() { });
item.setActionProvider(new GeckoShareActionProvider(this));

I wonder if we can remove that middle line entirely though and just have the menuitem automatically do something like:

if (menuitem.getParent() instanceof Menu) {
  menuitem.getParent().close();
}

@@ +1393,5 @@
> +                Intent shareIntent = new Intent(Intent.ACTION_SEND);
> +                shareIntent.putExtra(Intent.EXTRA_TEXT, url);
> +                shareIntent.putExtra(Intent.EXTRA_SUBJECT, tab.getDisplayTitle());
> +                shareIntent.setType("text/plain");
> +                provider.setShareIntent(shareIntent);

Use http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java#1061 instead here.

::: mobile/android/base/menu/GeckoMenuItem.java
@@ +154,5 @@
>      }
>  
>      public View getLayout() {
> +        if (mActionProvider != null)
> +            return getActionView();

This feels a bit like cheating... We're not really asking for the actionView here. We're asking for... a custom view?

@@ +155,5 @@
>  
>      public View getLayout() {
> +        if (mActionProvider != null)
> +            return getActionView();
> +        else

don't need this else.

::: mobile/android/base/menu/MenuItemActionView.java
@@ +16,5 @@
> +import android.view.View;
> +import android.widget.LinearLayout;
> +import android.widget.ImageButton;
> +
> +public class MenuItemActionView extends LinearLayout

Its a bit confusing to keep track of the differences/similarities between MenuItemDefault, MenuItemActionBar, and MenuItemActionView (none of which inherit from one another, although MenuItemActionView contains a MenuItemDefault inside it...). I'm almost more in favor of just moving this into MenuItemDefault, but hiding the actionButton in most cases? That means some extra, unnecessary views, but maybe its worth it?

@@ +21,5 @@
> +                                implements GeckoMenuItem.Layout {
> +    private static final String LOGTAG = "GeckoMenuItemActionView";
> +
> +    private MenuItemDefault mMenuItem;
> +    private ImageButton mDefaultActivity;

I think we could make this more generally. mActionButton maybe? Throughout as well, for instance the listener.

::: mobile/android/base/resources/layout/menu_item_action_view.xml
@@ +14,5 @@
> +    <View android:layout_width="1dip"
> +          android:layout_height="fill_parent"
> +          android:layout_marginTop="8dip"
> +          android:layout_marginBottom="8dip"
> +          android:background="#FFD1D5DA"/>

I'm not a huge fan of this hacky divider. Ideally we could add it to the linear layout. i.e. I used:

        <item name="android:showDividers">middle</item>
        <item name="android:divider">@drawable/toast_divider</item>
        <item name="android:dividerPadding">16dp</item>

for the super toast dividers...

::: mobile/android/base/widget/GeckoShareActionProvider.java
@@ +18,5 @@
> +import android.view.SubMenu;
> +import android.view.View;
> +import android.view.View.OnClickListener;
> +
> +public class GeckoShareActionProvider extends ActionProvider {

I think I would be much less out-of-love with this class if we just removed the word 'Share' everywhere inside it. It doesn't seem to know much about sharing anyway. Just "Get me activities/defaults for this intent".

Comment 14

[Sriram Ramasubramanian [:sriram]]

(In reply to Wesley Johnston (:wesj) from comment #13)
> Comment on attachment 749973 [details] [diff] [review]
> Part 3: Quick Share button
> 
> Review of attachment 749973 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I hate bringing in this much code for something so simple,

I know, but, I didn't have a choice.

> but overall it
> looks fine and I haven't found any way around it either. I do think we can
> simplify in some of these cases and limit the number of GeckoMenuItem,
> MenuItem, MenuItemDefault, etc. classes we're juggling. Not sure if you guys
> have looked into and rejected those ideas before or not, so holding my r+ to
> get your feedback on my feedback.
> 
> ::: mobile/android/base/BrowserApp.java
> @@ +1291,5 @@
> > +                @Override
> > +                public void onShareTargetSelected() {
> > +                    closeOptionsMenu();
> > +                }
> > +            });
> 
> Instinctively, I think I expect this listener to be added to the view, not
> the provider. i.e. something like
> 
> MenuItemActionView item = (MenuItemActionView) mMenu;
> item.setOnActionClickListener(View.OnClickListener() { });
> item.setActionProvider(new GeckoShareActionProvider(this));
> 
> I wonder if we can remove that middle line entirely though and just have the
> menuitem automatically do something like:
> 
> if (menuitem.getParent() instanceof Menu) {
>   menuitem.getParent().close();
> }

I don't understand where this piece of code go will in. The problem is, GeckoShareActionProvider doesn't know about the menu. It's pretty generic. So, when we return a MenuItemActionView in onCreateActionView(), that item doesn't have any clue about parent menu, and a way to close it. MenuItem doesn't have a "getParent()" with it. Hence, I had to do it as a cleanup, when a share target it selected. I used that name to be analogous with the ShareActionProvider.

> 
> @@ +1393,5 @@
> > +                Intent shareIntent = new Intent(Intent.ACTION_SEND);
> > +                shareIntent.putExtra(Intent.EXTRA_TEXT, url);
> > +                shareIntent.putExtra(Intent.EXTRA_SUBJECT, tab.getDisplayTitle());
> > +                shareIntent.setType("text/plain");
> > +                provider.setShareIntent(shareIntent);
> 
> Use
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> GeckoAppShell.java#1061 instead here.
> 

I wanted to use it. But, it wraps the intent with a "createChooser()" call, which we don't want. Hence had to copy-paste code here for the special case. createChooser() gives a list-view window.

> ::: mobile/android/base/menu/GeckoMenuItem.java
> @@ +154,5 @@
> >      }
> >  
> >      public View getLayout() {
> > +        if (mActionProvider != null)
> > +            return getActionView();
> 
> This feels a bit like cheating... We're not really asking for the actionView
> here. We're asking for... a custom view?
> 

This is because I made GeckoShareActionProvider extends ActionProvider, so that it follows MenuItem's norms. Everything's pretty generic. So, later, if we want another action-provider (say Reading List), we can create a GeckoReadingListActionProvider, and still things will work. "getActionView()" is an abstract method in ActionProvider.

> @@ +155,5 @@
> >  
> >      public View getLayout() {
> > +        if (mActionProvider != null)
> > +            return getActionView();
> > +        else
> 
> don't need this else.

A bit of old school in me. :)

> 
> ::: mobile/android/base/menu/MenuItemActionView.java
> @@ +16,5 @@
> > +import android.view.View;
> > +import android.widget.LinearLayout;
> > +import android.widget.ImageButton;
> > +
> > +public class MenuItemActionView extends LinearLayout
> 
> Its a bit confusing to keep track of the differences/similarities between
> MenuItemDefault, MenuItemActionBar, and MenuItemActionView (none of which
> inherit from one another, although MenuItemActionView contains a
> MenuItemDefault inside it...). I'm almost more in favor of just moving this
> into MenuItemDefault, but hiding the actionButton in most cases? That means
> some extra, unnecessary views, but maybe its worth it?

It's not worth adding an ImageView in the layout when it's not needed. We brought down the number of views from 7 to 1 (https://sriramramani.wordpress.com/2013/03/25/view-reduction/) by using compound drawables. Now, just for a Share menu item, if we go ahead and make every row have 4 views, that wouldn't be economical.

Also, I had plans to rename to MenuItemActionBar to a MenuItemIcon. That could be a follow up to make things clear. Though they also are different forms, they all implement GeckoMenuItem.Layout, and they form "a kind" there.

> 
> @@ +21,5 @@
> > +                                implements GeckoMenuItem.Layout {
> > +    private static final String LOGTAG = "GeckoMenuItemActionView";
> > +
> > +    private MenuItemDefault mMenuItem;
> > +    private ImageButton mDefaultActivity;
> 
> I think we could make this more generally. mActionButton maybe? Throughout
> as well, for instance the listener.
> 

I thought of it. But then, since we are showing a default activity there, I thought of naming it mDefaultActivity. I am open to anything.

> ::: mobile/android/base/resources/layout/menu_item_action_view.xml
> @@ +14,5 @@
> > +    <View android:layout_width="1dip"
> > +          android:layout_height="fill_parent"
> > +          android:layout_marginTop="8dip"
> > +          android:layout_marginBottom="8dip"
> > +          android:background="#FFD1D5DA"/>
> 
> I'm not a huge fan of this hacky divider. Ideally we could add it to the
> linear layout. i.e. I used:
> 
>         <item name="android:showDividers">middle</item>
>         <item name="android:divider">@drawable/toast_divider</item>
>         <item name="android:dividerPadding">16dp</item>
> 
> for the super toast dividers...
> 

I know we can use this. However it's available only 11+. It might not be a problem for this custom view. But it might be a problem for your SuperToastView. :)
I can change it. (Actually was a bit lazy to create a divider as android:divider won't accept a color value ;) Also wanted to refer it to ?android:attr/dividerVertical -- but that's a separate patch by itself.

> ::: mobile/android/base/widget/GeckoShareActionProvider.java
> @@ +18,5 @@
> > +import android.view.SubMenu;
> > +import android.view.View;
> > +import android.view.View.OnClickListener;
> > +
> > +public class GeckoShareActionProvider extends ActionProvider {
> 
> I think I would be much less out-of-love with this class if we just removed
> the word 'Share' everywhere inside it. It doesn't seem to know much about
> sharing anyway. Just "Get me activities/defaults for this intent".

I'm fine with any name.

Comment 15

[Sriram Ramasubramanian [:sriram]]

(In reply to Wesley Johnston (:wesj) from comment #11)
> Comment on attachment 749967 [details] [diff] [review]
> Part 1: MenuItemDefault
> 
> Review of attachment 749967 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/MenuItemDefault.java
> @@ +28,5 @@
> >      private boolean mChecked = false;
> >      private boolean mHasSubMenu = false;
> >  
> >      public MenuItemDefault(Context context, AttributeSet attrs) {
> > +        super(context, attrs, R.style.MenuItemDefault);
> 
> It would be kinda nice to have a MenuItemDefault(context, attrs) instead call
> 
> MenuItemDefault(context, attrs, R.style.MenuItemDefault).
> 
> which could then pass the value on the super class. i.e. then we could at
> some point fix this up so that you could override the defaults below?

I can do that, as we have a version of View(context, attrs, defStyle).

> 
> @@ +38,5 @@
> > +        setMinimumHeight(height);
> > +        setGravity(Gravity.CENTER_VERTICAL);
> > +
> > +        float density = res.getDisplayMetrics().density;
> > +        int padding = (int) (10 * density);
> 
> Why isn't this in R.dimen
> 
> @@ +41,5 @@
> > +        float density = res.getDisplayMetrics().density;
> > +        int padding = (int) (10 * density);
> > +        setPadding(padding, 0, padding, 0);
> > +
> > +        int drawablePadding = (int) (6 * density);
> 
> Or this?

I was planning on moving there. Then I thought of exposing it as an attribute and use a "?attr/menuItemDefaultStyle" with the theme. But then, we don't reuse this anywhere. And we don't set different values. In which case, both attributes and R.dimen are expensive in that they have to be resolved. So, it's better to have a final value directly mentioned in code. I can make this final if that's fine. Anyways, specifying a direct value is better.

Comment 16

[Wesley Johnston (:wesj)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #14)
> I don't understand where this piece of code go will in. The problem is,
> GeckoShareActionProvider doesn't know about the menu. It's pretty generic.
> So, when we return a MenuItemActionView in onCreateActionView(), that item
> doesn't have any clue about parent menu, and a way to close it. MenuItem
> doesn't have a "getParent()" with it. Hence, I had to do it as a cleanup,
> when a share target it selected. I used that name to be analogous with the
> ShareActionProvider.

You're right. Android attaches the click listener to the owner AbsListView which then forwards the click to the menu, which closes itself. That won't work here, since the Adapter isn't aware enough of the button to do click targetting on it.

Not exactly sure what the answer is, but I don't like having this code in here... I think the menu probably needs to add something to the MenuItem when it creates it (which can then forward it or call it when the button is clicked) that can close the menu. i.e. BrowserToolbar shouldn't have to do this...

> I wanted to use it. But, it wraps the intent with a "createChooser()" call, which we 
> don't want. Hence had to copy-paste code here for the special case. createChooser() 
> gives a list-view window.

Split it up and then you can use it!

> This is because I made GeckoShareActionProvider extends ActionProvider, so that it 
> follows MenuItem's norms. Everything's pretty generic. So, later, if we want another 
> action-provider (say Reading List), we can create a GeckoReadingListActionProvider, and
> still things will work. "getActionView()" is an abstract method in ActionProvider.

Yeah. Well ActionProvider.getActionView isn't really designed for this either right? Its designed to be used when the item is being shown on the ActionBar vs. on the Menu. i.e. we're not following MenuItem norms at all. TBH, it seems like a better way to do this would be to inflate a different view here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoMenuItem.java#64

which would mean GeckoMenuInflater/GeckoMenu would need to somehow know this was a different type of item. Then BrowserApp wouldn't have to know anything about this... What do you think about doing that?

> It's not worth adding an ImageView in the layout when it's not needed. We
> brought down the number of views from 7 to 1

I agree.

> I thought of it. But then, since we are showing a default activity there, I
> thought of naming it mDefaultActivity. I am open to anything.

Yeah. As far as MenuItemActionView cares, this is just a button (i.e. it doesn't know or care what happens when you hit it), so I'd rather call it something generic.

> I know we can use this. However it's available only 11+. It might not be a
> problem for this custom view. But it might be a problem for your
> SuperToastView. :)

Heh. I should check. But if we can use 'em here, we should (maybe with a comment...)

> I'm fine with any name.

Thanks.

Comment 17

[Sriram Ramasubramanian [:sriram]]

(In reply to Wesley Johnston (:wesj) from comment #16)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #14)
> > I don't understand where this piece of code go will in. The problem is,
> > GeckoShareActionProvider doesn't know about the menu. It's pretty generic.
> > So, when we return a MenuItemActionView in onCreateActionView(), that item
> > doesn't have any clue about parent menu, and a way to close it. MenuItem
> > doesn't have a "getParent()" with it. Hence, I had to do it as a cleanup,
> > when a share target it selected. I used that name to be analogous with the
> > ShareActionProvider.
> 
> You're right. Android attaches the click listener to the owner AbsListView
> which then forwards the click to the menu, which closes itself. That won't
> work here, since the Adapter isn't aware enough of the button to do click
> targetting on it.
> 
> Not exactly sure what the answer is, but I don't like having this code in
> here... I think the menu probably needs to add something to the MenuItem
> when it creates it (which can then forward it or call it when the button is
> clicked) that can close the menu. i.e. BrowserToolbar shouldn't have to do
> this...

http://developer.android.com/reference/android/app/Activity.html#closeOptionsMenu%28%29
closeOptionsMenu() is available at Activity level. We are just using that, in case of the provider. Menu wouldn't be knowing about the event click, and hence cannot do anything about it.

The code related to BrowserToolbar is there because, the custom menu's menu button is owned by BrowserToolbar. That's why we pass the event to BrowserToolbar to do any cleanup. In general cases (including the h/w menu button case), this goes through standard Android's menu based cleanup.

http://hg.mozilla.org/mozilla-central/file/tip/mobile/android/base/BrowserToolbar.java#l1190
BrowserToolbar also owns the PopupWindow. So, this feels a right approach to me. You are looking as we attaching an event to the provider. But essentially, provider exposes a callback to do something when an option is selected. We are just implementing the callback, which closes the menu. I don't have a better option than this in mind.

> > This is because I made GeckoShareActionProvider extends ActionProvider, so that it 
> > follows MenuItem's norms. Everything's pretty generic. So, later, if we want another 
> > action-provider (say Reading List), we can create a GeckoReadingListActionProvider, and
> > still things will work. "getActionView()" is an abstract method in ActionProvider.
> 
> Yeah. Well ActionProvider.getActionView isn't really designed for this
> either right? Its designed to be used when the item is being shown on the
> ActionBar vs. on the Menu. i.e. we're not following MenuItem norms at all.
> TBH, it seems like a better way to do this would be to inflate a different
> view here:
> 
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/
> GeckoMenuItem.java#64
> 
> which would mean GeckoMenuInflater/GeckoMenu would need to somehow know this
> was a different type of item. Then BrowserApp wouldn't have to know anything
> about this... What do you think about doing that?

Custom menu was designed with this motive: The XML files will adhere to android's spec, so that they remain same between GB and ICS phones. In other words, if we think of removing the custom menu and go with Android, this would work just fine. This means, the attributes that go with the menu cannot change from what android gives. If we want an identifier from the XML to be a quantifier to differentiate the view, we don't have a choice here. That also means, GeckoMenu cannot know that "Share" is a different kind of item. Hence, the other way round is to use something like an ActionProvider.

I agree that ActionProvider.getActionView() was not made for this. But then, we don't have an Android like menu here. We use a similar methodology (ShareActionView) in a different place. I thought of re-using what we have, with our custom view. With MenuItem being something given by Android, I don't find any other method than setActionProvider() to use in this situation.

We could cast ActionProvider to a GeckoActionProvider and do a "onCreateView()" or "getView()" call on it, if that's something you would like.

Comment 18

[Wesley Johnston (:wesj)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #17)
> http://developer.android.com/reference/android/app/Activity.
> html#closeOptionsMenu%28%29
> closeOptionsMenu() is available at Activity level. We are just using that,
> in case of the provider. Menu wouldn't be knowing about the event click, and
> hence cannot do anything about it.

Heck, why don't we just use that in the MenuItem's onClick listener then (then call the callback if one exists...?) BrowserToolbar doesn't have to close the menu when you do anything else with it. I don't think its consistent to expect it to handle things here.

> Custom menu was designed with this motive: The XML files will adhere to
> android's spec, so that they remain same between GB and ICS phones. In other
> words, if we think of removing the custom menu and go with Android, this
> would work just fine. This means, the attributes that go with the menu
> cannot change from what android gives.

I don't understand this. Can't we can add our own attributes and it shouldn't affect the normal menu inflater (which ours is basically just an exact copy of)?

> We could cast ActionProvider to a GeckoActionProvider and do a
> "onCreateView()" or "getView()" call on it, if that's something you would
> like.

Maybe?

Comment 19

[Sriram Ramasubramanian [:sriram]]

(In reply to Wesley Johnston (:wesj) from comment #18)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #17)
> > http://developer.android.com/reference/android/app/Activity.
> > html#closeOptionsMenu%28%29
> > closeOptionsMenu() is available at Activity level. We are just using that,
> > in case of the provider. Menu wouldn't be knowing about the event click, and
> > hence cannot do anything about it.
> 
> Heck, why don't we just use that in the MenuItem's onClick listener then
> (then call the callback if one exists...?) BrowserToolbar doesn't have to
> close the menu when you do anything else with it. I don't think its
> consistent to expect it to handle things here.

I don't understand this part. Could you show me some code example?

> 
> > Custom menu was designed with this motive: The XML files will adhere to
> > android's spec, so that they remain same between GB and ICS phones. In other
> > words, if we think of removing the custom menu and go with Android, this
> > would work just fine. This means, the attributes that go with the menu
> > cannot change from what android gives.
> 
> I don't understand this. Can't we can add our own attributes and it
> shouldn't affect the normal menu inflater (which ours is basically just an
> exact copy of)?

Our menu inflater is not a copy of Android's. We wrote our own. Also, I wanted the menu files to adhere to Android -- not adding our own attributes. So, we cannot distinguish a menu item  in the XML file. We need to attach such providers later.

> 
> > We could cast ActionProvider to a GeckoActionProvider and do a
> > "onCreateView()" or "getView()" call on it, if that's something you would
> > like.
> 
> Maybe?

Will do it that way.

Comment 20

[Wesley Johnston (:wesj)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #19)
> I don't understand this part. Could you show me some code example?

Right now we have:

public void setDefaultActivityClickListener(View.OnClickListener listener) {
  mDefaultActivity.setOnClickListener(listener);
}

Can we instead do something like:

private View.OnClickListener mListener;
public void setDefaultActivityClickListener(View.OnClickListener listener) {
  mListener = listener;
}

// somewhere else
mDefaultActivity.setOnClickListener(new Listener() {
  public void onClick(...) {
    ((Activity)mContext).closeOptionMenu();
    if (mListener != null)
      mListener.onClick(...);
  }
});

> Our menu inflater is not a copy of Android's. We wrote our own. Also, I
> wanted the menu files to adhere to Android -- not adding our own attributes.
> So, we cannot distinguish a menu item in the XML file. We need to attach
> such providers later.

Whats the downside to adding our own attributes? If we ever went back to Android's menu inflater, this actionitem will not appear either...?

Comment 21

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

I'm kinda on the fence about attributes for menus. I don't know why.

Could we move ahead without needing the comment 20 changes and see what we could do in a followup bug?

I am more interested in hacking the android code to be specific to Firefox and not just a generic copy of the Android code. I don't know if we intend to keep uplifting the newest version from Android.

Comment 22

[Wesley Johnston (:wesj)]

I'm mostly concerned because they menu code is hard to follow. I would rather remove that complexity than to keep piling it on. i.e. the simplest flow right now is something like:

1.) When the menu is shown the GeckoMenuInflater inflates an xml resource in the GeckoMenu

2.) GeckoMenu is a ListView, when items are added, it:
    2a.) Creates an internal GeckoMenuItem object for the item, and adds them to an internal array. Can't remember what that's for...
    2b.) also adds them to a ListAdapter inside itself.

3.) When the listAdapater needs a view, it calls mItem.getLayout(). The GeckoMenuItem returns a MenuItemDefault (TextView) it created in its constructor.

There's also a MenuPresenter for submenus, and an ActionItemPresenter to show certain items on the toolbar in tablet mode as well that I've left out here. But the difference between GeckoMenuItem and MenuItem and MenuItemDefault (and a few other types), is confusing in here. When you start to ask any question like "Who handles the click on the menuitem"

1.) GeckoMenu adds itself a listener (to itself...) in its constructor
2.) GeckoMenu also adds itself as a listener to every MenuItem when they're added
3.) When the item is clicked, GeckoMenu gets the click, and passes it to the MenuItem
4.) The menu item then passes the click to its listener (GeckoMenu in most cases)
5.) GeckoMenu then passes the event to IT'S listener, GeckoApp, which was added in GeckoApp's constructor

It takes a lot of digging to find the answer.

This patch has BrowserToolbar add a GeckoShareActionProvider to a GeckoMenuItem during its construction, which then basically aliases the getView call from GeckoMenuItem to one in the ActionProvider (i.e. you can't tell from GeckoMenuItem what's getting added anymore, you have to find where the ActionProvider was added and track down what type of view was created).

I'm not saying that there's a better solution, just that I want to look for 'em before we add more code.

Comment 23

[Sriram Ramasubramanian [:sriram]]

First two patches to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b227941864b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8945d2378fc2

Comment 24

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 3: Quick Share button

This has all changes (except for Comment 20).
I've renamed files, removed "share", renamed "mDefaultActivity", split and cleanup getOpenURIExternal(), added a divider instead of a view.
Could we handle menu cleanup in a separate bug? I feel like pushing this patch and clean monitoring how it works, and cleanup the menu mess in the background.

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/6b227941864b
https://hg.mozilla.org/mozilla-central/rev/8945d2378fc2

Comment 26

[Wesley Johnston (:wesj)]

Comment on attachment 753420 [details] [diff] [review]
Part 3: Quick Share button

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

I talked to mfinkle about this for a long time yesterday. I think he talked me into being mostly fine with this. I have a few things I still think we should fix, and I think as a follow up we at least need to start documenting these better. A block at the top of GeckoMenu, GeckoMenuItem, MenuItemDefault, etc. explaining what they do. Some comments in ActivityChooserModel and GeckoActionProvider explaining why we had to imported it and write our own. Things to help me so that its not so much work to track things through the code.

I'd like to see the fix to move the menu closing out of BrowserApp here, but (to help me better be able to follow things) maybe you could do that in a separate patch? I'll r+ this with the idea being we'll wait for that to land it.

As a nice follow up, it would be good to shove a diagram of how these classes are related into the menu folder too. Maybe with an explanation/comparison of where our implementation and the Android menu impelmentation are the same or differ and why?

::: mobile/android/base/BrowserApp.java
@@ +1412,5 @@
> +                @Override
> +                public void onTargetSelected() {
> +                    closeOptionsMenu();
> +                }
> +            });

I still want this removed. For normal menuitems, the GeckoMenu class adds itself as a listener and closes itself when they're tappend (before forwarding the event back to the input. I think we should do the same thing here.

::: mobile/android/base/menu/GeckoMenuItem.java
@@ +102,5 @@
>      public View getActionView() {
> +        if (mActionProvider != null && mActionProvider instanceof GeckoActionProvider) {
> +            final View view = ((GeckoActionProvider) mActionProvider).getView(this);
> +            view.setOnClickListener(this);
> +            return view;

This will recreate the view every time this is called. Can we cache it?

@@ +169,5 @@
>  
>      @Override
>      public SubMenu getSubMenu() {
> +        if (mActionProvider != null)
> +            mActionProvider.onPrepareSubMenu(mSubMenu);

braces here and throughout the patch.

::: mobile/android/base/resources/values/dimens.xml
@@ +37,5 @@
>      <!-- Max width of the doorhanger on tablets -->
>      <dimen name="doorhanger_width">400dp</dimen>
>  
>      <dimen name="flow_layout_spacing">6dp</dimen>
> +    <dimen name="menu_item_action_icon">80dp</dimen>

This is the width? Lets add _width to it...

::: mobile/android/base/widget/GeckoActionProvider.java
@@ +67,5 @@
> +        view.setTitle(item.getTitle());
> +        view.setIcon(item.getIcon());
> +        view.setVisibility(item.isVisible() ? View.VISIBLE : View.GONE);
> +        view.setEnabled(item.isEnabled());
> +        return view;

Similarly, can we cache this here?

Comment 27

[Sriram Ramasubramanian [:sriram]]

(In reply to Wesley Johnston (:wesj) from comment #26)
> Comment on attachment 753420 [details] [diff] [review]
> Part 3: Quick Share button
> 
> Review of attachment 753420 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I talked to mfinkle about this for a long time yesterday. I think he talked
> me into being mostly fine with this. I have a few things I still think we
> should fix, and I think as a follow up we at least need to start documenting
> these better. A block at the top of GeckoMenu, GeckoMenuItem,
> MenuItemDefault, etc. explaining what they do. Some comments in
> ActivityChooserModel and GeckoActionProvider explaining why we had to
> imported it and write our own. Things to help me so that its not so much
> work to track things through the code.
> 
> I'd like to see the fix to move the menu closing out of BrowserApp here, but
> (to help me better be able to follow things) maybe you could do that in a
> separate patch? I'll r+ this with the idea being we'll wait for that to land
> it.

Thanks a bunch! :)

> 
> As a nice follow up, it would be good to shove a diagram of how these
> classes are related into the menu folder too. Maybe with an
> explanation/comparison of where our implementation and the Android menu
> impelmentation are the same or differ and why?
> 

May be a wiki page?

> ::: mobile/android/base/BrowserApp.java
> @@ +1412,5 @@
> > +                @Override
> > +                public void onTargetSelected() {
> > +                    closeOptionsMenu();
> > +                }
> > +            });
> 
> I still want this removed. For normal menuitems, the GeckoMenu class adds
> itself as a listener and closes itself when they're tappend (before
> forwarding the event back to the input. I think we should do the same thing
> here.
> 

Will file a followup with what needs to be done to make this better. This is being using my both default activity and the sub menu and involves more work.

> ::: mobile/android/base/menu/GeckoMenuItem.java
> @@ +102,5 @@
> >      public View getActionView() {
> > +        if (mActionProvider != null && mActionProvider instanceof GeckoActionProvider) {
> > +            final View view = ((GeckoActionProvider) mActionProvider).getView(this);
> > +            view.setOnClickListener(this);
> > +            return view;
> 
> This will recreate the view every time this is called. Can we cache it?

Right. The reason is that, the default activity could have changed between two menu opening calls (based on the count and frequency of the app opened). This would ensure that we always have the default activity shown.

> 
> @@ +169,5 @@
> >  
> >      @Override
> >      public SubMenu getSubMenu() {
> > +        if (mActionProvider != null)
> > +            mActionProvider.onPrepareSubMenu(mSubMenu);
> 
> braces here and throughout the patch.

Done.

> 
> ::: mobile/android/base/resources/values/dimens.xml
> @@ +37,5 @@
> >      <!-- Max width of the doorhanger on tablets -->
> >      <dimen name="doorhanger_width">400dp</dimen>
> >  
> >      <dimen name="flow_layout_spacing">6dp</dimen>
> > +    <dimen name="menu_item_action_icon">80dp</dimen>
> 
> This is the width? Lets add _width to it...

Done.

> 
> ::: mobile/android/base/widget/GeckoActionProvider.java
> @@ +67,5 @@
> > +        view.setTitle(item.getTitle());
> > +        view.setIcon(item.getIcon());
> > +        view.setVisibility(item.isVisible() ? View.VISIBLE : View.GONE);
> > +        view.setEnabled(item.isEnabled());
> > +        return view;
> 
> Similarly, can we cache this here?

Nope. Like I mentioned before, we pass the menu item to get the values. These could have been changed my some other piece of code (MenuItem is a logical entity) which the actual View (physical entity) wouldn't know. We have to set it when we are about to show the menu.

Comment 28

[Sriram Ramasubramanian [:sriram]]

Pusssshed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d27425f46c14

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Backed out because something in this push was causing frequent robocop-2 failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f038a760d09

https://tbpl.mozilla.org/php/getParsedLog.php?id=23509945&tree=Mozilla-Inbound

Comment 30

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 4: Hide test

The test expects a popup. We need to change it to accept a submenu. I will fix it in a followup.

Comment 31

[Sriram Ramasubramanian [:sriram]]

Trying: https://tbpl.mozilla.org/?tree=Try&rev=1a6e3c171f0b

Comment 32

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 4: Add test

Change the test so that it now works with the Share submenu for 14+.

Comment 33

[Sriram Ramasubramanian [:sriram]]

Trying again: https://tbpl.mozilla.org/?tree=Try&rev=535f8ee49b0e

Comment 34

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

Comment on attachment 755662 [details] [diff] [review]
Part 4: Add test

Thanks for getting the test working

Comment 35

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9c1d3eb3ce6
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9593bf12a59

Comment 36

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c2a8c555821e because before you landed, robocop-1 on 4.0 was doing fine, and afterward, most of the time it was doing https://tbpl.mozilla.org/php/getParsedLog.php?id=23570178&tree=Mozilla-Inbound.

Lord knows the build system doesn't deserve either word of its name, so it could be that passes were on successful clobber builds and failures were on failed dep builds - if you're sure there's no other possibility, push to try where you'll get a clobber, and retrigger rc1 on 4.0 ten times or so, and if you don't get any testAllPagesTab failures come on back into the tree.

Comment 37

[Teodora Vermesan (:TeoVermesan)]

Test Cases added in moztrap:
https://moztrap.mozilla.org/manage/case/8227/
https://moztrap.mozilla.org/manage/case/8226/

Comment 38

[Aaron Train [:aaronmt]]

(In reply to Teodora Vermesan (:TeoVermesan) from comment #37)
> Test Cases added in moztrap:
> https://moztrap.mozilla.org/manage/case/8227/
> https://moztrap.mozilla.org/manage/case/8226/

How did you add test-cases when the feature has not landed yet?

Comment 39

[Teodora Vermesan (:TeoVermesan)]

The test cases are disabled in moztrap. I will enable them after the feature will be integrated in Nightly and I will change them if something is needed.
I created the test cases based on the design specifications (mockup and feature description from the bug).

Comment 40

[Sriram Ramasubramanian [:sriram]]

(In reply to Phil Ringnalda (:philor) from comment #36)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c2a8c555821e because
> before you landed, robocop-1 on 4.0 was doing fine, and afterward, most of
> the time it was doing
> https://tbpl.mozilla.org/php/getParsedLog.php?id=23570178&tree=Mozilla-
> Inbound.
> 
> Lord knows the build system doesn't deserve either word of its name, so it
> could be that passes were on successful clobber builds and failures were on
> failed dep builds - if you're sure there's no other possibility, push to try
> where you'll get a clobber, and retrigger rc1 on 4.0 ten times or so, and if
> you don't get any testAllPagesTab failures come on back into the tree.

Should I clobber if I change robocop files?
https://tbpl.mozilla.org/?tree=Try&rev=535f8ee49b0e <--- this is all green! that's why I got an r+ and pushed it.

Comment 41

[Sriram Ramasubramanian [:sriram]]

Basically the records file is read on the UI thread many times.

1. We call invalidateOptionsMenu() for most tab related changes. (oh well!)
2. This call onPrepareOptionsMenu()
3. That sets a new Intent to the provider everytime!
   Problem: Set the intent once and update it everytime. So, records won't be fetched for each page unless it changes (by sharing).
4. Reading happens on UI thread.
   Problem?? But Android's examples does it. This is a XML file parsing -- same as Menu file reading. We can put it to test and change it later I guess.
5. That stalls the network and the URLBar isn't updated leading to robocop failure.

Comment 42

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 3b: Fix set intent

This adds a new intent if none exists, else changes the values in an existing intent (as per Android recommendation).

(Locally testAllPagesTab passes 5 times in 5 successive calls. No n/w lag too!)

Pushing it to try soon.

Comment 43

[Sriram Ramasubramanian [:sriram]]

https://tbpl.mozilla.org/?tree=Try&rev=9d4bd34351a5

Comment 44

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

Comment on attachment 756236 [details] [diff] [review]
Part 3b: Fix set intent

This is a good change.

Sadly, even with this change, the Try run has 7 out of 15 rc1 failures.

Comment 45

[Sriram Ramasubramanian [:sriram]]

Reading the data from the share_history.xml takes about 45-50ms. This cannot be a performance bottleneck which would stall network access.

Comment 46

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 3c: ActionProvider usage

We've been calling onPrepareSubMenu() whenever the menu wanted to access anything (findItem() call), on the ActionProvider. This was querying, building and stalling the UI thread most of the times.
This patch fixes it and the onPrepareSubMenu() will be called only when the intent changes. So, UI thread is left free and the n/w access is happy.

https://tbpl.mozilla.org/?tree=Try&rev=ad3b444f90e1 <-- ohhhh soooo greeen

Comment 47

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 3c: Option 2: ActionProvider usage

This is a cleaner approach where there are no interfaces to listen to when an intent is set, and where we push get the list until the "Share" button is clicked.

Comment 48

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

Comment on attachment 757689 [details] [diff] [review]
Part 3c: Option 2: ActionProvider usage

I favor this one a bit more. Not needing a listener might be part of it.

You need to do a Try run on this one too, in case it acts differently than Option 1.

Also, I'd like Wes to give some feedback on Option 1 and Option 2

Comment 49

[Sriram Ramasubramanian [:sriram]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=75e1b79c0a70

Comment 50

[Wesley Johnston (:wesj)]

Comment on attachment 757689 [details] [diff] [review]
Part 3c: Option 2: ActionProvider usage

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

I think I like this one better too.

::: mobile/android/base/menu/GeckoMenu.java
@@ +260,5 @@
>              } else if (menuItem.hasSubMenu()) {
> +                if (menuItem.getActionProvider() == null) {
> +                    SubMenu subMenu = menuItem.getSubMenu();
> +                    MenuItem item = subMenu.findItem(id);
> +                    if (item != null)

Why do you have this null check? You will return null anyway, right?

Comment 51

[Sriram Ramasubramanian [:sriram]]

(In reply to Wesley Johnston (:wesj) from comment #50)
> Comment on attachment 757689 [details] [diff] [review]
> Part 3c: Option 2: ActionProvider usage
> 
> Review of attachment 757689 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think I like this one better too.
> 
> ::: mobile/android/base/menu/GeckoMenu.java
> @@ +260,5 @@
> >              } else if (menuItem.hasSubMenu()) {
> > +                if (menuItem.getActionProvider() == null) {
> > +                    SubMenu subMenu = menuItem.getSubMenu();
> > +                    MenuItem item = subMenu.findItem(id);
> > +                    if (item != null)
> 
> Why do you have this null check? You will return null anyway, right?

That piece of code is running inside a for-loop. It tries to find an item in the submenu. If it can find it, it will return early (breaking the for-loop). If not, other items will be checked, and if everything fails, null will be returned.

Comment 52

[Sriram Ramasubramanian [:sriram]]

Phew!
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fc2d7079ac7
https://hg.mozilla.org/integration/mozilla-inbound/rev/055a2357e1a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9eaa9f9b291
https://hg.mozilla.org/integration/mozilla-inbound/rev/02fd86cfa7ed

Comment 53

[Sriram Ramasubramanian [:sriram]]

I have a weird feeling that we might face the same issue as we had in with "share items in custom sub menu doesn't show" on phones running CyanogenMod. The reason there was that it took a bit of time to query and get the list of activities, and then show them.
With the "Part 3c: Option 2", the problem is quite similar where we wait populating until we show the submenu the first time. On my phone, it shows up a bit slower on the first time.

Aaron, could you verify if it works on CM09 phones and update me? If it doesn't work there properly, we can go with "Part 3c", which populates the list when we set the ActionProvider, and the sub menu is ready to be shown anytime.

Comment 54

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0fc2d7079ac7
https://hg.mozilla.org/mozilla-central/rev/055a2357e1a8
https://hg.mozilla.org/mozilla-central/rev/d9eaa9f9b291
https://hg.mozilla.org/mozilla-central/rev/02fd86cfa7ed

Comment 55

[Aaron Train [:aaronmt]]

I don't have access to CM09; I think Dave Miller has a Galaxy Tab 10.1 running CM09.

Comment 56

[Aaron Train [:aaronmt]]

Am I supposed to be seeing the old share menu again? I thought it was problematic?

Comment 57

[Kevin Brosnan [Ex-Mozilla]]

New feature.

Comment 58

[Gabriela [:gaby2300]]

I and cuantocarlos(a Mozilla Hispano QA contributor) can confirm the bug is fixed using me a Samsung Galaxy S2 phone and he a Samsung Galaxy S3 phone, both with the latest beta.

Comment 59

[Alex Keybl [:akeybl]]

Adding the feature keyword to be included in the new Release Tracking page.