878136 - Add back ContextMenu functionality from AwesomeBar to BrowserApp

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sriram Ramasubramanian [:sriram]]

Bug 862796 moved ContextMenu related code from awesomebar/AwesomeBar to home/BookmarksPage. However, the event handlers weren't capable of handling few menu items. Add them back in BrowserApp as one common place.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Part 1: ContextMenu

This moves the bookmarks list view to a HomeListView. This is because, we return a new type of ContextMenuInfo from this ListView, that can hold values like url, title, etc. (This is a better way of doing context-menu -- and recommended way).

This ListView internally holds a HomeContextMenuInfo, which is the new datastructure holding data about the row that was long pressed.

I need to add TextWatchers to the AlertDialog in edit. However I faced a problem.
( http://hg.mozilla.org/projects/fig/file/tip/mobile/android/base/AwesomeBar.java#l525 )
1. I don't want to copy that code into BrowserApp and make BrowserApp bigger and bigger.
2. Putting in a separate file will make me add 3 different files! :O
I need some direction here. Shall I just copy them into BrowserApp?

Some part of me doesn't like the huuuuge switch case statement for onContextItemSelected(). Probably they should all go into smaller methods. Is that fine? (This is a refactor+rewrite right? ;) ).

And the HomeListView could be used with other ListViews in about:home. History is an exception, as it uses an ExpandedListView (and the ContextMenuInfo changes there). That makes me feel, even EventHandlers should move into HomeListView. But, some of them are tied to Bookmarks. (So, an HomeListViewEventHandlers and a BookmarkListViewEventHandlers that extends the other?? That's making things worse and I stopped there! :( ).

If and when we move EventHandlers out, I would like to make a "BookmarksListView extends HomeListView", and move the adapters out of BookmarksPage. So, BookmarksListView will be self contained and BookmarksPage will be clean as a fragment. (But, those aren't in the scope of this. If you feel those are good, I can file followups and carry out those stuff).

And I don't like the check for info.url is null in every case. 1. It should never be null. 2. We shouldn't accept a change in bookmark url, if user erased all of its contents. (Till we resolve that, I would have to have those if conditions).

Ideally, if this patch helps Lucas with visited panel, then moving out code to separate file was a good idea :D

Comment 2

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

Comment on attachment 756801 [details] [diff] [review]
Part 1: ContextMenu

Flipping roles

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 756801 [details] [diff] [review]
Part 1: ContextMenu

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

Ok, my take on this would be something like this:
1. Create a HomeListFrament that extends ListFragment.
2. Implement onContextItemSelected() in HomeListFragment to handle the common context actions (open_private_tab, open_new_tab, edit_bookmark, etc)
3. Change BookmarksPage to extend HomeListFragment  and make it conform to ListFragment requirements (e.g. listview id)
4. Move HomeListView and HomeContextMenuInfo as inner static classes of HomeListFragment.
5. Encapsulate all bookmark editing bits (dialog creating, text watcher, etc) into a EditBookmarkDialog thingy. 

If there are overlapping context actions between fragments in HomePager and BrowserApp, you can use something like this to only handle the actions coming from the fragments;
http://stackoverflow.com/questions/5297842/how-to-handle-oncontextitemselected-in-a-multi-fragment-activity

::: mobile/android/base/BrowserApp.java
@@ +634,5 @@
> +                Toast.makeText(BrowserApp.this, R.string.new_tab_opened, Toast.LENGTH_SHORT).show();
> +                return true;
> +            }
> +            case R.id.edit_bookmark: {
> +                AlertDialog.Builder editPrompt = new AlertDialog.Builder(this);

You should probably create a custom dialog that encapsulates all this code. EditBookmarkDialog or something.

::: mobile/android/base/home/BookmarksPage.java
@@ +221,5 @@
>          }
>  
>          @Override
>          public void onCreateContextMenu(ContextMenu menu, View view, ContextMenuInfo menuInfo) {
> +            if (!(menuInfo instanceof HomeContextMenuInfo)) {

Is this check really necessary? In any case, if you really want to catch this, it should actually throw an IllegalArgumentException instead of silently bailing here.

::: mobile/android/base/home/HomeListView.java
@@ +23,5 @@
> +/**
> + * HomeListView is a custom extension of ListView, that packs a HomeContextMenuInfo
> + * when any of its rows is long pressed.
> + */
> +public class HomeListView extends ListView

Not very fond of this name. But I can think of a better one, so... ;-)

@@ +47,5 @@
> +    @Override
> +    public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id) {
> +        Cursor cursor = (Cursor) parent.getItemAtPosition(position);
> +        mContextMenuInfo = new HomeContextMenuInfo(view, position, id, cursor);
> +        return super.showContextMenuForChild(HomeListView.this);

Simply 'this' should do it here.

@@ +58,5 @@
> +
> +    /**
> +     * Custom ContextMenuInfo that adds details from the cursor.
> +     */
> +    public static class HomeContextMenuInfo extends AdapterContextMenuInfo {

You can probably just call this ContextMenuInfo as you'll likely use it with the HomeListView namespace anyway. The 'Home' prefix is a bit weird.

@@ +65,5 @@
> +        public byte[] favicon;
> +        public String title;
> +        public String keyword;
> +        public int display;
> +        public boolean isFolder;

Make all these properties final so that you can only set their values in the constructor.

@@ +70,5 @@
> +
> +        public HomeContextMenuInfo(View targetView, int position, long id, Cursor cursor) {
> +            super(targetView, position, id);
> +
> +            if (cursor != null) {

An early return here would make this code a bit cleaner by avoiding one extre indentation level.

@@ +71,5 @@
> +        public HomeContextMenuInfo(View targetView, int position, long id, Cursor cursor) {
> +            super(targetView, position, id);
> +
> +            if (cursor != null) {
> +                isFolder = cursor.getInt(cursor.getColumnIndexOrThrow(Bookmarks.TYPE)) == Bookmarks.TYPE_FOLDER;

nit: I usually like to add parens around expressions in the context of assignments like this.

@@ +87,5 @@
> +                rowId = cursor.getInt(cursor.getColumnIndexOrThrow(Bookmarks._ID));
> +                url = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL));
> +                title = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.TITLE));
> +                favicon = cursor.getBlob(cursor.getColumnIndexOrThrow(URLColumns.FAVICON));
> +                display = Combined.DISPLAY_NORMAL;

Always DISPLAY_NORMAL?

Comment 4

[Wesley Johnston (:wesj)]

(In reply to Lucas Rocha (:lucasr) from comment #3)
> You should probably create a custom dialog that encapsulates all this code.
> EditBookmarkDialog or something.
This is already done in bug 872388.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #4)
> (In reply to Lucas Rocha (:lucasr) from comment #3)
> > You should probably create a custom dialog that encapsulates all this code.
> > EditBookmarkDialog or something.
> This is already done in bug 872388.

Excellent :-)

Comment 6

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #5)
> (In reply to Wesley Johnston (:wesj) from comment #4)
> > (In reply to Lucas Rocha (:lucasr) from comment #3)
> > > You should probably create a custom dialog that encapsulates all this code.
> > > EditBookmarkDialog or something.
> > This is already done in bug 872388.
> 
> Excellent :-)

Oh! Landed 2 days back. Will refresh my build.

Comment 7

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #3)
> Comment on attachment 756801 [details] [diff] [review]
> Part 1: ContextMenu
> 
> Review of attachment 756801 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, my take on this would be something like this:
> 1. Create a HomeListFrament that extends ListFragment.

So, BookmarksPage is not just about ListViews. That stopped me from taking this approach.

> 2. Implement onContextItemSelected() in HomeListFragment to handle the
> common context actions (open_private_tab, open_new_tab, edit_bookmark, etc)
> 3. Change BookmarksPage to extend HomeListFragment  and make it conform to
> ListFragment requirements (e.g. listview id)

BookmarksPage will have a GridView too. Basically it would be a complex layout with 2 or more views instead of just being a ListView alone. Does it make sense to make it a ListFragment? I agree with moving the ListView out and making a class of its own (not the Fragment part, but #4). But I am not sure about treating BookmarksPage a ListFragment. Could BookmarksPage be a generic Fragment (like now) and implement onContextItemSelected() ?

> 4. Move HomeListView and HomeContextMenuInfo as inner static classes of
> HomeListFragment.
> 5. Encapsulate all bookmark editing bits (dialog creating, text watcher,
> etc) into a EditBookmarkDialog thingy. 
> 

Will make use of the other bug.


> ::: mobile/android/base/home/HomeListView.java
> @@ +23,5 @@
> > +/**
> > + * HomeListView is a custom extension of ListView, that packs a HomeContextMenuInfo
> > + * when any of its rows is long pressed.
> > + */
> > +public class HomeListView extends ListView
> 
> Not very fond of this name. But I can think of a better one, so... ;-)

I am not good with names. So I went with a voting in channel between HomePageListView, PageListView and HomeListView :P This won! :P

> 
> @@ +47,5 @@
> > +    @Override
> > +    public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id) {
> > +        Cursor cursor = (Cursor) parent.getItemAtPosition(position);
> > +        mContextMenuInfo = new HomeContextMenuInfo(view, position, id, cursor);
> > +        return super.showContextMenuForChild(HomeListView.this);
> 
> Simply 'this' should do it here.

I wanted to change it, but forgot :(

> 
> @@ +58,5 @@
> > +
> > +    /**
> > +     * Custom ContextMenuInfo that adds details from the cursor.
> > +     */
> > +    public static class HomeContextMenuInfo extends AdapterContextMenuInfo {
> 
> You can probably just call this ContextMenuInfo as you'll likely use it with
> the HomeListView namespace anyway. The 'Home' prefix is a bit weird.
> 

I know its a bit big. But the part that made me confused is Android's terminology. It uses AdapterContextMenuInfo, ExpandedListContextMenuInfo and so on. So I thought of using HomeContextMenuInfo. I can make it just ContextMenuInfo if that wouldn't cause any confusion.

> @@ +65,5 @@
> > +        public byte[] favicon;
> > +        public String title;
> > +        public String keyword;
> > +        public int display;
> > +        public boolean isFolder;
> 
> Make all these properties final so that you can only set their values in the
> constructor.

Making it final is what I wanted to do. And then I referred Android's documentation. Their ContextMenuInfo properties aren't made final. So I felt a bit lazy ;)

> @@ +87,5 @@
> > +                rowId = cursor.getInt(cursor.getColumnIndexOrThrow(Bookmarks._ID));
> > +                url = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL));
> > +                title = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.TITLE));
> > +                favicon = cursor.getBlob(cursor.getColumnIndexOrThrow(URLColumns.FAVICON));
> > +                display = Combined.DISPLAY_NORMAL;
> 
> Always DISPLAY_NORMAL?

Ya. We won't show reading list items right?

Comment 8

[Lucas Rocha (:lucasr)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #7)
> (In reply to Lucas Rocha (:lucasr) from comment #3)
> > Comment on attachment 756801 [details] [diff] [review]
> > Part 1: ContextMenu
> > 
> > Review of attachment 756801 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Ok, my take on this would be something like this:
> > 1. Create a HomeListFrament that extends ListFragment.
> 
> So, BookmarksPage is not just about ListViews. That stopped me from taking
> this approach.
> 
> > 2. Implement onContextItemSelected() in HomeListFragment to handle the
> > common context actions (open_private_tab, open_new_tab, edit_bookmark, etc)
> > 3. Change BookmarksPage to extend HomeListFragment  and make it conform to
> > ListFragment requirements (e.g. listview id)
> 
> BookmarksPage will have a GridView too. Basically it would be a complex
> layout with 2 or more views instead of just being a ListView alone. Does it
> make sense to make it a ListFragment? I agree with moving the ListView out
> and making a class of its own (not the Fragment part, but #4). But I am not
> sure about treating BookmarksPage a ListFragment. Could BookmarksPage be a
> generic Fragment (like now) and implement onContextItemSelected() ?

Yep, that should be fine. Just create a "HomeFragment" with the onContextItemSelected() bits and make BookmarksPage inherit from it.

Comment 9

[Sriram Ramasubramanian [:sriram]]

(In reply to Lucas Rocha (:lucasr) from comment #8)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #7)
> > (In reply to Lucas Rocha (:lucasr) from comment #3)
> > > Comment on attachment 756801 [details] [diff] [review]
> > > Part 1: ContextMenu
> > > 
> > > Review of attachment 756801 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Ok, my take on this would be something like this:
> > > 1. Create a HomeListFrament that extends ListFragment.
> > 
> > So, BookmarksPage is not just about ListViews. That stopped me from taking
> > this approach.
> > 
> > > 2. Implement onContextItemSelected() in HomeListFragment to handle the
> > > common context actions (open_private_tab, open_new_tab, edit_bookmark, etc)
> > > 3. Change BookmarksPage to extend HomeListFragment  and make it conform to
> > > ListFragment requirements (e.g. listview id)
> > 
> > BookmarksPage will have a GridView too. Basically it would be a complex
> > layout with 2 or more views instead of just being a ListView alone. Does it
> > make sense to make it a ListFragment? I agree with moving the ListView out
> > and making a class of its own (not the Fragment part, but #4). But I am not
> > sure about treating BookmarksPage a ListFragment. Could BookmarksPage be a
> > generic Fragment (like now) and implement onContextItemSelected() ?
> 
> Yep, that should be fine. Just create a "HomeFragment" with the
> onContextItemSelected() bits and make BookmarksPage inherit from it.

I thought of doing it when I add the GridView.. :P
But I'll do with this patch.

Comment 10

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

Diff over old patch:
1. Created a HomeFragment and BookmarksPage extends from it.
2. HomeFragment handles the onContextItemSelected() for anything that extends it (for now). If there is a special case, the extended fragment can handle it.
3. HomeListView is placed outside (as it will be used by Visited page too).
4. Waiting on wesj's patch for EditBookmark to land in central. I can remove that piece of code once that lands (may be a follow up to this). (or remove handling that menu-item and mark it a followup).
5. HomeContextMenuInfo properties cannot be made final. It has to call super() in its constructor -- and by doing that, a "final url" won't be initialized and Java cries.
6. Didn't change the name of HomeContextMenuInfo yet (confirming to other ContextMenuInfo's). I can change it if we want it.

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 760629 [details] [diff] [review]
Patch

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

Looks good to me.

::: mobile/android/base/GeckoAppShell.java
@@ +679,5 @@
>          createShortcut(aTitle, aURI, aURI, aIconData, aType);
>      }
>  
>      // internal, for non-webapps
> +    public static void createShortcut(String aTitle, String aURI, Bitmap aBitmap, String aType) {

You should probably remove the comment then, no?

::: mobile/android/base/home/HomeFragment.java
@@ +64,5 @@
> +
> +                GeckoAppShell.openUriExternal(info.url, SHARE_MIME_TYPE, "", "",
> +                                              Intent.ACTION_SEND, info.title);
> +                return true;
> +            }

nit: add empty lines between each 'case'.

@@ +95,5 @@
> +                Tabs.getInstance().loadUrl(info.url, flags);
> +                Toast.makeText(activity, R.string.new_tab_opened, Toast.LENGTH_SHORT).show();
> +                return true;
> +            }
> +            case R.id.edit_bookmark: {

Please file a follow-up to use Wes' EditBookmark.

::: mobile/android/base/home/HomeListView.java
@@ +69,5 @@
> +        public HomeContextMenuInfo(View targetView, int position, long id, Cursor cursor) {
> +            super(targetView, position, id);
> +
> +            if (cursor == null)
> +                return;

nit: I tend to prefer if's with {}'s, even for one-liners.

@@ +74,5 @@
> +
> +            isFolder = (cursor.getInt(cursor.getColumnIndexOrThrow(Bookmarks.TYPE)) == Bookmarks.TYPE_FOLDER);
> +
> +            if (isFolder)
> +                return;

Ditto.

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/478e2819dbf7