Closed Bug 1219804 Opened 9 years ago Closed 8 years ago

Show last 5 recent bookmarks in the bookmarks menu

Categories

(Firefox :: Bookmarks & History, defect, P2)

All
Unspecified
defect

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox47 --- disabled
firefox48 --- fixed
firefox49 --- verified

People

(Reporter: verdi, Assigned: dao)

References

(Depends on 1 open bug)

Details

(Whiteboard: [Onboarding])

Attachments

(2 files, 3 obsolete files)

Attached image recent-bm-in-menu.png
When someone creates a bookmark, they often follow the jumping star animation and check to see their newly created bookmark in the bookmarks menu. But because the default location of bookmarks is the unsorted bookmarks subfolder they are not immediately visible. User tests show that this is very confusing to people.

We can fix this without reworking the bookmarks IA if we display the recently bookmarked items directly inline in the menu instead of in a flyout panel. We should limit this to 5 items to avoid making the menu any taller than we have to.
Blocks: 1219810
Priority: -- → P1
Whiteboard: [Onboarding]
We can't just "move" the "Recently Bookmarked" menu contents to the top level because "Recently Bookmarked" is actually a smart folder in the "Bookmarks Menu" category. I guess we'd have to remove that folder in a migration step when people upgrade. Are we sure we want to go down this route?
Flags: needinfo?(mverdi)
Flags: needinfo?(mak77)
I don't think that's the major problem from a technical point of view. We can either remove it with ui migration or just leave it there for existing profiles and let users remove it manually. This is the easy part.

The problem here is that currently we assume 1 Places view <=> 1 popup. to properly support drag&drop, icons and contextual options we'd need to make popups allow multiple views, and that may be non-trivial, most of menu.xml, BrowserPlacesViews and controller.js assume 1:1 relation.
The current views assume the viewElt is the popup, we'd need to change that to an element inside the popup instead. Then we'd need to fix menu.xml to not assume there's only one view (no _rootView), and fix drag&drop in it and the static part building code. This may require a new view implementation in browserPlacesViews.js. There would be no more a _placesView property on this popup and I'm not sure how many things this would break. The only good thing is that the arrow panel menu already has a separate view from other popups (PlacesPanelMenuView), maybe it could become sort of a multiview forwarding to subviews? no idea.
Otherwise, it may be possible to try creating a subview inside a main view, managed by its own view code in browserPlacesViews. Everything would stay the same for the main view, but we'd have this view inside a view. It may be simpler, but I'm not sure how many details will not work without trying.

The final alternative would be to hardcode bookmarks in the menu, like we did for the view in the history widget (for whatever reason that still escapes me). It would be less work, but they would have a limited functionality compared to the rest of the menu, and we'd have some duped code.
Flags: needinfo?(mak77)
Jared, you mentioned you had a work-in-progress patch for this. Could you please attach it? It might give me a head start even if it's a rough patch.
Flags: needinfo?(mverdi) → needinfo?(jaws)
Attached patch Rough WIP (obsolete) — Splinter Review
Flags: needinfo?(jaws)
Dao, any update on this? If you're not able to keep working on this can you post your WIP?
Flags: needinfo?(dao)
Attached patch WIP (obsolete) — Splinter Review
Does this look sane to you? It only works for the bookmarks button menu, not for the menu bar's bookmarks menu which I'm guessing uses a separate implementation. I'm not sure if this bug was supposed to cover the menu bar in the first place; comment 0 isn't explicit about it. I suppose we'll want to keep both menus somewhat consistent, though.
Assignee: nobody → dao
Attachment #8709560 - Attachment is obsolete: true
Flags: needinfo?(dao)
Attachment #8717461 - Flags: feedback?(mak77)
Comment on attachment 8717461 [details] [diff] [review]
WIP

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

one thing the users would likely want to do is to drag & drop these items to the menu or the toolbar. That is likely worth a follow-up. In this first implementation they should not be draggable at all, cause otherwise we will end up creating copies of the bookmarks. I don't know off-hand how hard it will be to properly support moving, it will require populating the dataTransfer with fake Places nodes, and convince other views they can move the bookmarks, not just copy them.

::: browser/base/content/browser-places.js
@@ +1336,5 @@
>        },
>        insertionPoint: ".panel-subview-footer"
>      });
> +
> +    this._insertRecentBookmarks();

I assume we need to update these items everytime the menu is opened, while this is updating only when the bookmarks view is created the first time.
Attachment #8717461 - Flags: feedback?(mak77) → feedback+
Attached patch patchSplinter Review
Attachment #8717461 - Attachment is obsolete: true
Attachment #8717817 - Flags: review?(mak77)
Comment on attachment 8717817 [details] [diff] [review]
patch

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

please file a follow-up bug for D&D.

::: browser/base/content/browser-places.js
@@ +1345,5 @@
> +    const kMaxResults = 5;
> +
> +    let options = PlacesUtils.history.getNewQueryOptions();
> +    options.excludeQueries = true;
> +    options.includeHidden = false;

false is the default value, no need to set it

@@ +1346,5 @@
> +
> +    let options = PlacesUtils.history.getNewQueryOptions();
> +    options.excludeQueries = true;
> +    options.includeHidden = false;
> +    options.resultType = options.RESULTS_AS_URI;

it's the default as well (feel free to update the idl if it doesn't state that)
Attachment #8717817 - Flags: review?(mak77) → review+
Flags: qe-verify+
https://hg.mozilla.org/mozilla-central/rev/2ba1faa2dcf9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Attached image useless-bookmarks.png (obsolete) —
It's not working. It shows five bookmarks but not the last bookmarks but old bookmarks and not the same bookmarks as in "Recently Bookmarked"… and there is no preference to disable the five random (?) bookmarks…
Reopened because the patch does something different than the bug description.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
As usual, please file a new bug and mark it blocking this one.
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Depends on: 1248258
Depends on: 1248259
Depends on: 1248266
Depends on: 1248267
Attachment #8719280 - Attachment is obsolete: true
You are showing the last 5 opened bookmarks and *not* the last 5 bookmarked links.
Maybe this should be reopened.
Depends on: 1248268
Depends on: 1248616
Depends on: 1248617
that's already fixed in bug 1248258.
(In reply to Worcester12345 from comment #18)
> (In reply to Marco Bonardo [::mak] from comment #17)
> > that's already fixed in bug 1248258.
> 
> Perhaps the bug owner should be able to chime in first that it is working.
> Verdi?

This is not representative of how we work. Marco is a fine representative, and as the reviewer on bug 1248258 he is more than sufficient in answering that this has been fixed. Please move comment specific to bug 1248258 to that bug, and make sure that you are testing with a build that includes the changes that landed in that bug.
Depends on: 1249473
Adding :mboldan to the CC list, since he manually tested this area in the past.
I found an new issue related to the new "recently bookmarked" section. bug 1250203
Depends on: 1250203
Depends on: 1250828
Depends on: 1253930
No longer depends on: 1253930
Depends on: 1253996
Depends on: 1254960
[Tracking Requested - why for this release]:
It has some obvious blocks.
(In reply to YF (Yang) from comment #22)
> [Tracking Requested - why for this release]:
> It has some obvious blocks.

It was backed out from 47 in bug 1254960.
Target Milestone: Firefox 47 → Firefox 48
Depends on: 1267939
I managed to verify this issue on Firefox 49.0a1 (2016-05-05) and I confirm that the last 5 recent bookmarks are correctly displayed in the bookmarks menu.
The tests were performed on Windows 10 x64, Mac OS X 10.11.1 and on Ubuntu 14.04 x86.
I am marking this issue Verified-Fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1326707
You need to log in before you can comment on or make changes to this bug.