[fig] "Remove" context menu item only tries to remove a bookmark, not history

RESOLVED FIXED in Firefox 26

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

Trunk
Firefox 26
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: abouthome-hackathon, fixed-fig)

Attachments

(1 attachment, 1 obsolete attachment)

We should figure out what we want "Remove" on the VISITED page list to do. Right now it's removing a bookmark, but we probably just want it to remove the history entry, right?
Yes, that's right.
I can take this.
Assignee: nobody → margaret.leibovic
Posted patch patch (obsolete) — Splinter Review
I initially tried splitting the bookmark/history handling into different pages, but it became a complicated mess, so I just decided to fix up the shared method instead, and I'm happy with the result.

I combined these two menuitems into one single "Remove" item, which makes things less confusing (gets rid of questions about which "Remove" menuitem is showing in any given context menu).

To make the data in HomeContextMenuInfo clearer, I decided to explicitly store a bookmarkId and a historyId, instead of just a rowId. Getting these out of the cursor is a little confusing because these can come from either the combined view on the bookmarks table. I took some notes on an etherpad to convince myself I did this correctly:
https://etherpad.mozilla.org/fig-queries

Also worth noting, I got rid of the keyword and display properties on HomeContextMenuInfo, since we're not using them right now. We don't need the keyword one anymore, since we used to use that for the "Edit" menuitem, but we now get that separately in EditBookmarkDialog. We used to use display for deciding whether or not to open reading list items in reader mode, but it looks like Capella already made a different fix for that in bug 897764!
Attachment #780729 - Flags: review?(wjohnston)
Comment on attachment 780729 [details] [diff] [review]
patch

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

::: mobile/android/base/home/HomeFragment.java
@@ +140,5 @@
>                  Toast.makeText(activity, R.string.new_tab_opened, Toast.LENGTH_SHORT).show();
>                  return true;
>              }
> +            case R.id.home_remove: {
> +                // Prioritize removing a bookmark over removing a history item in the case of a combined item.

Hm, I'm not actually sure about this now, I think we may want it to be the other way around. I'll ask Ian about this tomorrow.
Comment on attachment 780729 [details] [diff] [review]
patch

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

Drive-by:

::: mobile/android/base/home/HomeFragment.java
@@ +66,5 @@
>          inflater.inflate(R.menu.home_contextmenu, menu);
>  
> +        // Hide the "Edit" menuitem if this item isn't a bookmark.
> +        if (info.bookmarkId < 0) {
> +            menu.findItem(R.id.home_edit_bookmark).setVisible(false);            

whitespace.

@@ +161,5 @@
>          }
>          return false;
>      }
> +
> +    private void removeBookmark(final Activity activity, final int id, final String url, final boolean inReadingList) {

Using a Context feels generic than an Activity.

@@ +162,5 @@
>          return false;
>      }
> +
> +    private void removeBookmark(final Activity activity, final int id, final String url, final boolean inReadingList) {
> +        (new UiAsyncTask<Void, Void, Integer>(ThreadUtils.getBackgroundHandler()) {

This could better be a private class by itself. This big anonymous class doesn't feel so good.

@@ +175,5 @@
> +                int messageId = R.string.bookmark_removed;
> +                if (inReadingList) {
> +                    messageId = R.string.reading_list_removed;
> +
> +                    GeckoEvent e = GeckoEvent.createBroadcastEvent("Reader:Remove", url);

This entire section could be moved to doInBackground(). That would remove "aCount" from this method.

::: mobile/android/base/home/HomeListView.java
@@ +143,2 @@
>              } else {
> +                bookmarkId = cursor.getInt(bookmarkIdCol);                

whitespace.
Whiteboard: abouthome-hackathon
(In reply to :Margaret Leibovic from comment #4)
> Comment on attachment 780729 [details] [diff] [review]
> patch
> 
> Review of attachment 780729 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/home/HomeFragment.java
> @@ +140,5 @@
> >                  Toast.makeText(activity, R.string.new_tab_opened, Toast.LENGTH_SHORT).show();
> >                  return true;
> >              }
> > +            case R.id.home_remove: {
> > +                // Prioritize removing a bookmark over removing a history item in the case of a combined item.
> 
> Hm, I'm not actually sure about this now, I think we may want it to be the
> other way around. I'll ask Ian about this tomorrow.

Yeah, this should be reversed. If not, this won't properly remove history items that also happen to be bookmarked, since we're using the combined view to get history items. I think the only time we want "Remove" to remove a bookmark is when you are on the BOOKMARKS page. That would match the current behavior in the awesomescreen on m-c.
(In reply to Sriram Ramasubramanian [:sriram] from comment #5)

> @@ +161,5 @@
> >          }
> >          return false;
> >      }
> > +
> > +    private void removeBookmark(final Activity activity, final int id, final String url, final boolean inReadingList) {
> 
> Using a Context feels generic than an Activity.

We were already passing around an Activity variable, so I just continued the trend. But you're right, it looks like we don't need to be actually using an Activity, a Context would suffice.

> @@ +162,5 @@
> >          return false;
> >      }
> > +
> > +    private void removeBookmark(final Activity activity, final int id, final String url, final boolean inReadingList) {
> > +        (new UiAsyncTask<Void, Void, Integer>(ThreadUtils.getBackgroundHandler()) {
> 
> This could better be a private class by itself. This big anonymous class
> doesn't feel so good.

This is how it currently is... I feel like it's fine like this if it's only used in this one place. Although I suppose instead of creating these removeBookmark/removeHistory methods, I could just create UiAsyncTask classes that get created/executed inside onContextItemSelected. I can give that a try.

> @@ +175,5 @@
> > +                int messageId = R.string.bookmark_removed;
> > +                if (inReadingList) {
> > +                    messageId = R.string.reading_list_removed;
> > +
> > +                    GeckoEvent e = GeckoEvent.createBroadcastEvent("Reader:Remove", url);
> 
> This entire section could be moved to doInBackground(). That would remove
> "aCount" from this method.

Capella added this in bug 802093, I wonder if there is a reason why it was done this way. Cc'ing him to ask for his feedback.
(In reply to :Margaret Leibovic from comment #7)
> (In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> > @@ +162,5 @@
> > >          return false;
> > >      }
> > > +
> > > +    private void removeBookmark(final Activity activity, final int id, final String url, final boolean inReadingList) {
> > > +        (new UiAsyncTask<Void, Void, Integer>(ThreadUtils.getBackgroundHandler()) {
> > 
> > This could better be a private class by itself. This big anonymous class
> > doesn't feel so good.
> 
> This is how it currently is... I feel like it's fine like this if it's only
> used in this one place. Although I suppose instead of creating these
> removeBookmark/removeHistory methods, I could just create UiAsyncTask
> classes that get created/executed inside onContextItemSelected. I can give
> that a try.
> 

That's exactly what I recommend :)
(In reply to Sriram Ramasubramanian [:sriram] from comment #8)
> (In reply to :Margaret Leibovic from comment #7)
> > (In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> > > @@ +162,5 @@
> > > >          return false;
> > > >      }
> > > > +
> > > > +    private void removeBookmark(final Activity activity, final int id, final String url, final boolean inReadingList) {
> > > > +        (new UiAsyncTask<Void, Void, Integer>(ThreadUtils.getBackgroundHandler()) {
> > > 
> > > This could better be a private class by itself. This big anonymous class
> > > doesn't feel so good.
> > 
> > This is how it currently is... I feel like it's fine like this if it's only
> > used in this one place. Although I suppose instead of creating these
> > removeBookmark/removeHistory methods, I could just create UiAsyncTask
> > classes that get created/executed inside onContextItemSelected. I can give
> > that a try.
> > 
> 
> That's exactly what I recommend :)

One thing about this that's more annoying is that we're currently passing a bunch of parameters into removeBookmark, so I guess we'd need to set those as member variables on RemoveBookmarkTask (or whatever we name it). Is there a better way to do that? It seems annoying to need to set all those variables in the constructor.
margaret: the patch in bug 802093 was put into code in m-c and when we cloned it into HomePager for fig we just straight translated versus improving. So, no specific reason not to change it it you wish :)
Posted patch patch v2Splinter Review
I built this on top of lucasr's patch from bug 897689, since that will probably land before this.
Attachment #780729 - Attachment is obsolete: true
Attachment #780729 - Flags: review?(wjohnston)
Attachment #781110 - Flags: review?(wjohnston)
Attachment #781110 - Flags: feedback?(sriram)
Duplicate of this bug: 897705
Comment on attachment 781110 [details] [diff] [review]
patch v2

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

Overall looks good. Couple of changes with the use of Context.

::: mobile/android/base/home/HomeFragment.java
@@ +96,5 @@
>              return false;
>          }
>  
>          HomeContextMenuInfo info = (HomeContextMenuInfo) menuInfo;
> +        final Context context = (Context) getActivity();

This might not be needed. You could pass in an activity. I'm happy with the UiAsyncTask taking Context in their constructor -- which is what I wanted. The argument in the calling method can be an activity.

Even better, If you would want to use a "final Context context", I would like it to be "getActivity().getApplicationContext()". The ContentResolver is tied to ApplicationContext and the Toast is happy with ApplicationContext.

@@ +162,5 @@
> +                if (historyId > -1) {
> +                    new RemoveHistoryTask(context, historyId).execute();
> +                    return true;
> +                }
> +                final int bookmarkId = info.bookmarkId;

New line before this line.

@@ +172,5 @@
>          }
>          return false;
>      }
> +
> +    private class RemoveBookmarkTask extends UiAsyncTask<Void, Void, Void> {

Please make this a static class. No outer class members are used in this.

@@ +208,5 @@
> +            Toast.makeText(mContext, messageId, Toast.LENGTH_SHORT).show();
> +        }
> +    }
> +
> +    private class RemoveHistoryTask extends UiAsyncTask<Void, Void, Void> {

Please make this a static class. No outer class members are used in this.
Attachment #781110 - Flags: feedback?(sriram) → feedback+
(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> Comment on attachment 780729 [details] [diff] [review]
> patch
> 
> Review of attachment 780729 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive-by:
> 
> @@ +161,5 @@
> >          }
> >          return false;
> >      }
> > +
> > +    private void removeBookmark(final Activity activity, final int id, final String url, final boolean inReadingList) {
> 
> Using a Context feels generic than an Activity.

I meant using Context inside the UiAsyncTask. That's taken care of in new patch. Thanks.
Comment on attachment 781110 [details] [diff] [review]
patch v2

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

Looks good. A question about having one unified "Remove" item, but after talking that through on irc, I think I'm ok with it.

It would be good to test that hitting "home" while the dialog is open doesn't cause crashes when you return.

::: mobile/android/base/home/HomeFragment.java
@@ +189,5 @@
> +        }
> +
> +        @Override
> +        public Void doInBackground(Void... params) {
> +            BrowserDB.removeBookmark(mContext.getContentResolver(), mId);

Since you use it twice, maybe store a reference to the contentResolver here.

::: mobile/android/base/strings.xml.in
@@ +196,5 @@
>  
>    <string name="contextmenu_open_new_tab">&contextmenu_open_new_tab;</string>
>    <string name="contextmenu_open_private_tab">&contextmenu_open_private_tab;</string>
>    <string name="contextmenu_open_in_reader">&contextmenu_open_in_reader;</string>
> +  <string name="contextmenu_remove">&contextmenu_remove;</string>

Combining these strings makes me a little nervous. Do we feel confident other locales are ok using the same string for both situations?

I glanced at desktop but they seem to use "Delete" for bookmarks and "Forget" for history. i.e. it doesn't line up well with us...
Attachment #781110 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #15)

> It would be good to test that hitting "home" while the dialog is open
> doesn't cause crashes when you return.

I tested hitting home, no crash happened.

https://hg.mozilla.org/projects/fig/rev/73eef60094a0

It probably wouldn't be too hard to write some tests for this. I filed bug 899155 about that.
Whiteboard: abouthome-hackathon → abouthome-hackathon, fixed-fig
https://hg.mozilla.org/mozilla-central/rev/73eef60094a0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
You need to log in before you can comment on or make changes to this bug.