Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fix checked status on menu template (impacts Windows only) #4834

Merged
merged 1 commit into from
Oct 17, 2016
Merged

Fix checked status on menu template (impacts Windows only) #4834

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 47 additions & 2 deletions app/browser/lib/menuUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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) {
Expand All @@ -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)
Expand Down Expand Up @@ -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))
}
Expand Down
24 changes: 21 additions & 3 deletions app/browser/menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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], () => {
Expand Down
3 changes: 2 additions & 1 deletion app/renderer/components/menubar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
7 changes: 4 additions & 3 deletions js/components/addEditBookmark.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be cleaner to call this hasBookmarks since hasOneBookmark can be true when there are 2 bookmarks.

appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, true)
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes were made to prevent appActions.changeSetting() from being called every time. Now it will only call if this is your first bookmark and toolbar is not already visible

onSave () {
// First check if the title of the currentDetail is set
Expand Down
120 changes: 79 additions & 41 deletions test/unit/lib/menuUtilTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
})
Expand All @@ -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)
Expand All @@ -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([
Expand Down