Merge pull request #4834 from bsclifton/fix-btb-menu-checkbox

Fix checked status on menu template (impacts Windows only)
brave · Oct 17, 2016 · a89bb7a · a89bb7a
2 parents 022fe26 + b4ecd0e
commit a89bb7a
Show file tree

Hide file tree

Showing 5 changed files with 153 additions and 50 deletions.
diff --git a/app/browser/lib/menuUtil.js b/app/browser/lib/menuUtil.js
@@ -3,6 +3,7 @@
 
 'use strict'
 
+const Immutable = require('immutable')
 const CommonMenu = require('../../common/commonMenu')
 const messages = require('../../../js/constants/messages')
 const siteTags = require('../../../js/constants/siteTags')
@@ -12,8 +13,10 @@ const locale = require('../../locale')
 
 /**
  * Get the an electron MenuItem object from a Menu based on its label
- * @param {string} label - the text associated with the menu
- * NOTE: label may be a localized string
+ *
+ * @param {Object} appMenu - the electron Menu object
+ * @param {string} label - text to search each menu item for
+ * NOTE: label is a localized string
  */
 module.exports.getMenuItem = (appMenu, label) => {
   if (appMenu && appMenu.items && appMenu.items.length > 0) {
@@ -29,6 +32,43 @@ module.exports.getMenuItem = (appMenu, label) => {
   return null
 }
 
+/**
+ * Similar to getMenuItem (above) but with a menu template. The menu template
+ * is used by our tests and also with the custom rendered Windows titlebar.
+ *
+ * @param {Object} template - object in the format which gets passed to Menu.buildFromTemplate()
+ * @param {string} label - text to search each menu item for
+ * NOTE: label is a localized string
+ */
+const getTemplateItem = (template, label) => {
+  if (template && template.length && template.length > 0) {
+    for (let i = 0; i < template.length; i++) {
+      const item = template[i]
+      if (item && item.label === label) return item
+      if (item.submenu) {
+        const nestedItem = getTemplateItem(item.submenu, label)
+        if (nestedItem) return nestedItem
+      }
+    }
+  }
+  return null
+}
+
+/**
+ * Search a menu template and update the checked status
+ *
+ * @return the new template OR null if no change was made (no update needed)
+ */
+module.exports.setTemplateItemChecked = (template, label, checked) => {
+  const menu = template.toJS()
+  const menuItem = getTemplateItem(menu, label)
+  if (menuItem.checked !== checked) {
+    menuItem.checked = checked
+    return Immutable.fromJS(menu)
+  }
+  return null
+}
+
 const createBookmarkMenuItems = (bookmarks, parentFolderId) => {
   const filteredBookmarks = parentFolderId
     ? bookmarks.filter((bookmark) => bookmark.get('parentFolderId') === parentFolderId)
@@ -66,6 +106,11 @@ const createBookmarkMenuItems = (bookmarks, parentFolderId) => {
   return payload
 }
 
+/**
+ * Add bookmarks / folders to "Bookmarks" menu
+ *
+ * @param sites The application state's Immutable sites list
+ */
 module.exports.createBookmarkMenuItems = (sites) => {
   return createBookmarkMenuItems(siteUtil.getBookmarks(sites))
 }

diff --git a/app/browser/menu.js b/app/browser/menu.js
@@ -582,13 +582,31 @@ const createMenu = () => {
   }
 }
 
+const setMenuItemChecked = (label, checked) => {
+  // Update electron menu (Mac / Linux)
+  const systemMenuItem = menuUtil.getMenuItem(appMenu, label)
+  systemMenuItem.checked = checked
+
+  // Update in-memory menu template (Windows)
+  const oldTemplate = appStore.getState().getIn(['menu', 'template'])
+  const newTemplate = menuUtil.setTemplateItemChecked(oldTemplate, label, checked)
+  if (newTemplate) {
+    appActions.setMenubarTemplate(newTemplate)
+  }
+}
+
 const doAction = (action) => {
   switch (action.actionType) {
     case windowConstants.WINDOW_SET_FOCUSED_FRAME:
-      // TODO check/uncheck menu item instead of recreating menu
+      // Update the checkbox next to "Bookmark Page" (Bookmarks menu)
       currentLocation = action.frameProps.get('location')
-      let menuItem = menuUtil.getMenuItem(appMenu, locale.translation('bookmarkPage'))
-      menuItem.checked = isCurrentLocationBookmarked()
+      setMenuItemChecked(locale.translation('bookmarkPage'), isCurrentLocationBookmarked())
+      break
+    case appConstants.APP_CHANGE_SETTING:
+      if (action.key === settings.SHOW_BOOKMARKS_TOOLBAR) {
+        // Update the checkbox next to "Bookmarks Toolbar" (Bookmarks menu)
+        setMenuItemChecked(locale.translation('bookmarksToolbar'), action.value)
+      }
       break
     case windowConstants.WINDOW_UNDO_CLOSED_FRAME:
       appDispatcher.waitFor([appStore.dispatchToken], () => {

diff --git a/app/renderer/components/menubar.js b/app/renderer/components/menubar.js
@@ -294,7 +294,8 @@ class Menubar extends ImmutableComponent {
     }
   }
   shouldComponentUpdate (nextProps, nextState) {
-    return this.props.selectedIndex !== nextProps.selectedIndex
+    return this.props.selectedIndex !== nextProps.selectedIndex ||
+      this.props.template !== nextProps.template
   }
   render () {
     let i = 0

diff --git a/js/components/addEditBookmark.js b/js/components/addEditBookmark.js
@@ -89,11 +89,12 @@ class AddEditBookmark extends ImmutableComponent {
     windowActions.setBookmarkDetail(currentDetail, this.props.originalDetail, this.props.destinationDetail)
   }
   showToolbarOnFirstBookmark () {
-    const showBookmarksToolbar = getSetting(settings.SHOW_BOOKMARKS_TOOLBAR)
-    const isFirstBookmark = this.props.sites.find(
+    const hasOneBookmark = this.props.sites.find(
       (site) => siteUtil.isBookmark(site) || siteUtil.isFolder(site)
     )
-    appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, !isFirstBookmark || showBookmarksToolbar)
+    if (!hasOneBookmark && !getSetting(settings.SHOW_BOOKMARKS_TOOLBAR)) {
+      appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, true)
+    }
   }
   onSave () {
     // First check if the title of the currentDetail is set

diff --git a/test/unit/lib/menuUtilTest.js b/test/unit/lib/menuUtilTest.js
@@ -5,56 +5,15 @@ const assert = require('assert')
 const Immutable = require('immutable')
 require('../braveUnit')
 
-const defaultMenu = {
-  items: [
-    {
-      label: 'File',
-      submenu: {
-        items: [
-          { label: 'open', temp: 1 },
-          { label: 'quit', temp: 2 }
-        ]
-      }
-    },
-    {
-      label: 'Edit',
-      submenu: {
-        items: [
-          { label: 'copy', temp: 3 },
-          { label: 'paste', temp: 4 }
-        ]
-      }
-    },
-    {
-      label: 'Bookmarks',
-      submenu: {
-        items: [
-          {
-            label: 'bookmark folder 1',
-            submenu: {
-              items: [
-                { label: 'my bookmark', url: 'https://brave.com' }
-              ]
-            }
-          }
-        ]
-      }
-    }
-  ]
-}
-
 describe('menuUtil', function () {
   let menuUtil
 
   before(function () {
-    // https://github.com/mfncooper/mockery
-    // TODO: consider moving to braveUnit.js
     mockery.enable({
       warnOnReplace: false,
       warnOnUnregistered: false,
       useCleanCache: true
     })
-
     mockery.registerMock('electron', require('./fakeElectron'))
     menuUtil = require('../../../app/browser/lib/menuUtil')
   })
@@ -64,6 +23,49 @@ describe('menuUtil', function () {
   })
 
   describe('getMenuItem', function () {
+    const defaultMenu = {
+      items: [
+        {
+          label: 'File',
+          submenu: {
+            items: [
+              { label: 'open', temp: 1 },
+              { label: 'quit', temp: 2 }
+            ]
+          }
+        },
+        {
+          label: 'Edit',
+          submenu: {
+            items: [
+              { label: 'copy', temp: 3 },
+              { label: 'paste', temp: 4 }
+            ]
+          }
+        },
+        {
+          label: 'Bookmarks',
+          submenu: {
+            items: [
+              {
+                label: 'Bookmarks Toolbar',
+                type: 'checkbox',
+                checked: false
+              },
+              {
+                label: 'bookmark folder 1',
+                submenu: {
+                  items: [
+                    { label: 'my bookmark', url: 'https://brave.com' }
+                  ]
+                }
+              }
+            ]
+          }
+        }
+      ]
+    }
+
     it('returns the electron MenuItem based on the label', function () {
       const menuItem = menuUtil.getMenuItem(defaultMenu, 'quit')
       assert.equal(menuItem.temp, 2)
@@ -78,6 +80,42 @@ describe('menuUtil', function () {
     })
   })
 
+  describe('setTemplateItemChecked', function () {
+    const defaultTemplate = Immutable.fromJS([
+      {
+        'label': 'Bookmarks',
+        'submenu': [
+          {
+            'label': 'Bookmarks Toolbar',
+            'type': 'checkbox',
+            'checked': false
+          }
+        ]
+      }
+    ])
+
+    it('returns the new template when checked status is updated', function () {
+      const expectedTemplate = Immutable.fromJS([
+        {
+          'label': 'Bookmarks',
+          'submenu': [
+            {
+              'label': 'Bookmarks Toolbar',
+              'type': 'checkbox',
+              'checked': true
+            }
+          ]
+        }
+      ])
+      const newTemplate = menuUtil.setTemplateItemChecked(defaultTemplate, 'Bookmarks Toolbar', true)
+      assert.deepEqual(newTemplate.toJS(), expectedTemplate.toJS())
+    })
+    it('returns null when no change is made', function () {
+      const newTemplate = menuUtil.setTemplateItemChecked(defaultTemplate, 'Bookmarks Toolbar', false)
+      assert.equal(newTemplate, null)
+    })
+  })
+
   describe('createBookmarkMenuItems', function () {
     it('returns an array of items w/ the bookmark tag', function () {
       const appStateSites = Immutable.fromJS([