Closed Bug 967293 Opened 7 years ago Closed 7 years ago

Remove duplicate logic from TopSitesPanel.onContextItemSelected()

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 31

People

(Reporter: oogunsakin, Assigned: oogunsakin)

Details

Attachments

(1 file, 5 obsolete files)

TopSitesPanel.onContextItemSelected() currently duplicates a lot of logic already implemented its super class HomeFragment.onContextItemSelected(). Remove duplicated logic for options: "Share", "Open in new Tab", "Open in private Tab" and "Add to Home Screen".
Attached patch top-sites-contextmenu.patch (obsolete) — Splinter Review
Tests:
-Opened context menu in top sites panel. Saw same options listed as before the patch. 
-Tested all context menu items in "Top Sites" panel. They all functioned correctly.
Attachment #8369737 - Flags: review?(liuche)
Comment on attachment 8369737 [details] [diff] [review]
top-sites-contextmenu.patch

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

Good choice to refactor, I like that you're ripping out duplicate code.

::: mobile/android/base/home/HomeFragment.java
@@ +154,5 @@
>                  flags |= Tabs.LOADURL_PRIVATE;
>  
>              final String url = (info.isInReadingList() ? ReaderModeUtils.getAboutReaderForUrl(info.url) : info.url);
> +            // Decode "user-entered" URLs before loading them.
> +            Tabs.getInstance().loadUrl(decodeUserEnteredUrl(info.url), flags);

Hm, what's the case where this is user-entered (and why wasn't it necessary before)? Maybe add a comment to clarify what "user-entered" means, and when that might happen.

@@ +210,5 @@
>      boolean getCanLoadHint() {
>          return mCanLoadHint;
>      }
>  
> +    static String decodeUserEnteredUrl(String url) {

Since this is copied from TopSitesPanel, you should remove it from that class, and also make this public probably.

::: mobile/android/base/home/TopSitesGridView.java
@@ +10,3 @@
>  import org.mozilla.gecko.R;
>  import org.mozilla.gecko.ThumbnailHelper;
> +import org.mozilla.gecko.db.BrowserContract.URLColumns;

Hm, I wonder why we used BrowserDB.URLColumns in the first place.

@@ +124,5 @@
>              @Override
>              public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id) {
>                  Cursor cursor = (Cursor) parent.getItemAtPosition(position);
> +
> +                if (cursor == null) {

When can the cursor be null? If it's an unexpected case, you might want to log here.

@@ +247,5 @@
>          mEditPinnedSiteListener = listener;
>      }
>  
>      /**
> +     * A ContextMenuInfo for TopSitesGridView items

Nit: period, and possibly include the rest of the original comment (or even make it more specific and useful!)

@@ +255,2 @@
>  
> +        public TopSitesGridContextMenuInfo(View targetView, int position, long id) {

If you're just calling the super constructor, do you need this?

::: mobile/android/base/home/TopSitesPanel.java
@@ +269,5 @@
>  
>      @Override
>      public boolean onContextItemSelected(MenuItem item) {
> +        if (super.onContextItemSelected(item)) {
> +            //HomeFragment was able to handle to selected item

Nit: space between // and HomeFragment.

::: mobile/android/base/resources/menu/top_sites_contextmenu.xml
@@ +5,5 @@
>  
>  <menu xmlns:android="http://schemas.android.com/apk/res/android">
>  
> +    <!-- Items prefixed "home" are handled by HomeFragment, "top_sites"
> +         by TopSitesPanel -->

Can we just get rid of top_sites_contextmenu.xml and handle showing/hiding the items in Java?
Attachment #8369737 - Flags: review?(liuche) → feedback+
Comment on attachment 8369737 [details] [diff] [review]
top-sites-contextmenu.patch

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

::: mobile/android/base/home/HomeFragment.java
@@ +154,5 @@
>                  flags |= Tabs.LOADURL_PRIVATE;
>  
>              final String url = (info.isInReadingList() ? ReaderModeUtils.getAboutReaderForUrl(info.url) : info.url);
> +            // Decode "user-entered" URLs before loading them.
> +            Tabs.getInstance().loadUrl(decodeUserEnteredUrl(info.url), flags);

when a user enters a URL manually in the "Edit Pin Site" dialog in the top sites panel, it is wrapped in a user-entered url. we didn't need this here before because we weren't getting pinned site items in HomeFragment.onContextItemSelected() but it was being used in TopSitesPanel.onContextItemSelected() for loading urls. decodeUserEnteredUrl() handles both user-entered URL's and well defined urls.

@@ +210,5 @@
>      boolean getCanLoadHint() {
>          return mCanLoadHint;
>      }
>  
> +    static String decodeUserEnteredUrl(String url) {

alright

::: mobile/android/base/home/TopSitesGridView.java
@@ +124,5 @@
>              @Override
>              public boolean onItemLongClick(AdapterView<?> parent, View view, int position, long id) {
>                  Cursor cursor = (Cursor) parent.getItemAtPosition(position);
> +
> +                if (cursor == null) {

legacy code. i googled around, not sure why it would happen. i'll Log it

@@ +255,2 @@
>  
> +        public TopSitesGridContextMenuInfo(View targetView, int position, long id) {

we need to expose the super constructor

::: mobile/android/base/resources/menu/top_sites_contextmenu.xml
@@ +5,5 @@
>  
>  <menu xmlns:android="http://schemas.android.com/apk/res/android">
>  
> +    <!-- Items prefixed "home" are handled by HomeFragment, "top_sites"
> +         by TopSitesPanel -->

Wouldn't it be a bit messy?
Attached patch top-sites-contextmenu.patch (obsolete) — Splinter Review
Attachment #8369737 - Attachment is obsolete: true
Attachment #8371691 - Flags: review?(liuche)
Comment on attachment 8371691 [details] [diff] [review]
top-sites-contextmenu.patch

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

Looks good, small nit.

> ::: mobile/android/base/resources/menu/top_sites_contextmenu.xml
> @@ +5,5 @@
> >  
> >  <menu xmlns:android="http://schemas.android.com/apk/res/android">
> >  
> > +    <!-- Items prefixed "home" are handled by HomeFragment, "top_sites"
> > +         by TopSitesPanel -->
> 
> Can we just get rid of top_sites_contextmenu.xml and handle showing/hiding
> the items in Java?

Sorry, I might not have been clear enough. top_sites_contextmenu looks almost exactly like home_contextmenu, so we should combine the two. Then in TopSitesPanel, instead of inflating top_sites_contextmenu, use home_contextmenu instead, and handle setting visible/gone in Java.

(Also, as a reviewing nit, when responding to a comment, be sure to include the relevant text from both parties in the conversation. For example, in comment #3, it's hard to tell what specific review comment from comment #2 you're responding to.)

Also, I'll be on PTO next week, so make sure to flag someone else for the final review!

::: mobile/android/base/home/HomeFragment.java
@@ +155,5 @@
>  
>              final String url = (info.isInReadingList() ? ReaderModeUtils.getAboutReaderForUrl(info.url) : info.url);
> +
> +            // Some pinned site items have "user-entered" urls. URLs entered in the PinSiteDialog are wrapped in
> +            // a special URI until we can get a valid URL. if the url is a user-entered url, decode the URL before loading it.

Nit: capitalize "If"
Attachment #8371691 - Flags: review?(liuche) → feedback+
Attached patch top-sites-contextmenu.patch (obsolete) — Splinter Review
Try push: https://tbpl.mozilla.org/?tree=Try&rev=5dfc0cd7acc1
Attachment #8371691 - Attachment is obsolete: true
Attachment #8378766 - Flags: review?(liuche)
Comment on attachment 8378766 [details] [diff] [review]
top-sites-contextmenu.patch

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

Looks good! Just a couple of questions and a bunch of nits. I'd like to see one more version, but I think this pretty much there.

Also regarding comments, try to keep in mind their intent of helping someone skimming this code to get an idea of what's going on. It's tricky for the TopSites list/grid view handling because there *is* so much shared code and you do need to know what the other super/sub-classes are doing. It's a bit spaghetti code to start with too, so maybe that's a hint that this code needs another refactor down the line anyways...

::: mobile/android/base/home/HomeFragment.java
@@ +87,5 @@
>          inflater.inflate(R.menu.home_contextmenu, menu);
>  
>          menu.setHeaderTitle(info.getDisplayTitle());
>  
> +        // Remove top ununsed menu items.

Be consistent with the wording of the other comments in this file doing the same thing.
// Hide unused menu items specific to top sites.

@@ +161,5 @@
>              final String url = (info.isInReadingList() ? ReaderModeUtils.getAboutReaderForUrl(info.url) : info.url);
> +
> +            // Some pinned site items have "user-entered" urls. URLs entered in the PinSiteDialog are wrapped in
> +            // a special URI until we can get a valid URL. If the url is a user-entered url, decode the URL before loading it.
> +            Tabs.getInstance().loadUrl(decodeUserEnteredUrl(info.url), flags);

Split out decodeUserEnteredUrl into a separate variable. That will make the comment more obvious.

@@ +217,5 @@
>      boolean getCanLoadHint() {
>          return mCanLoadHint;
>      }
>  
> +    public static String decodeUserEnteredUrl(String url) {

Add javadoc-style comment for this method.

::: mobile/android/base/home/TopSitesGridView.java
@@ +132,5 @@
> +
> +                mContextMenuInfo = new TopSitesGridContextMenuInfo(view, position, id);
> +                mContextMenuInfo.url = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL));
> +                mContextMenuInfo.title = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.TITLE));
> +                mContextMenuInfo.isPinned = ((TopSitesCursorWrapper) cursor).isPinned();

For simplicity, split this into another function.

@@ +247,5 @@
>          mEditPinnedSiteListener = listener;
>      }
>  
>      /**
> +     * Stores information regarding the creation of the context menu for a grid view item

Nit: period at the end.
Nit: "GridView item"

::: mobile/android/base/home/TopSitesPanel.java
@@ +281,5 @@
>      }
>  
>      @Override
>      public void onCreateContextMenu(ContextMenu menu, View view, ContextMenuInfo menuInfo) {
> +        // TopSitesPanel.onCreateContextMenu() is shared between the mList and mGrid views.

Reading this comment without context is really confusing. Just highlight that we need to handle the GridView as a special case; including the part about the TopSites Lists being handled by HomeContextMenuInfo just adds extraneous information that is confusing.

@@ +282,5 @@
>  
>      @Override
>      public void onCreateContextMenu(ContextMenu menu, View view, ContextMenuInfo menuInfo) {
> +        // TopSitesPanel.onCreateContextMenu() is shared between the mList and mGrid views.
> +        // When a menu for an mList item is to be created, menuInfo is an instance of

In general, just referring to the variables mList and mGrid is not very specific - it's more useful to refer to them as "Top Sites list/grid item," so change that throughout.

@@ +284,5 @@
>      public void onCreateContextMenu(ContextMenu menu, View view, ContextMenuInfo menuInfo) {
> +        // TopSitesPanel.onCreateContextMenu() is shared between the mList and mGrid views.
> +        // When a menu for an mList item is to be created, menuInfo is an instance of
> +        // HomeContextMenuInfo. For mGrid, menuInfo is an instance of TopSitesGridContextMenuInfo,
> +        // as subclass of HomeContextMenuInfo.

Nit: "a subclass"

@@ +285,5 @@
> +        // TopSitesPanel.onCreateContextMenu() is shared between the mList and mGrid views.
> +        // When a menu for an mList item is to be created, menuInfo is an instance of
> +        // HomeContextMenuInfo. For mGrid, menuInfo is an instance of TopSitesGridContextMenuInfo,
> +        // as subclass of HomeContextMenuInfo.
> +        if (menuInfo == null || !(menuInfo instanceof HomeContextMenuInfo)) {

When can the menuInfo be null or not an instance of HomeContextMenuInfo?

@@ +292,5 @@
>  
> +        if (!(menuInfo instanceof TopSitesGridContextMenuInfo)) {
> +            // An mList item was long pressed as opposed to an mGrid item.
> +            // menuInfo is only an instance of HomeContextMenuInfo. HomeFragment
> +            // can handle this.

This is another potentially confusing comment referencing different classes and local variables; it's clearer to say this since this isn't a Top Sites grid item that needs a different/special context menu, the superclass can handle the context menu.

@@ +300,5 @@
>  
>          MenuInflater inflater = new MenuInflater(view.getContext());
> +        inflater.inflate(R.menu.home_contextmenu, menu);
> +
> +        // Remove top ununsed menu items.

Same as earlier comment; this also shouldn't refer to hiding "top sites" menu items.

@@ +325,5 @@
>  
>      @Override
>      public boolean onContextItemSelected(MenuItem item) {
> +        if (super.onContextItemSelected(item)) {
> +            // HomeFragment was able to handle to selected item

This is a little messy, and relies on calling into the HomeFragment method to do a check that we could do here, right? It seems like we should do the check in this method, and if we can't handle this case, just return whatever HomeFragment.onContextItemSelected returns.
Attachment #8378766 - Flags: review?(liuche) → feedback+
(In reply to Chenxia Liu [:liuche] from comment #7)
> @@ +285,5 @@
> > +        // TopSitesPanel.onCreateContextMenu() is shared between the mList and mGrid views.
> > +        // When a menu for an mList item is to be created, menuInfo is an instance of
> > +        // HomeContextMenuInfo. For mGrid, menuInfo is an instance of TopSitesGridContextMenuInfo,
> > +        // as subclass of HomeContextMenuInfo.
> > +        if (menuInfo == null || !(menuInfo instanceof HomeContextMenuInfo)) {
> 
> When can the menuInfo be null or not an instance of HomeContextMenuInfo?

Not sure actually, this was in the pre-existing code. Seemed like a safe thing to do so I left it in.

> @@ +325,5 @@
> >  
> >      @Override
> >      public boolean onContextItemSelected(MenuItem item) {
> > +        if (super.onContextItemSelected(item)) {
> > +            // HomeFragment was able to handle to selected item
> 
> This is a little messy, and relies on calling into the HomeFragment method
> to do a check that we could do here, right? It seems like we should do the
> check in this method, and if we can't handle this case, just return whatever
> HomeFragment.onContextItemSelected returns.

I think this was the reason for this patch. Works kind of like event propagation (in reverse). See of parent can handle the selected item, if not then attempt to handle it. A simple type check won't be enough to determine if HomeFragment can handle it; that depends also on what item was selected. I think its cleaner to do it this way.
(In reply to Chenxia Liu [:liuche] from comment #7)
> ::: mobile/android/base/home/TopSitesGridView.java
> @@ +132,5 @@
> > +
> > +                mContextMenuInfo = new TopSitesGridContextMenuInfo(view, position, id);
> > +                mContextMenuInfo.url = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL));
> > +                mContextMenuInfo.title = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.TITLE));
> > +                mContextMenuInfo.isPinned = ((TopSitesCursorWrapper) cursor).isPinned();
> 
> For simplicity, split this into another function.
> 
That method (makeContextMenuInfo() ?) would require too much of this scope. In addition to the cursor it would need the position and id. Should I still do it?
Looks like this bug fell off the wagon for a while. I started looking at bug 913457, and in the interest of not bit-rotting you, let's get the comments for this bug addressed and land it.

(In reply to Sola Ogunsakin [:sola] from comment #8)

> > @@ +325,5 @@
> > >  
> > >      @Override
> > >      public boolean onContextItemSelected(MenuItem item) {
> > > +        if (super.onContextItemSelected(item)) {
> > > +            // HomeFragment was able to handle to selected item
> > 
> > This is a little messy, and relies on calling into the HomeFragment method
> > to do a check that we could do here, right? It seems like we should do the
> > check in this method, and if we can't handle this case, just return whatever
> > HomeFragment.onContextItemSelected returns.
> 
> I think this was the reason for this patch. Works kind of like event
> propagation (in reverse). See of parent can handle the selected item, if not
> then attempt to handle it. A simple type check won't be enough to determine
> if HomeFragment can handle it; that depends also on what item was selected.
> I think its cleaner to do it this way.

Ok, looking through this again, I guess that makes sense.

> > ::: mobile/android/base/home/TopSitesGridView.java
> > @@ +132,5 @@
> > > +
> > > +                mContextMenuInfo = new TopSitesGridContextMenuInfo(view, position, id);
> > > +                mContextMenuInfo.url = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.URL));
> > > +                mContextMenuInfo.title = cursor.getString(cursor.getColumnIndexOrThrow(URLColumns.TITLE));
> > > +                mContextMenuInfo.isPinned = ((TopSitesCursorWrapper) cursor).isPinned();
> > 
> > For simplicity, split this into another function.
> > 
> That method (makeContextMenuInfo() ?) would require too much of this scope.
> In addition to the cursor it would need the position and id. Should I still
> do it?

You don't need to split out the creation of TopSitesGridContextMenu out, but make another private function like updateContextMenuFromCursor(TopSitesGridContextMenuInfo info, Cursor cursor).
Flags: needinfo?(oogunsakin)
Attached patch top-sites-contextmenu.patch (obsolete) — Splinter Review
Attachment #8378766 - Attachment is obsolete: true
Attachment #8402126 - Flags: review?(liuche)
Flags: needinfo?(oogunsakin)
Comment on attachment 8402126 [details] [diff] [review]
top-sites-contextmenu.patch

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

Almost! One more cycle should do it. Nits and comments.

::: mobile/android/base/home/HomeFragment.java
@@ +233,5 @@
> +     * this method returns "//www.google.com". If the passed url
> +     * is not of the user-entered scheme, the same url is returned.
> +     *
> +     * @param  url to be decoded
> +     * @return     url component of the user

Nit: match indenting

::: mobile/android/base/home/TopSitesGridView.java
@@ -261,5 @@
> -        }
> -
> -        public String getDisplayTitle() {
> -            return TextUtils.isEmpty(title) ?
> -                StringUtils.stripCommonSubdomains(StringUtils.stripScheme(url, StringUtils.UrlFlags.STRIP_HTTPS)) : title;

Remove StringUtils import since it's unused now.

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

Why return if it's not an instance of HomeContextMenuInfo? When does this happen? Need a comment.

@@ +274,3 @@
>              return;
>          }
> +        // Long pressed item was a Top Sites GridView item, handle it.

Nit: newline above.

@@ +279,2 @@
>  
> +        // Hide top ununsed menu items.

Comment seems wrong.

@@ +301,5 @@
>  
>      @Override
>      public boolean onContextItemSelected(MenuItem item) {
> +        if (super.onContextItemSelected(item)) {
> +            // HomeFragment was able to handle to selected item

Nit: period

@@ -320,5 @@
> -                flags |= Tabs.LOADURL_PRIVATE;
> -
> -            // Decode "user-entered" URLs before loading them.
> -            Tabs.getInstance().loadUrl(decodeUserEnteredUrl(info.url), flags);
> -            Toast.makeText(activity, R.string.new_tab_opened, Toast.LENGTH_SHORT).show();

Check your imports, there are some unnecessary ones now.
Attachment #8402126 - Flags: review?(liuche) → feedback+
Attached patch top-sites-contextmenu.patch (obsolete) — Splinter Review
Attachment #8402126 - Attachment is obsolete: true
Attachment #8402766 - Flags: review?(liuche)
Comment on attachment 8402766 [details] [diff] [review]
top-sites-contextmenu.patch

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

Two nits, fix those and carry over my r+.

::: mobile/android/base/home/HomeFragment.java
@@ +233,5 @@
> +     * this method returns "//www.google.com". If the passed url
> +     * does not have a user-entered scheme, the same url will be returned.
> +     *
> +     * @param  url to be decoded
> +     * @return url component of the user

"url component entered by user"

::: mobile/android/base/home/TopSitesGridView.java
@@ +127,5 @@
> +
> +                if (cursor == null) {
> +                    mContextMenuInfo = null;
> +                    return false;
> +                }

Nit: newline here
Attachment #8402766 - Flags: review?(liuche) → review+
fix nits
Attachment #8402766 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ab1487fbc9ba
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
You need to log in before you can comment on or make changes to this bug.