Bookmarks Menu should list the bookmarks like other browsers do

[garvankeeley]

(support request)

[bridiver]

when working on this ticket keep in mind that we will eventually want to manage the bookmarks with https://developer.chrome.com/extensions/bookmarks

[bbondy]

Here's a WIP for this task.
From here it needs:

• Rebase to work on current master tip
• Add folder support (recursive traversal of bookmarks)
• Favicons
• Apply the same thing for recent history (maybe 10 sites or so, whatever chrome does there is fine).

From: secret <void@void.void>
Date: Fri, 15 Jul 2016 11:36:46 -0700
Subject: [PATCH] bookmarks: first pass at showing bookmarks in menu

---
 app/menu.js          |  5 +++--
 js/commonMenu.js     | 24 ++++++++++++++++++++++++
 js/state/siteUtil.js |  9 +++++++++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/app/menu.js b/app/menu.js
index bb3f3b3..cc00ecf 100644
--- a/app/menu.js
+++ b/app/menu.js
@@ -426,8 +426,9 @@ const init = (settingsState, args) => {
         CommonMenu.bookmarksMenuItem(),
         CommonMenu.bookmarksToolbarMenuItem(),
         CommonMenu.separatorMenuItem,
-        CommonMenu.importBookmarksMenuItem()
-      ]
+        CommonMenu.importBookmarksMenuItem(),
+        CommonMenu.separatorMenuItem
+      ].concat(CommonMenu.createLinks(siteUtil.getBookmarks(appStore.getState().get('sites'))))
     }, {
       label: locale.translation('window'),
       role: 'window',
diff --git a/js/commonMenu.js b/js/commonMenu.js
index 1e4a007..75f9f68 100644
--- a/js/commonMenu.js
+++ b/js/commonMenu.js
@@ -250,6 +250,30 @@ module.exports.importBookmarksMenuItem = () => {
   */
 }

+module.exports.createLinks = (sites) => {
+  var payload = []
+  sites.forEach((site) => {
+    // TODO implement bookmark folders
+    // this if statement will currently ignore folders
+    // we should include folders as submenus populated with
+    // their bookmarks
+    if (site.get('location')) {
+      payload.push({
+        label: site.get('customTitle') || site.get('title'),
+        click: (item, focusedWindow) => {
+          module.exports.sendToFocusedWindow(focusedWindow, [messages.SHORTCUT_ACTIVE_FRAME_LOAD_URL, site.get('location')])
+        }
+        // TODO include label made from favicon. It needs to be of type NativeImage
+        // which can be made using a Buffer / DataURL / local image
+        // the image will likely need to be included in the site data
+        // there was potentially concern about the size of the app state
+        // and as such there may need to be another mechanism or cache
+      })
+    }
+  })
+  return payload
+}
+
 module.exports.reportAnIssueMenuItem = () => {
   return {
     label: locale.translation('reportAnIssue'),
diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js
index cd0dc17..8ffbc3f 100644
--- a/js/state/siteUtil.js
+++ b/js/state/siteUtil.js
@@ -331,6 +331,15 @@ module.exports.hasNoTagSites = function (sites) {
 }

 /**
+ * Returns all sites that have a bookmark tag.
+ * @param sites The application state's Immutabe sites list.
+ */
+
+module.exports.getBookmarks = function (sites) {
+  return sites.filter((site) => site.get('tags') && (site.get('tags').includes('bookmark') || site.get('tags').includes('bookmark-folder')))
+}
+
+/**
  * Gets a site origin (scheme + hostname + port) from a URL or null if not
  * available.
  * @param {string} location
--
2.9.0

cc @bsclifton for on route to 1.0.

[bsclifton]

Manually re-opening (my PR auto-closed it). The work is technically done, but we need a clean way to handle dynamic menu changes. Bookmarks and History menu will change often and continually calling Menu.buildFromTemplate(template) is leaking resources. The PR I had calls Menu.init whenever the appState changes, which means it'll get called more than it already has been previously.

The ideal solution would to expose a delete method in electron and then call that only for the dynamic menu items (electron already exposes a MenuItem insert)