Closed Bug 913457 Opened 7 years ago Closed 6 years ago

Add a "Remove" context menu item to top sites thumbnails on about:home

Categories

(Firefox for Android :: Awesomescreen, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
fennec + ---

People

(Reporter: kats, Assigned: liuche)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mentor=liuche][lang=java])

Attachments

(3 files, 6 obsolete files)

So when the new about:home came about, there was one site that I had apparently bookmarked a long time ago but totally forgotten about that kept showing up on my bookmarks tab thumbnails (I only have like 3 bookmarks, including that one). To unbookmark it I had to actually visit the page and then un-star it from the menu. Now that bookmarks are more promimently shown on about:home via the thumbnails, it would be nice if the context menu for those items allowed the user to un-bookmark the item. I note that the line items below the thumbnails do have a "Remove" context menu item.
Having 'Remove' and 'Unpin' options in the same menu might be a bit confusing. I'm personally finding the current thumbnails behaviour a bit confusing with the unpinned spots being auto-filled with top bookmarks.

This space was created to give users control over what's displayed on startup (mostly). Maybe auto-filling the thumbnails with top bookmarks conflicts a bit with that original intent? If the idea with the auto-filling is to avoid empty state, maybe we should simply pin some bookmarks by default?

Ian, thoughts?
Flags: needinfo?(ibarlow)
Blocks: 931021
According to ibarlow's mockup in bug 931021 [1], we should make a "Remove" context menu item for thumbnails that removes both the history entry and the bookmark.

I'm going to work on doing this.

[1] https://bug931021.bugzilla.mozilla.org/attachment.cgi?id=822334
Assignee: nobody → margaret.leibovic
Duplicate of this bug: 927249
Summary: You should be able to un-bookmark things from the new about:home bookmarks screen → Add a "Remove" context menu item to top sites thumbnails on about:home
tracking-fennec: --- → ?
Bug 943408 has a big nag about wanting something like this
tracking-fennec: ? → 29+
Duplicate of this bug: 950040
I haven't had time to work on this.
Assignee: margaret.leibovic → nobody
Flags: needinfo?(ibarlow)
Whiteboard: [mentor=liuche][lang=java]
> tracking-fennec: 29+

Not happening.
If we really want this, we could have this track 31, but I don't think we should try to uplift it. At this point, this might be more of a tracking-fennec:+
tracking-fennec: 29+ → ?
tracking-fennec: ? → +
Attached patch Part 0: Cleanup unused var (obsolete) — Splinter Review
These patches depend on bug 996850.
Assignee: nobody → liuche
Status: NEW → ASSIGNED
Attachment #8407921 - Flags: review?(oogunsakin)
Attachment #8407922 - Flags: review?(oogunsakin)
Attachment #8407925 - Flags: review?(oogunsakin)
This makes bug 856565 and bug 921136 more obvious. I guess I should fix those too.
Depends on: 996850
Attachment #8407921 - Flags: review?(oogunsakin) → review+
Attachment #8407922 - Flags: review?(oogunsakin) → review+
Comment on attachment 8407925 [details] [diff] [review]
Part 2: Handle unpinning removed sites

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

Looks good but when I try removing a pinned item it only unpins it. If I hit remove a second time it then goes away.

Looked into it briefly, looks like for pinned items historyId is null even when its in your history. So the history remove operation fails.
Attachment #8407925 - Flags: review?(oogunsakin) → feedback+
(In reply to Sola Ogunsakin [:sola] from comment #13)
> Comment on attachment 8407925 [details] [diff] [review]
> Part 2: Handle unpinning removed sites
> 
> Review of attachment 8407925 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good but when I try removing a pinned item it only unpins it. If I hit
> remove a second time it then goes away.
> 
> Looked into it briefly, looks like for pinned items historyId is null even
> when its in your history. So the history remove operation fails.

That's bug 921136 :/
...or actually, maybe not!
Unbitrot.
Attachment #8407922 - Attachment is obsolete: true
Attachment #8422447 - Flags: review+
Attachment #8422447 - Attachment description: 1-topsites-remove → Part 1: Add "Remove" to top sites context menu
I still need to finish the patch for removing Suggested Sites.
Attachment #8407925 - Attachment is obsolete: true
Attachment #8422449 - Flags: review?(lucasr.at.mozilla)
One more extraneous import.
Attachment #8407921 - Attachment is obsolete: true
Attachment #8422558 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8422449 [details] [diff] [review]
Part 2: Handle unpinning removed sites

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

Needs some clarifications before giving r+.

::: mobile/android/base/home/HomeFragment.java
@@ +208,3 @@
>              }
>  
>              if (info.hasBookmarkId()) {

You should only start this async task if handle is still false at this point, no? Or is the intention here to remove both? If that's the case, it would probably be better if we could trigger only one asynctask to remove both history and bookmark entries.
Attachment #8422449 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8422558 [details] [diff] [review]
Part 0: Cleanup unused code

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

Nice.
Attachment #8422558 - Flags: review?(lucasr.at.mozilla) → review+
> 
> Needs some clarifications before giving r+.
> 
> ::: mobile/android/base/home/HomeFragment.java
> @@ +208,3 @@
> >              }
> >  
> >              if (info.hasBookmarkId()) {
> 
> You should only start this async task if handle is still false at this
> point, no? Or is the intention here to remove both? If that's the case, it
> would probably be better if we could trigger only one asynctask to remove
> both history and bookmark entries.

The intent is to remove both of them (as well as from the reading list, if it's present there), so I made an async task that removes from all of them.

I'm not sure what the benefit of removing by id is vs removing by url. I suppose that if you've bookmarked the same page in multiple folders, those would all be removed, but this is consistent with "remove once, remove everywhere."
Attachment #8422447 - Attachment is obsolete: true
Attachment #8424243 - Flags: review?(lucasr.at.mozilla)
Attachment #8422447 - Attachment is obsolete: false
Comment on attachment 8422449 [details] [diff] [review]
Part 2: Handle unpinning removed sites

(oops, obsoleted wrong patch)
Attachment #8422449 - Attachment is obsolete: true
Comment on attachment 8424243 [details] [diff] [review]
Part 2: Handle unpinning removed sites v2

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

Looking good. Slightly mixed about deleting both bookmark and history entry on remove. I wonder if users will find this behaviour a bit unexpected. I'd like to see a final version of this patch and questions answered before giving r+.

::: mobile/android/base/home/HomeFragment.java
@@ +200,5 @@
>  
> +            // Removing a Top Sites grid item also requires unpinning, so for Top Sites
> +            // grid items, don't consume the ContextMenu selection for history/bookmarks.
> +            boolean handled = false;
> +            boolean ret = menuInfo instanceof TopSitesGridContextMenuInfo ? false : true;

This can probably be simplified a bit. Having separate 'ret' and 'handled' is a bit confusing.

@@ +290,5 @@
>          @Override
>          public Void doInBackground(Void... params) {
>              ContentResolver cr = mContext.getContentResolver();
> +
> +            BrowserDB.removeBookmarksWithURL(cr, mUrl);

nit: remove this empty line.

@@ +296,5 @@
> +            BrowserDB.removeHistoryEntry(cr, mUrl);
> +
> +            BrowserDB.removeReadingListItemWithURL(cr, mUrl);
> +            GeckoEvent e = GeckoEvent.createBroadcastEvent("Reader:Remove", mUrl);
> +            GeckoAppShell.sendEventToGecko(e);

Hmm, what is the URL is not a reading list item? Does it show the toast notification even if the deleted URL is not a reading list?
Attachment #8424243 - Flags: review?(lucasr.at.mozilla) → feedback+
(In reply to Lucas Rocha (:lucasr) from comment #23)
> Comment on attachment 8424243 [details] [diff] [review]
> Part 2: Handle unpinning removed sites v2
> 
> Review of attachment 8424243 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good. Slightly mixed about deleting both bookmark and history entry
> on remove. I wonder if users will find this behaviour a bit unexpected. I'd
> like to see a final version of this patch and questions answered before
> giving r+.
> 

The intent behind removing everything at once is so that users don't have to search everywhere for something. I kind of feel like "Delete" would be a better term for this "remove everywhere," but we could see how this goes.
https://bugzilla.mozilla.org/show_bug.cgi?id=921136#c12

> ::: mobile/android/base/home/HomeFragment.java
> @@ +200,5 @@
> >  
> > +            // Removing a Top Sites grid item also requires unpinning, so for Top Sites
> > +            // grid items, don't consume the ContextMenu selection for history/bookmarks.
> > +            boolean handled = false;
> > +            boolean ret = menuInfo instanceof TopSitesGridContextMenuInfo ? false : true;
> 
> This can probably be simplified a bit. Having separate 'ret' and 'handled'
> is a bit confusing.
> 

Done, moved check for Top Sites grid item (and comment) to later.

> 
> @@ +296,5 @@
> > +            BrowserDB.removeHistoryEntry(cr, mUrl);
> > +
> > +            BrowserDB.removeReadingListItemWithURL(cr, mUrl);
> > +            GeckoEvent e = GeckoEvent.createBroadcastEvent("Reader:Remove", mUrl);
> > +            GeckoAppShell.sendEventToGecko(e);
> 
> Hmm, what is the URL is not a reading list item? Does it show the toast
> notification even if the deleted URL is not a reading list?

It does - the toast notification is shown in in onPostExecute. This actually means that for Reading List items, we show the "Page removed" toast twice.  The handling of Reading List items isn't actually correct because we do some Reading List message handling in BrowserApp that isn't necessary (and redundant even), but bug 1011714 is for fixing that.
Attachment #8425011 - Flags: review?(lucasr.at.mozilla)
Attachment #8424243 - Attachment is obsolete: true
Comment on attachment 8425011 [details] [diff] [review]
Part 2: Handle unpinning removed sites v3

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

This looks fine but I'd really like to get rid of this level indirection between the unpin and remove actions. I'm fine with doing it in a follow-up. Up to you.

::: mobile/android/base/home/HomeFragment.java
@@ +206,5 @@
>  
> +            if (handled) {
> +                // Removing a Top Sites grid item also requires unpinning, so for Top Sites
> +                // grid items, don't consume the ContextMenu selection for history/bookmarks.
> +                if (menuInfo instanceof TopSitesGridContextMenuInfo) {

I'd prefer to simply call BrowserDB.unpinSite() directly in RemoveItemByUrlTask() and be done with it instead of relying on not consuming the action here. This level of indirection is bug-prone. For example, someone might decide to change how TopSitesPanel handles unpin in the future and unintentionally break the 'Remove' behaviour here.
Attachment #8425011 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Lucas Rocha (:lucasr) from comment #25)
> Comment on attachment 8425011 [details] [diff] [review]
> Part 2: Handle unpinning removed sites v3
> 
> Review of attachment 8425011 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine but I'd really like to get rid of this level indirection
> between the unpin and remove actions. I'm fine with doing it in a follow-up.
> Up to you.

That's a good point. I added that to this patch - this actually makes things much cleaner, too!
Attachment #8425011 - Attachment is obsolete: true
Attachment #8425790 - Flags: review+
Target Milestone: Firefox 31 → Firefox 32
Remove option is present in the context menu on top sites thumbnails and removes the page.
Verified fixed on:
Device: LG Nexus 4 (Android 4.4.2)
Build: Firefox for Android 32.0a1 (2014-05-25)

Choosing to remove the suggested tiles: Mozilla or Marketplace, a toast notification appears "Page removed" but the pages are still displayed. Is it expected?
I filed bug 1016363. I don't believe there's been any consideration yet for how remove will work on suggested tiles. Marking this verified based on how the interaction works on non-suggested based tiles.
Status: RESOLVED → VERIFIED
Depends on: 1016363
(In reply to Lucas Rocha (:lucasr) from comment #23)
> Comment on attachment 8424243 [details] [diff] [review]
> Part 2: Handle unpinning removed sites v2
> 
> Review of attachment 8424243 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good. Slightly mixed about deleting both bookmark and history entry
> on remove. I wonder if users will find this behaviour a bit unexpected.

Speaking from an user perspective, I agree.
Just because I want to remove an item from the Top Sites page that doesn't mean that I'd necessarily want to unbookmark it as well.
Similar to how on Desktop, I can remove a tile from about:newtab without having to automagically unbookmark it at the same time.

Besides, the list items below the thumbnails already offer the remove action, and that currently doesn't remove any bookmarks. If that pre-existing behaviour gets changed without any notice, it might lead to users inadvertently deleting bookmarks they in fact wanted to keep. 

On a side note, I'm a bit confused about the references that unpinned thumbnails spots are only being populated by 'top bookmarks'. I'm using the current release version (FF 30) and at least two of my thumbnails are occupied by pages which haven't been bookmarked, only frequently visited in the recent time.
> If that pre-existing behaviour gets changed without any notice, it might lead to
> users inadvertently deleting bookmarks they in fact wanted to keep.

Hmm, so that is exactly what we have implemented in bug 921136. This is intentional, and part of the attached mocks for the metabug (bug 931021).

JanH, if you don't mind, can you explain why you feel like "Remove" should behave the way it does (remove the bookmark/history item) but not "Remove" the item from Firefox completely? How about from the bookmarks panel, should hitting "Remove" delete the item from History? What about from the history panel, and hitting "Remove" on a bookmarked item - should that remove the bookmark? Thanks!

(As an FYI, the behavior that's in release isn't exactly correct - if you hit "Remove" for a bookmarked-history item in the Top Sites list in 30, it doesn't necessarily remove the item from the list, but just moves it further down - bug 996312.)
Flags: needinfo?(jh+bugzilla)
(In reply to Chenxia Liu [:liuche] from comment #32)

Hmm, good question. I guess my expectations come at least partly from the way Desktop Firefox behaves.
There, as far as deleting items is concerned, history, bookmarks and about:newtab seem to be kept more or less separately, i.e. deleting an item in one of those categories doesn't affect its presence in one of the other two categories. So for a start, I'd expect things to work similarly on Mobile.

I can see the point behind providing an option to remove an item from *everywhere*, however personally, I've rarely, if ever, felt a need for it.
Looking at it, I think my main reason for wanting to delete items from the Top Sites so far was removing pages I only occasionally visit on Mobile, but which are pushed onto the Top Sites list through Sync because I use them quite frequently on Desktop. In those cases, I wanted to declutter the Top Sites list, so it would only show frequently visited pages relevant to my mobile usage, however *without* removing its history or bookmark entry, because occasionally, I might still want to visit that page on Mobile as well.
There might be other cases where I'd want to delete a page from Top Sites because I didn't want to wait until it would finally get pushed down the list by other entries, but nevertheless would want to keep the bookmark - or possibly even the history entry, I'm not really sure about that in this case - for reference purposes and possible future usage.

As far as deleting history items is concerned, I rarely do that, so I can't really say what my expectations would be, except that I'd again probably feel a bit uneasy about simultaneously deleting its bookmark entries if present.

In the case of deleting bookmarks, while in some way unbookmarking a page might signify that I don't need it any more, so far I haven't felt a need to complain about the fact that unbookmarking a page leaves its history entry intact. In fact, having the history entry still present provides a kind of backup in case I erroneously unbookmarked a paged, or changed my mind later on.
If I really never used that page again, it would get pushed of the history list eventually anyway, and if I was really desperate about removing that page, I'd simply explicitly delete its history entry as well - though I can see the value a 'Remove entry everywhere' button would provide in that case.
Flags: needinfo?(jh+bugzilla)
Duplicate of this bug: 1042721
Depends on: 1062257
You need to log in before you can comment on or make changes to this bug.