Closed Bug 869123 Opened 11 years ago Closed 11 years ago

Quick Share from Android menu

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(relnote-firefox 24+)

RESOLVED FIXED
Firefox 24
Tracking Status
relnote-firefox --- 24+

People

(Reporter: ibarlow, Assigned: sriram)

References

Details

(Keywords: feature, Whiteboard: [testday-20130823])

Attachments

(7 files, 3 obsolete files)

Attached image 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.
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.
Sriram, does this depend on bug 825799?
Flags: needinfo?(sriram)
Duplicate of 869123
Flags: needinfo?(sriram)
That's this bug (so I guess its technically true?)
(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.
Blocks: 801043
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).
It's better to set the properties inside MenuItemDefault, than doing in XML and "inflating" it. This makes MenuItemDefault re-usable.
Attachment #749967 - Flags: review?(mark.finkle)
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.
Attachment #749968 - Flags: review?(mark.finkle)
Attachment #749967 - Flags: review?(mark.finkle) → review?(wjohnston)
Attachment #749968 - Flags: review?(mark.finkle) → review?(wjohnston)
Attached patch Part 3: Quick Share button (obsolete) — Splinter Review
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.
Attachment #749973 - Flags: review?(wjohnston)
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 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?
Attachment #749967 - Flags: review?(wjohnston) → review+
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!
Attachment #749968 - Flags: review?(wjohnston) → review+
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".
Attachment #749973 - Flags: review?(wjohnston) → review-
(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.
(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.
(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.
(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.
(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?
(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.
(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...?
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.
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.
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.
Attachment #749973 - Attachment is obsolete: true
Attachment #753420 - Flags: review?(wjohnston)
Flags: in-moztrap?(fennec)
OS: Mac OS X → Android
Hardware: x86 → ARM
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?
Attachment #753420 - Flags: review?(wjohnston) → review+
(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.
Depends on: 876938
Depends on: 876941
Attached patch Part 4: Hide test (obsolete) — Splinter Review
The test expects a popup. We need to change it to accept a submenu. I will fix it in a followup.
Attachment #755459 - Flags: review?(mark.finkle)
Attached patch Part 4: Add testSplinter Review
Change the test so that it now works with the Share submenu for 14+.
Attachment #755459 - Attachment is obsolete: true
Attachment #755459 - Flags: review?(mark.finkle)
Attachment #755662 - Flags: review?(mark.finkle)
Comment on attachment 755662 [details] [diff] [review]
Part 4: Add test

Thanks for getting the test working
Attachment #755662 - Flags: review?(mark.finkle) → review+
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.
Test Cases added in moztrap:
https://moztrap.mozilla.org/manage/case/8227/
https://moztrap.mozilla.org/manage/case/8226/
Flags: in-moztrap?(fennec) → in-moztrap+
(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?
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).
(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.
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.
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.
Attachment #756236 - Flags: review?(mark.finkle)
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.
Attachment #756236 - Flags: review?(mark.finkle) → review+
Reading the data from the share_history.xml takes about 45-50ms. This cannot be a performance bottleneck which would stall network access.
Attached patch Part 3c: ActionProvider usage (obsolete) — Splinter Review
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
Attachment #757608 - Flags: review?(mark.finkle)
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.
Attachment #757689 - Flags: review?(mark.finkle)
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
Attachment #757689 - Flags: review?(mark.finkle)
Attachment #757689 - Flags: review+
Attachment #757689 - Flags: feedback?(wjohnston)
Attachment #757608 - Flags: feedback?(wjohnston)
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?
Attachment #757689 - Flags: feedback?(wjohnston) → feedback+
(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.
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.
Flags: needinfo?(aaron.train)
I don't have access to CM09; I think Dave Miller has a Galaxy Tab 10.1 running CM09.
Flags: needinfo?(aaron.train)
Attachment #757608 - Flags: feedback?(wjohnston)
Depends on: 880230
Am I supposed to be seeing the old share menu again? I thought it was problematic?
Attachment #757608 - Attachment is obsolete: true
Attachment #757608 - Flags: review?(mark.finkle)
Depends on: 881861
Depends on: 884566
New feature.
relnote-firefox: --- → ?
Depends on: 888255
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.
Whiteboard: [testday-20130823]
Adding the feature keyword to be included in the new Release Tracking page.
Keywords: feature
Depends on: 963363
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: