Closed Bug 921136 Opened 7 years ago Closed 7 years ago

Unify removal behavior for bookmark-history-readinglist items from Home items

Categories

(Firefox for Android :: Awesomescreen, defect)

27 Branch
ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 32
Tracking Status
fennec + ---

People

(Reporter: aaronmt, Assigned: liuche)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 1 obsolete file)

Currently one can long tap and select 'Remove' and you get a toast saying 'Page Removed', but the result is still there as it's a bookmarked.

STR:

i) Visit http://nytimes.com
ii) Bookmark the page
iii) Tap the address bar and search up 'nytimes'
iv) Long tap on the switch to tab result and hit 'Remove'

Expected: Removes the bookmark
Actual: Removes the page history; the site remains visible because it's bookmarked so it looks like tapping 'Remove' does nothing

Not sure if we want to be more clear what exactly were removing by specifying in the context menu label (i.e, Remove Bookmark or Remove from History)
Doesn't need to be a page currently open.
Summary: Unclear what 'Remove' does on a bookmarked and open AwesomeScreen search result → Unclear what 'Remove' does on a bookmarked AwesomeScreen search result
To anyone who's curious, this isn't a regression, it's what we do on the awesomescreen in release/beta.
Blocks: 931021
According to ibarlow's mockup in bug 931021, we should make this remove item remove both the history entry and the bookmark.

I'm tempted to dupe this to bug 913457, but let's keep them separate for now. That bug can be about thumbnail context menu items, and this one can be about list items.

Ian, question: What should the toast say in the different remove cases? What about this:

1) Item is only a history entry -> "Page removed"
2) Item is only a bookmark -> "Bookmark removed"
3) Item is both a history entry *and* a bookmark -> "Bookmark removed"

I feel like the bookmark being removed is more important info than the history entry being removed. Do you think users would be surprised if we removed a history item when removing a bookmark? For simplicity's sake, I like having one "Remove" item that removes everything about the site, regardless of what about:home page you're on.

Or maybe the toast should always just say "Page removed"?
Assignee: nobody → margaret.leibovic
Flags: needinfo?(ibarlow)
(In reply to :Margaret Leibovic from comment #3)
> According to ibarlow's mockup in bug 931021, we should make this remove item
> remove both the history entry and the bookmark.
> 
> I'm tempted to dupe this to bug 913457, but let's keep them separate for
> now. That bug can be about thumbnail context menu items, and this one can be
> about list items.
> 
> Ian, question: What should the toast say in the different remove cases? What
> about this:
> 
> 1) Item is only a history entry -> "Page removed"
> 2) Item is only a bookmark -> "Bookmark removed"
> 3) Item is both a history entry *and* a bookmark -> "Bookmark removed"
> 
> I feel like the bookmark being removed is more important info than the
> history entry being removed. Do you think users would be surprised if we
> removed a history item when removing a bookmark? For simplicity's sake, I
> like having one "Remove" item that removes everything about the site,
> regardless of what about:home page you're on.
> 
> Or maybe the toast should always just say "Page removed"?

I prefer to keep it simple with "Page removed" everywhere, all the time.
Flags: needinfo?(ibarlow)
(In reply to Ian Barlow (:ibarlow) from comment #4)
> (In reply to :Margaret Leibovic from comment #3)
> > According to ibarlow's mockup in bug 931021, we should make this remove item
> > remove both the history entry and the bookmark.
> > 
> > I'm tempted to dupe this to bug 913457, but let's keep them separate for
> > now. That bug can be about thumbnail context menu items, and this one can be
> > about list items.
> > 
> > Ian, question: What should the toast say in the different remove cases? What
> > about this:
> > 
> > 1) Item is only a history entry -> "Page removed"
> > 2) Item is only a bookmark -> "Bookmark removed"
> > 3) Item is both a history entry *and* a bookmark -> "Bookmark removed"
> > 
> > I feel like the bookmark being removed is more important info than the
> > history entry being removed. Do you think users would be surprised if we
> > removed a history item when removing a bookmark? For simplicity's sake, I
> > like having one "Remove" item that removes everything about the site,
> > regardless of what about:home page you're on.
> > 
> > Or maybe the toast should always just say "Page removed"?
> 
> I prefer to keep it simple with "Page removed" everywhere, all the time.

Works for me! Makes writing the patch easier :)
I kept the reading_list_removed and bookmark_removed strings, because they're used in other toast messages from the reader mode toolbar and the app menu star.

I decided to change the entity name to make the code clearer, but also to give localizers a chance to update the string, since the meaning has changed.
Attachment #829014 - Flags: review?(wjohnston)
Attachment #829014 - Flags: review?(wjohnston) → review+
I realized this fix isn't quite totally correct... on the bookmarks page, we only query the bookmarks table, not the combined view, so we won't remove history entries on that page.
Status: NEW → ASSIGNED
Attachment #829014 - Flags: review+
I haven't had time to work on this myself, so making into a mentor bug.

The patch in here is likely bitrotted by now, but it also needs work to make it correct.
Assignee: margaret.leibovic → nobody
Whiteboard: [mentor=margaret][lang=java]
Duplicate of this bug: 996312
This should be implemented per comment #3.
Status: ASSIGNED → NEW
Summary: Unclear what 'Remove' does on a bookmarked AwesomeScreen search result → Fix behavior for selecting "Remove" for a combined bookmark-history item from Home items
tracking-fennec: --- → +
Ian, I took a look at the mocks, and I think that removing a history/bookmark item => removing the item from the reading list seems a little unexpected. What do you think?
Flags: needinfo?(ibarlow)
Mm, I think I disagree. It strikes me that if I have a thing that I saved, and I hit "remove", I really mean "remove" it from Firefox. 

The less-than-ideal alternative is leaving users to hunt and peck around the app, wondering if they have actually removed the page from everywhere...
Flags: needinfo?(ibarlow)
Summary: Fix behavior for selecting "Remove" for a combined bookmark-history item from Home items → Unify removal behavior for bookmark-history-readinglist items from Home items
Unfortunately, this will still double notify if removing a bookmark/history item that also happens to be a reading list item because Reading List removal waits for a Gecko message (which gets handled in BrowserApp).
Assignee: nobody → liuche
Attachment #829014 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8422832 - Flags: review?(margaret.leibovic)
Comment on attachment 8422832 [details] [diff] [review]
Patch: Unify removing bookmark/history/reading list

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

The reading list message dance looks like it has some problems, but let's file a follow-up bug to address that.

::: mobile/android/base/home/HomeFragment.java
@@ +206,5 @@
>              if (info.hasBookmarkId()) {
> +                new RemoveBookmarkTask(context, info.bookmarkId, !notifyQueued).execute();
> +                notifyQueued = true;
> +            } else {
> +                new RemoveBookmarkTask(context, url, false).execute();

This could be confusing, because it will remove any bookmark with that URL (e.g. ones in different folders), but that's what the bookmark star does now, so it's not unprecedented.

It's hard to have a perfect solution here, but at least we can be consistent if we just go with the "I want this thing gone from Firefox" approach.

@@ +345,3 @@
>  
>              GeckoEvent e = GeckoEvent.createBroadcastEvent("Reader:Remove", mUrl);
>              GeckoAppShell.sendEventToGecko(e);

I think we should just move the notification to here. The only reason we need to send a message to gecko is to remove the article from the cache. In fact, the current logic is kinda wrong, because even if we fail to remove the article from the cache, we're still removing the item from the list, so as far as the user is concerned, it's removed. Actually, looking at the current logic, we call BrowserDB.removeReadinglistItem twice in this case!

I want us to clean up this logic, but let's do it in a follow-up bug, since it looks like there could be a few things at play here.

::: mobile/android/base/locales/en-US/android_strings.dtd
@@ +280,5 @@
>  <!ENTITY pref_scroll_title_bar "Scroll title bar">
>  
> +<!-- Localization note (page_removed): This string appears in a toast message when
> +     any page is removed frome about:home. This includes pages that are in history,
> +     bookmarks, or reading list. -->

Nice l10n note.
Attachment #8422832 - Flags: review?(margaret.leibovic) → review+
> ::: mobile/android/base/home/HomeFragment.java
> @@ +206,5 @@
> >              if (info.hasBookmarkId()) {
> > +                new RemoveBookmarkTask(context, info.bookmarkId, !notifyQueued).execute();
> > +                notifyQueued = true;
> > +            } else {
> > +                new RemoveBookmarkTask(context, url, false).execute();
> 
> This could be confusing, because it will remove any bookmark with that URL
> (e.g. ones in different folders), but that's what the bookmark star does
> now, so it's not unprecedented.
> 
> It's hard to have a perfect solution here, but at least we can be consistent
> if we just go with the "I want this thing gone from Firefox" approach.

Right, I was following ibarlow's comment #12 on removing everything everywhere so a user doesn't need to peck through all their home panels to make sure something is actually gone. I suppose the "bookmarks in different folders" could be a problem though, but we can talk about that later, I suppose.
> 
> @@ +345,3 @@
> >  
> >              GeckoEvent e = GeckoEvent.createBroadcastEvent("Reader:Remove", mUrl);
> >              GeckoAppShell.sendEventToGecko(e);
> 
> I think we should just move the notification to here. The only reason we
> need to send a message to gecko is to remove the article from the cache. In
> fact, the current logic is kinda wrong, because even if we fail to remove
> the article from the cache, we're still removing the item from the list, so
> as far as the user is concerned, it's removed. Actually, looking at the
> current logic, we call BrowserDB.removeReadinglistItem twice in this case!
> 
> I want us to clean up this logic, but let's do it in a follow-up bug, since
> it looks like there could be a few things at play here.
> 
Filed: bug 1011714.
https://hg.mozilla.org/integration/fx-team/rev/53ba60264695
Target Milestone: --- → Firefox 31
https://hg.mozilla.org/mozilla-central/rev/53ba60264695
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 31 → Firefox 32
Depends on: 1024365
You need to log in before you can comment on or make changes to this bug.