From 2e238dc9b05ccd6fdf2f2877ba851928b0bbc915 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Fri, 12 Aug 2016 09:16:22 -0700 Subject: [PATCH 1/6] - Brand new about:history page. It's very minimal but works (you can double click entries to launch), - History menu now has a "Show Full History" which pulls up about:history. Fixes https://github.com/brave/browser-laptop/issues/444 - History in back/forward nav buttons is limited to 10. Fixes https://github.com/brave/browser-laptop/issues/2889 - History menu shows recently closed frames (based on WindowState) - Broke methods out from menu.js to a helper file (menuUtil.js) - menuUtil tests now use mockery (and work properly), stubbing electron (we can also unit test other electron dependent code like this). - history and bookmarks menus are now dynamic and uses clear/insert calls instead of rebuilding all menu items. Fixes https://github.com/brave/browser-laptop/issues/1993 - history menu now considers modifier keys (similar to bookmarks menu) --- app/extensions/brave/about-history.html | 20 ++ .../brave/locales/en-US/history.properties | 2 + .../brave/locales/en-US/menu.properties | 4 +- app/index.js | 20 +- app/locale.js | 2 + app/menu.js | 195 +++++++------- js/about/entry.js | 3 + js/about/history.js | 128 +++++++++ js/commonMenu.js | 59 ++-- js/components/frame.js | 4 + js/components/main.js | 3 + js/constants/config.js | 3 +- js/constants/messages.js | 4 + js/contextMenus.js | 34 ++- js/entry.js | 4 + js/lib/appUrlUtil.js | 2 +- js/lib/menuUtil.js | 166 ++++++++++++ less/about/history.less | 47 ++++ package.json | 1 + test/unit/braveUnit.js | 1 - test/unit/lib/menuUtilTest.js | 255 ++++++++++++++++++ 21 files changed, 799 insertions(+), 158 deletions(-) create mode 100644 app/extensions/brave/about-history.html create mode 100644 app/extensions/brave/locales/en-US/history.properties create mode 100644 js/about/history.js create mode 100644 js/lib/menuUtil.js create mode 100644 less/about/history.less create mode 100644 test/unit/lib/menuUtilTest.js diff --git a/app/extensions/brave/about-history.html b/app/extensions/brave/about-history.html new file mode 100644 index 00000000000..b8ea8b77eef --- /dev/null +++ b/app/extensions/brave/about-history.html @@ -0,0 +1,20 @@ + + + + + + + + + + + + + + + +
+ + diff --git a/app/extensions/brave/locales/en-US/history.properties b/app/extensions/brave/locales/en-US/history.properties new file mode 100644 index 00000000000..e89afaabd05 --- /dev/null +++ b/app/extensions/brave/locales/en-US/history.properties @@ -0,0 +1,2 @@ +historyTitle=History +history=History diff --git a/app/extensions/brave/locales/en-US/menu.properties b/app/extensions/brave/locales/en-US/menu.properties index 2137b03fe07..a29832e1479 100644 --- a/app/extensions/brave/locales/en-US/menu.properties +++ b/app/extensions/brave/locales/en-US/menu.properties @@ -43,10 +43,12 @@ home=Home back=Back forward=Forward reopenLastClosedWindow=Reopen Last Closed Window -showAllHistory=Show All History +showAllHistory=Show Full History clearHistory=Clear History... clearCache=Clear Cache... clearSiteData=Clear All Cookies and Site Data... +recentlyClosed=Recently Closed +recentlyVisited=Recently Visited bookmarks=Bookmarks addToFavoritesBar=Add to Favorites Bar window=Window diff --git a/app/index.js b/app/index.js index 9bb3b764fa2..8ca9268a911 100644 --- a/app/index.js +++ b/app/index.js @@ -294,6 +294,13 @@ app.on('ready', () => { saveIfAllCollected() }) + // Window state must be fetched from main process; this is fired once it's retrieved + ipcMain.on(messages.RESPONSE_WINDOW_STATE_FOR_MENU, (wnd, data) => { + if (data) { + Menu.init(AppStore.getState(), data) + } + }) + ipcMain.on(messages.LAST_WINDOW_STATE, (wnd, data) => { if (data) { lastWindowState = data @@ -367,8 +374,11 @@ app.on('ready', () => { // reset the browser window. This will default to en-US if // not yet configured. locale.init(initialState.settings[settings.LANGUAGE], (strings) => { - // Initialize after localization strings async loaded - Menu.init(AppStore.getState().get('settings'), AppStore.getState().get('sites')) + if (BrowserWindow.getFocusedWindow()) { + BrowserWindow.getFocusedWindow().webContents.send(messages.REQUEST_WINDOW_STATE_FOR_MENU) + } else { + Menu.init(AppStore.getState(), null) + } }) // Do this after loading the state @@ -523,7 +533,11 @@ app.on('ready', () => { // save app state every 5 minutes regardless of update frequency setInterval(initiateSessionStateSave, 1000 * 60 * 5) AppStore.addChangeListener(() => { - Menu.init(AppStore.getState().get('settings'), AppStore.getState().get('sites')) + if (BrowserWindow.getFocusedWindow()) { + BrowserWindow.getFocusedWindow().webContents.send(messages.REQUEST_WINDOW_STATE_FOR_MENU) + } else { + Menu.init(AppStore.getState(), null) + } }) let masterKey diff --git a/app/locale.js b/app/locale.js index 1569f2dae8f..c5bac0f84a9 100644 --- a/app/locale.js +++ b/app/locale.js @@ -110,6 +110,8 @@ var rendererIdentifiers = function () { 'clearCache', 'clearHistory', 'clearSiteData', + 'recentlyClosed', + 'recentlyVisited', 'bookmarks', 'addToFavoritesBar', 'window', diff --git a/app/menu.js b/app/menu.js index a2c57888b09..bb8c7c543cf 100644 --- a/app/menu.js +++ b/app/menu.js @@ -7,11 +7,12 @@ const electron = require('electron') const appConfig = require('../js/constants/appConfig') const Menu = electron.Menu +const MenuItem = electron.MenuItem const messages = require('../js/constants/messages') const settings = require('../js/constants/settings') const dialog = electron.dialog const appActions = require('../js/actions/appActions') -// const siteUtil = require('../js/state/siteUtil') +const menuUtil = require('../js/lib/menuUtil') const getSetting = require('../js/settings').getSetting const locale = require('./locale') const isDarwin = process.platform === 'darwin' @@ -21,14 +22,8 @@ const aboutUrl = 'https://brave.com/' let appMenu = Menu.buildFromTemplate([]) Menu.setApplicationMenu(appMenu) -// Static menu definitions (initialized once in createMenu()) -let fileSubmenu, editSubmenu, viewSubmenu, braverySubmenu, windowSubmenu, helpSubmenu, debugSubmenu - -// States which can trigger dynamic menus to change -let lastSettingsState, lastSites - // Used to hold the default value for "isBookmarked" (see createBookmarksSubmenu) -let initBookmarkChecked = false +let isBookmarkChecked = false // Submenu initialization const createFileSubmenu = (CommonMenu) => { @@ -290,7 +285,7 @@ const createViewSubmenu = (CommonMenu) => { } const createHistorySubmenu = (CommonMenu) => { - return [ + let submenu = [ { label: locale.translation('home'), accelerator: 'CmdOrCtrl+Shift+H', @@ -349,6 +344,20 @@ const createHistorySubmenu = (CommonMenu) => { } } ] + + submenu = submenu.concat(menuUtil.createRecentlyClosedMenuItems()) + + submenu.push( + // TODO: recently visited + // CommonMenu.separatorMenuItem, + // { + // label: locale.translation('recentlyVisited'), + // enabled: false + // }, + // CommonMenu.separatorMenuItem, + CommonMenu.historyMenuItem()) + + return submenu } const createBookmarksSubmenu = (CommonMenu) => { @@ -357,7 +366,7 @@ const createBookmarksSubmenu = (CommonMenu) => { label: locale.translation('bookmarkPage'), type: 'checkbox', accelerator: 'CmdOrCtrl+D', - checked: initBookmarkChecked, // NOTE: checked status is updated via updateBookmarkedStatus() + checked: isBookmarkChecked, // NOTE: checked status is updated via updateBookmarkedStatus() click: function (item, focusedWindow) { var msg = item.checked ? messages.SHORTCUT_ACTIVE_FRAME_REMOVE_BOOKMARK @@ -374,13 +383,9 @@ const createBookmarksSubmenu = (CommonMenu) => { CommonMenu.bookmarksManagerMenuItem(), CommonMenu.bookmarksToolbarMenuItem(), CommonMenu.separatorMenuItem, - CommonMenu.importBookmarksMenuItem() - ] - // TODO: commented out temporarily. - // Needs to be changed to update existing menu, not rebuild all menus (even if some are cached). - // - // ,CommonMenu.separatorMenuItem - // ].concat(CommonMenu.createBookmarkMenuItems(siteUtil.getBookmarks(lastSites))) + CommonMenu.importBookmarksMenuItem(), + CommonMenu.separatorMenuItem + ].concat(menuUtil.createBookmarkMenuItems()) } const createWindowSubmenu = (CommonMenu) => { @@ -497,89 +502,29 @@ const createDebugSubmenu = (CommonMenu) => { } /** - * Get the electron MenuItem object based on its label - * @param {string} label - the text associated with the menu - * NOTE: label may be a localized string - */ -const getMenuItem = (label) => { - if (appMenu && appMenu.items && appMenu.items.length > 0) { - for (let i = 0; i < appMenu.items.length; i++) { - const menuItem = appMenu.items[i].submenu.items.find(function (item) { - return item.label === label - }) - if (menuItem) return menuItem - } - } - return null -} - -/** - * Called from navigationBar.js; used to update bookmarks menu status - * @param {boolean} isBookmarked - true if the currently viewed site is bookmarked - */ -const updateBookmarkedStatus = (isBookmarked) => { - const menuItem = getMenuItem(locale.translation('bookmarkPage')) - if (menuItem) { - menuItem.checked = isBookmarked - } - // menu may be rebuilt without the location changing - // this holds the last known status - initBookmarkChecked = isBookmarked -} - -/** - * Check for uneeded updates. - * Updating the menu when it is not needed causes the menu to close if expanded - * and also causes menu clicks to not work. So we don't want to update it a lot - * when app state changes, like when there are downloads. - * NOTE: settingsState is not used directly; it gets used indirectly via getSetting() - * @param {} - */ -const isUpdateNeeded = (settingsState, sites) => { - let stateChanged = false - if (settingsState !== lastSettingsState) { - lastSettingsState = settingsState - stateChanged = true - } - if (sites !== lastSites) { - lastSites = sites - stateChanged = true - } - return stateChanged -} - -/** - * Will only build the static items once - * Dynamic items (Bookmarks, History) are built each time + * Will only build the initial menu, which is mostly static items + * Dynamic items (Bookmarks, History) get updated w/ updateMenu */ const createMenu = (CommonMenu) => { - if (!fileSubmenu) { fileSubmenu = createFileSubmenu(CommonMenu) } - if (!editSubmenu) { editSubmenu = createEditSubmenu(CommonMenu) } - if (!viewSubmenu) { viewSubmenu = createViewSubmenu(CommonMenu) } - if (!braverySubmenu) { - braverySubmenu = [ - CommonMenu.braveryGlobalMenuItem(), - CommonMenu.braverySiteMenuItem() - ] - } - if (!windowSubmenu) { windowSubmenu = createWindowSubmenu(CommonMenu) } - if (!helpSubmenu) { helpSubmenu = createHelpSubmenu(CommonMenu) } - - // Creation of the menu. Notice Bookmarks and History are created each time. const template = [ - { label: locale.translation('file'), submenu: fileSubmenu }, - { label: locale.translation('edit'), submenu: editSubmenu }, - { label: locale.translation('view'), submenu: viewSubmenu }, + { label: locale.translation('file'), submenu: createFileSubmenu(CommonMenu) }, + { label: locale.translation('edit'), submenu: createEditSubmenu(CommonMenu) }, + { label: locale.translation('view'), submenu: createViewSubmenu(CommonMenu) }, { label: locale.translation('history'), submenu: createHistorySubmenu(CommonMenu) }, { label: locale.translation('bookmarks'), submenu: createBookmarksSubmenu(CommonMenu) }, - { label: locale.translation('bravery'), submenu: braverySubmenu }, - { label: locale.translation('window'), submenu: windowSubmenu, role: 'window' }, - { label: locale.translation('help'), submenu: helpSubmenu, role: 'help' } + { + label: locale.translation('bravery'), + submenu: [ + CommonMenu.braveryGlobalMenuItem(), + CommonMenu.braverySiteMenuItem() + ] + }, + { label: locale.translation('window'), submenu: createWindowSubmenu(CommonMenu), role: 'window' }, + { label: locale.translation('help'), submenu: createHelpSubmenu(CommonMenu), role: 'help' } ] if (process.env.NODE_ENV === 'development') { - if (!debugSubmenu) { debugSubmenu = createDebugSubmenu(CommonMenu) } - template.push({ label: 'Debug', submenu: debugSubmenu }) + template.push({ label: 'Debug', submenu: createDebugSubmenu(CommonMenu) }) } if (isDarwin) { @@ -625,35 +570,75 @@ const createMenu = (CommonMenu) => { const oldMenu = appMenu appMenu = Menu.buildFromTemplate(template) Menu.setApplicationMenu(appMenu) - oldMenu.destroy() + if (oldMenu) { + oldMenu.destroy() + } +} + +const updateMenu = (CommonMenu, appState, windowState) => { + const updated = menuUtil.checkForUpdate(appState, windowState) + if (updated.nothingUpdated) { + return + } + + // Only update menu when necessary + + if (updated.settings || updated.closedFrames) { + console.log('rebuilding history menu') + let historyMenu = menuUtil.getParentMenuDetails(appMenu, locale.translation('history')) + if (historyMenu && historyMenu.menu && historyMenu.menu.submenu && historyMenu.index !== -1) { + const menu = historyMenu.menu.submenu + const menuItems = createHistorySubmenu(CommonMenu) + menu.clear() + menuItems.forEach((item) => menu.append(new MenuItem(item))) + } + } + + if (updated.sites) { + console.log('rebuilding bookmarks menu') + let bookmarksMenu = menuUtil.getParentMenuDetails(appMenu, locale.translation('bookmarks')) + if (bookmarksMenu && bookmarksMenu.menu && bookmarksMenu.menu.submenu && bookmarksMenu.index !== -1) { + const menu = bookmarksMenu.menu.submenu + const menuItems = createBookmarksSubmenu(CommonMenu) + menu.clear() + menuItems.forEach((item) => menu.append(new MenuItem(item))) + } + } + + Menu.setApplicationMenu(appMenu) } /** * Sets up the menu. - * @param {Object} settingsState - Application settings state - * @param {List} - list of siteDetails + * @param {Object} appState - Application state + * @param {Object} windowState - Current window state */ -const init = (settingsState, sites) => { +module.exports.init = (appState, windowState) => { // The menu will always be called once localization is done // so don't bother loading anything until it is done. if (!locale.initialized) { return } - if (!isUpdateNeeded(settingsState, sites)) { - return - } - // This needs to be within the init method to handle translations const CommonMenu = require('../js/commonMenu') - - // Only rebuild menu if it doesn't already exist (prevent leaking resources). if (appMenu.items.length === 0) { createMenu(CommonMenu) + } else { + updateMenu(CommonMenu, appState, windowState) } } -module.exports = { - init, - updateBookmarkedStatus +/** + * Called from navigationBar.js; used to update bookmarks menu status + * @param {boolean} isBookmarked - true if the currently viewed site is bookmarked + */ +module.exports.updateBookmarkedStatus = (isBookmarked) => { + const menuItem = menuUtil.getMenuItem(locale.translation('bookmarkPage')) + if (menuItem) { + menuItem.checked = isBookmarked + } + // menu may be rebuilt without the location changing + // this holds the last known status + isBookmarkChecked = isBookmarked } diff --git a/js/about/entry.js b/js/about/entry.js index 0dd4a66a30d..8891698c559 100644 --- a/js/about/entry.js +++ b/js/about/entry.js @@ -35,6 +35,9 @@ switch (getBaseUrl(getSourceAboutUrl(window.location.href))) { break case 'about:flash': element = require('./flashPlaceholder') + break + case 'about:history': + element = require('./history') } if (element) { diff --git a/js/about/history.js b/js/about/history.js new file mode 100644 index 00000000000..5523bd2e381 --- /dev/null +++ b/js/about/history.js @@ -0,0 +1,128 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +// Note that these are webpack requires, not CommonJS node requiring requires +const React = require('react') +const Immutable = require('immutable') +const Sticky = require('react-stickynode') +const ImmutableComponent = require('../components/immutableComponent') +const messages = require('../constants/messages') +const aboutActions = require('./aboutActions') + +const ipc = window.chrome.ipc + +// Stylesheets +require('../../less/about/itemList.less') +require('../../less/about/history.less') +require('../../node_modules/font-awesome/css/font-awesome.css') + +class HistoryItem extends ImmutableComponent { + navigate () { + aboutActions.newFrame({ + location: this.props.history.get('location'), + partitionNumber: this.props.history.get('partitionNumber') + }) + } + render () { + // Figure out the partition info display + let partitionNumberInfo + if (this.props.history.get('partitionNumber')) { + let l10nArgs = { + partitionNumber: this.props.history.get('partitionNumber') + } + partitionNumberInfo = +  () + } + + var className = 'listItem' + + // If the history item is in the selected folder, show + // it as selected + if (this.props.inSelectedFolder) { + className += ' selected' + } + + return
+ { + this.props.history.get('customTitle') || this.props.history.get('title') + ? + {new Date(this.props.history.get('lastAccessedTime')).toLocaleDateString()} + {this.props.history.get('customTitle') || this.props.history.get('title')} + {partitionNumberInfo} + -{this.props.history.get('location')} + + : + {new Date(this.props.history.get('lastAccessedTime')).toLocaleDateString()} + {this.props.history.get('location')} + {partitionNumberInfo} + + } +
+ } +} + +class HistoryList extends ImmutableComponent { + render () { + return + { + this.props.history.map((entry) => + ) + } + + } +} + +class AboutHistory extends React.Component { + constructor () { + super() + this.onChangeSelectedEntry = this.onChangeSelectedEntry.bind(this) + this.onChangeSearch = this.onChangeSearch.bind(this) + this.onClearSearchText = this.onClearSearchText.bind(this) + this.state = { + history: Immutable.Map(), + selectedEntry: 0, + search: '' + } + ipc.on(messages.HISTORY_UPDATED, (e, detail) => { + this.setState({ + history: Immutable.fromJS(detail && detail.history || {}) + }) + }) + } + onChangeSelectedEntry (id) { + this.setState({ + selectedEntry: id, + search: '' + }) + } + onChangeSearch (evt) { + this.setState({ + search: evt.target.value + }) + } + onClearSearchText (evt) { + this.setState({ + search: '' + }) + } + render () { + return
+

+ +
+ + site.get('tags').isEmpty())} + onChangeSelectedEntry={this.onChangeSelectedEntry} + selectedEntry={this.state.selectedEntry} /> + +
+

+ } +} + +module.exports = diff --git a/js/commonMenu.js b/js/commonMenu.js index c137263fe58..82ea01c4ec0 100644 --- a/js/commonMenu.js +++ b/js/commonMenu.js @@ -13,9 +13,6 @@ const settings = require('./constants/settings') const getSetting = require('./settings').getSetting const issuesUrl = 'https://github.com/brave/browser-laptop/issues' const isDarwin = process.platform === 'darwin' -const siteTags = require('./constants/siteTags') -const siteUtil = require('./state/siteUtil') -const eventUtil = require('./lib/eventUtil') let electron try { @@ -194,6 +191,22 @@ module.exports.bookmarksManagerMenuItem = () => { } } +module.exports.historyMenuItem = () => { + return { + label: locale.translation('showAllHistory'), + accelerator: 'CmdOrCtrl+Y', + click: function (item, focusedWindow) { + if (BrowserWindow.getAllWindows().length === 0) { + appActions.newWindow(Immutable.fromJS({ + location: 'about:history' + })) + } else { + module.exports.sendToFocusedWindow(focusedWindow, [messages.SHORTCUT_NEW_FRAME, 'about:history', { singleFrame: true }]) + } + } + } +} + module.exports.downloadsMenuItem = () => { return { label: locale.translation('downloadsManager'), @@ -253,46 +266,6 @@ module.exports.importBookmarksMenuItem = () => { */ } -module.exports.createBookmarkMenuItems = (bookmarks, parentFolderId) => { - let filteredBookmarks - if (parentFolderId) { - filteredBookmarks = bookmarks.filter((bookmark) => bookmark.get('parentFolderId') === parentFolderId) - } else { - filteredBookmarks = bookmarks.filter((bookmark) => !bookmark.get('parentFolderId')) - } - - var payload = [] - filteredBookmarks.forEach((site) => { - if (site.get('tags').includes(siteTags.BOOKMARK) && site.get('location')) { - payload.push({ - // 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 - // - // see: https://github.com/brave/browser-laptop/issues/3050 - label: site.get('customTitle') || site.get('title'), - click: (item, focusedWindow, e) => { - if (eventUtil.isForSecondaryAction(e)) { - module.exports.sendToFocusedWindow(focusedWindow, [messages.SHORTCUT_NEW_FRAME, site.get('location'), { openInForeground: !!e.shiftKey }]) - } else { - module.exports.sendToFocusedWindow(focusedWindow, [messages.SHORTCUT_ACTIVE_FRAME_LOAD_URL, site.get('location')]) - } - } - }) - } else if (siteUtil.isFolder(site)) { - const folderId = site.get('folderId') - const submenuItems = bookmarks.filter((bookmark) => bookmark.get('parentFolderId') === folderId) - payload.push({ - label: site.get('customTitle') || site.get('title'), - submenu: submenuItems.count() > 0 ? module.exports.createBookmarkMenuItems(bookmarks, folderId) : null - }) - } - }) - return payload -} - module.exports.reportAnIssueMenuItem = () => { return { label: locale.translation('reportAnIssue'), diff --git a/js/components/frame.js b/js/components/frame.js index 83a217d0a65..146cd6be7d5 100644 --- a/js/components/frame.js +++ b/js/components/frame.js @@ -85,6 +85,10 @@ class Frame extends ImmutableComponent { bookmarks: this.props.bookmarks.toJS(), bookmarkFolders: this.props.bookmarkFolders.toJS() }) + } else if (location === 'about:history') { + this.webview.send(messages.HISTORY_UPDATED, { + history: this.props.history.toJS() + }) } else if (location === 'about:downloads') { this.webview.send(messages.DOWNLOADS_UPDATED, { downloads: this.props.downloads.toJS() diff --git a/js/components/main.js b/js/components/main.js index f3dc402509a..68b55dda0d4 100644 --- a/js/components/main.js +++ b/js/components/main.js @@ -894,6 +894,9 @@ class Main extends ImmutableComponent { .filter((site) => site.get('tags') .includes(siteTags.BOOKMARK)) || emptyMap : null} + history={frame.get('location') === 'about:history' + ? this.props.appState.get('sites') || emptyMap + : null} downloads={this.props.appState.get('downloads') || emptyMap} bookmarkFolders={frame.get('location') === 'about:bookmarks' ? this.props.appState.get('sites') diff --git a/js/constants/config.js b/js/constants/config.js index 9e57191313b..7d6008a8a84 100644 --- a/js/constants/config.js +++ b/js/constants/config.js @@ -31,7 +31,8 @@ module.exports = { maxTopSites: 5 }, navigationBar: { - defaultSearchSuggestions: false + defaultSearchSuggestions: false, + maxHistorySites: 10 }, defaultOpenSearchPath: 'content/search/google.xml', vault: { diff --git a/js/constants/messages.js b/js/constants/messages.js index 4a3911f770b..f313dc5f9f1 100644 --- a/js/constants/messages.js +++ b/js/constants/messages.js @@ -110,6 +110,9 @@ const messages = { RESPONSE_WINDOW_STATE: _, LAST_WINDOW_STATE: _, UNDO_CLOSED_WINDOW: _, + // Menu rebuilding + REQUEST_WINDOW_STATE_FOR_MENU: _, + RESPONSE_WINDOW_STATE_FOR_MENU: _, // Ad block, safebrowsing, and tracking protection BLOCKED_RESOURCE: _, BLOCKED_PAGE: _, @@ -118,6 +121,7 @@ const messages = { SITE_SETTINGS_UPDATED: _, BRAVERY_DEFAULTS_UPDATED: _, BOOKMARKS_UPDATED: _, + HISTORY_UPDATED: _, DOWNLOADS_UPDATED: _, FLASH_UPDATED: _, // About pages from contentScript diff --git a/js/contextMenus.js b/js/contextMenus.js index c2dadfdad2e..3a1d11b7cf0 100644 --- a/js/contextMenus.js +++ b/js/contextMenus.js @@ -33,6 +33,7 @@ const {getBase64FromImageUrl} = require('./lib/imageUtil') const urlParse = require('url').parse const eventUtil = require('./lib/eventUtil') const currentWindow = require('../app/renderer/currentWindow') +const config = require('./constants/config') const isDarwin = process.platform === 'darwin' @@ -915,7 +916,6 @@ function mainTemplateInit (nodeProps, frame) { template.push( CommonMenu.separatorMenuItem, { - // TODO: use locale.translate label: passwordManager.get('displayName'), click: (item, focusedWindow) => { if (focusedWindow) { @@ -954,6 +954,8 @@ function onHamburgerMenu (location, e) { function onMainContextMenu (nodeProps, frame, contextMenuType) { if (contextMenuType === 'bookmark' || contextMenuType === 'bookmark-folder') { onBookmarkContextMenu(Immutable.fromJS(nodeProps), Immutable.fromJS({ location: '', title: '', partitionNumber: frame.get('partitionNumber') })) + } else if (contextMenuType === 'history') { + // TODO: add new onHistoryContextMenu() and associated methods. } else if (contextMenuType === 'download') { onDownloadsToolbarContextMenu(nodeProps.downloadId, Immutable.fromJS(nodeProps)) } else { @@ -1054,7 +1056,8 @@ function onBackButtonHistoryMenu (activeFrame, history, rect) { const menuTemplate = [] if (activeFrame && history) { - for (let index = (history.currentIndex - 1); index > -1; index--) { + const stopIndex = Math.max((history.currentIndex - config.navigationBar.maxHistorySites), -1) + for (let index = (history.currentIndex - 1); index > stopIndex; index--) { const url = history.entries[index].url menuTemplate.push({ @@ -1072,6 +1075,18 @@ function onBackButtonHistoryMenu (activeFrame, history, rect) { } }) } + + if (stopIndex !== -1) { + menuTemplate.push( + CommonMenu.separatorMenuItem, + { + label: locale.translation('showAllHistory'), + click: (e, focusedWindow) => { + windowActions.newFrame({ location: 'about:history' }) + windowActions.setContextMenuDetail() + } + }) + } } windowActions.setContextMenuDetail(Immutable.fromJS({ @@ -1085,7 +1100,8 @@ function onForwardButtonHistoryMenu (activeFrame, history, rect) { const menuTemplate = [] if (activeFrame && history) { - for (let index = (history.currentIndex + 1); index < history.entries.length; index++) { + const stopIndex = Math.min((history.currentIndex + config.navigationBar.maxHistorySites), history.entries.length) + for (let index = (history.currentIndex + 1); index < stopIndex; index++) { const url = history.entries[index].url menuTemplate.push({ @@ -1103,6 +1119,18 @@ function onForwardButtonHistoryMenu (activeFrame, history, rect) { } }) } + + if (stopIndex !== history.entries.length) { + menuTemplate.push( + CommonMenu.separatorMenuItem, + { + label: locale.translation('showAllHistory'), + click: (e, focusedWindow) => { + windowActions.newFrame({ location: 'about:history' }) + windowActions.setContextMenuDetail() + } + }) + } } windowActions.setContextMenuDetail(Immutable.fromJS({ diff --git a/js/entry.js b/js/entry.js index 87ba4e89050..45bed601ab7 100644 --- a/js/entry.js +++ b/js/entry.js @@ -53,6 +53,10 @@ ipc.on(messages.REQUEST_WINDOW_STATE, () => { ipc.send(messages.RESPONSE_WINDOW_STATE, windowStore.getState().toJS()) }) +ipc.on(messages.REQUEST_WINDOW_STATE_FOR_MENU, () => { + ipc.send(messages.RESPONSE_WINDOW_STATE_FOR_MENU, windowStore.getState().toJS()) +}) + if (process.env.NODE_ENV === 'test') { window.appStoreRenderer = appStoreRenderer window.windowActions = require('./actions/windowActions') diff --git a/js/lib/appUrlUtil.js b/js/lib/appUrlUtil.js index b002ad8d7b3..da24c34e1ba 100644 --- a/js/lib/appUrlUtil.js +++ b/js/lib/appUrlUtil.js @@ -75,7 +75,7 @@ module.exports.isIntermediateAboutPage = (location) => ['about:safebrowsing', 'about:error', 'about:certerror'].includes(getBaseUrl(location)) module.exports.isNotImplementedAboutPage = (location) => - ['about:config', 'about:history'].includes(getBaseUrl(location)) + ['about:config'].includes(getBaseUrl(location)) module.exports.isNavigatableAboutPage = (location) => !module.exports.isIntermediateAboutPage(location) && !module.exports.isNotImplementedAboutPage(location) && !['about:newtab', 'about:blank', 'about:flash'].includes(getBaseUrl(location)) diff --git a/js/lib/menuUtil.js b/js/lib/menuUtil.js new file mode 100644 index 00000000000..29a5af22573 --- /dev/null +++ b/js/lib/menuUtil.js @@ -0,0 +1,166 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +'use strict' + +const CommonMenu = require('../commonMenu') +const messages = require('../constants/messages') +const siteTags = require('../constants/siteTags') +const eventUtil = require('./eventUtil') +const siteUtil = require('../state/siteUtil') +const locale = require('../../app/locale') + +// States which can trigger dynamic menus to change +let lastSettingsState, lastSites, lastClosedFrames + +/** + * Get an electron MenuItem object for the PARENT menu (File, Edit, etc) based on its label + * @param {string} label - the text associated with the menu + * NOTE: label may be a localized string + */ +module.exports.getParentMenuDetails = (appMenu, label) => { + let menuIndex = -1 + let menuItem = null + + if (label && appMenu && appMenu.items && appMenu.items.length > 0) { + menuIndex = appMenu.items.findIndex(function (item, index) { + return item && item.label === label + }) + + if (menuIndex !== -1) { + menuItem = appMenu.items[menuIndex] + } + } + + return { + menu: menuItem, + index: menuIndex + } +} + +/** + * 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 + */ +module.exports.getMenuItem = (appMenu, label) => { + if (appMenu && appMenu.items && appMenu.items.length > 0) { + for (let i = 0; i < appMenu.items.length; i++) { + const menuItem = appMenu.items[i].submenu && appMenu.items[i].submenu.items.find(function (item) { + return item && item.label === label + }) + if (menuItem) return menuItem + } + } + return null +} + +/** + * Check for uneeded updates. + * Updating the menu when it is not needed causes the menu to close if expanded + * and also causes menu clicks to not work. So we don't want to update it a lot. + * Should only be updated when appState or windowState change (for history or bookmarks) + * NOTE: settingsState is not used directly; it gets used indirectly via getSetting() + * @param {Object} appState - Application state + * @param {Object} windowState - Current window state + */ +module.exports.checkForUpdate = (appState, windowState) => { + const updated = { + nothing: true, + settings: false, + sites: false, + closedFrames: false + } + + if (appState && appState.get('settings') !== lastSettingsState) { + // Currently only used for the HOMEPAGE value (bound to history menu) + lastSettingsState = appState.get('settings') + updated.nothing = false + updated.settings = true + } + + if (appState && appState.get('sites') !== lastSites) { + lastSites = appState.get('sites') + updated.nothing = false + updated.sites = true + } + + if (windowState && windowState.closedFrames !== lastClosedFrames) { + lastClosedFrames = windowState.closedFrames + updated.nothing = false + updated.closedFrames = true + } + + return updated +} + +const createBookmarkMenuItems = (bookmarks, parentFolderId) => { + const filteredBookmarks = parentFolderId + ? bookmarks.filter((bookmark) => bookmark.get('parentFolderId') === parentFolderId) + : bookmarks.filter((bookmark) => !bookmark.get('parentFolderId')) + + const payload = [] + filteredBookmarks.forEach((site) => { + if (site.get('tags').includes(siteTags.BOOKMARK) && site.get('location')) { + payload.push({ + // 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 + // + // see: https://github.com/brave/browser-laptop/issues/3050 + label: site.get('customTitle') || site.get('title'), + click: (item, focusedWindow, e) => { + if (eventUtil.isForSecondaryAction(e)) { + CommonMenu.sendToFocusedWindow(focusedWindow, [messages.SHORTCUT_NEW_FRAME, site.get('location'), { openInForeground: !!e.shiftKey }]) + } else { + CommonMenu.sendToFocusedWindow(focusedWindow, [messages.SHORTCUT_ACTIVE_FRAME_LOAD_URL, site.get('location')]) + } + } + }) + } else if (siteUtil.isFolder(site)) { + const folderId = site.get('folderId') + const submenuItems = bookmarks.filter((bookmark) => bookmark.get('parentFolderId') === folderId) + payload.push({ + label: site.get('customTitle') || site.get('title'), + submenu: submenuItems.count() > 0 ? createBookmarkMenuItems(bookmarks, folderId) : null + }) + } + }) + return payload +} + +module.exports.createBookmarkMenuItems = () => { + if (lastSites) { + return createBookmarkMenuItems(siteUtil.getBookmarks(lastSites)) + } + return [] +} + +module.exports.createRecentlyClosedMenuItems = () => { + const payload = [] + if (lastClosedFrames && lastClosedFrames.length > 0) { + payload.push( + CommonMenu.separatorMenuItem, + { + label: locale.translation('recentlyClosed'), + enabled: false + }) + + const lastTen = (lastClosedFrames.size < 10) ? lastClosedFrames : lastClosedFrames.slice(-10) + lastTen.forEach((closedFrame) => { + payload.push({ + label: closedFrame.title, + click: (item, focusedWindow, e) => { + if (eventUtil.isForSecondaryAction(e)) { + CommonMenu.sendToFocusedWindow(focusedWindow, [messages.SHORTCUT_NEW_FRAME, closedFrame.location, { openInForeground: !!e.shiftKey }]) + } else { + CommonMenu.sendToFocusedWindow(focusedWindow, [messages.SHORTCUT_ACTIVE_FRAME_LOAD_URL, closedFrame.location]) + } + } + }) + }) + } + return payload +} diff --git a/less/about/history.less b/less/about/history.less new file mode 100644 index 00000000000..bbe9c24260e --- /dev/null +++ b/less/about/history.less @@ -0,0 +1,47 @@ +@import "./itemList.less"; + +.historyPage { + margin: 20px; + + .historyPageContent { + border-top: 1px solid @chromeBorderColor; + display: flex; + + .historyList { + padding-top: 10px; + overflow: hidden; + + .listItem { + display: flex; + height: 1rem; + + .aboutListItem { + align-items: center; + } + + .aboutItemDate { + color: #aaa; + margin-right: 10px; + } + } + } + + .sticky-outer-wrapper { + min-width: 100%; + padding-top: 10px; + } + } +} + +.searchInput { + float: right; + padding: 5px; + margin-top: -35px; +} + +.searchInputClear { + float: right; + padding: 8px; + margin-top: -39px; + color: #999; +} diff --git a/package.json b/package.json index 9ff0f8d3e7f..31b4be6e332 100644 --- a/package.json +++ b/package.json @@ -127,6 +127,7 @@ "less-loader": "^2.2.1", "mkdirp": "^0.5.1", "mocha": "^2.3.4", + "mockery": "^1.7.0", "ncp": "^2.0.0", "node-gyp": "^3.2.1", "node-libs-browser": "^1.0.0", diff --git a/test/unit/braveUnit.js b/test/unit/braveUnit.js index 98a30f160d5..bbd0e636af8 100644 --- a/test/unit/braveUnit.js +++ b/test/unit/braveUnit.js @@ -1,2 +1 @@ require('jsdom-global')() -require('babel-polyfill') diff --git a/test/unit/lib/menuUtilTest.js b/test/unit/lib/menuUtilTest.js new file mode 100644 index 00000000000..1044e30d8cb --- /dev/null +++ b/test/unit/lib/menuUtilTest.js @@ -0,0 +1,255 @@ +/* global describe, before, after, it */ +const siteTags = require('../../../js/constants/siteTags') +const mockery = require('mockery') +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 } + ] + } + } + ] +} + +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 + }) + + const fakeElectron = { + ipcMain: { + on: function () { } + }, + remote: { + app: { } + }, + app: { } + } + + mockery.registerMock('electron', fakeElectron) + menuUtil = require('../../../js/lib/menuUtil') + }) + + after(function () { + mockery.disable() + }) + + describe('getParentMenuDetails', function () { + const emptyValue = { + menu: null, + index: -1 + } + it('returns an object with the electron MenuItem/index based on the label', function () { + const menu = menuUtil.getParentMenuDetails(defaultMenu, 'Edit') + assert.equal(menu.index, 1) + assert.equal(menu.menu, defaultMenu.items[1]) + }) + it('returns an object with null/-1 if input menu is not truthy', function () { + const menu = menuUtil.getParentMenuDetails(null, 'Edit') + assert.deepEqual(menu, emptyValue) + }) + it('returns an object with null/-1 if label is not truthy', function () { + const menu = menuUtil.getParentMenuDetails(defaultMenu, undefined) + assert.deepEqual(menu, emptyValue) + }) + it('returns an object with null/-1 if label is not found', function () { + const menu = menuUtil.getParentMenuDetails(defaultMenu, 'History') + assert.deepEqual(menu, emptyValue) + }) + }) + + describe('getMenuItem', function () { + it('returns the electron MenuItem based on the label', function () { + const menuItem = menuUtil.getMenuItem(defaultMenu, 'quit') + assert.equal(menuItem.temp, 2) + }) + it('returns null if label is not found', function () { + const menuItem = menuUtil.getMenuItem(defaultMenu, 'not-in-here') + assert.equal(menuItem, null) + }) + }) + + describe('checkForUpdate', function () { + const appStateSettings = Immutable.fromJS({ + settings: { + key: 'value' + } + }) + const appStateSites = Immutable.fromJS({ + sites: { + key: 'value' + } + }) + const windowStateClosedFrames = { + closedFrames: [{ + frameId: 1 + }] + } + it('sets updated.nothing to false if `settings` was updated', function () { + const updated = menuUtil.checkForUpdate(appStateSettings, null) + assert.equal(updated.nothing, false) + }) + it('sets updated.nothing to false if `sites` was updated', function () { + const updated = menuUtil.checkForUpdate(appStateSites, null) + assert.equal(updated.nothing, false) + console.log(menuUtil.lastSites) + }) + it('sets updated.nothing to false if `closedFrames` was updated', function () { + const updated = menuUtil.checkForUpdate(null, windowStateClosedFrames) + assert.equal(updated.nothing, false) + }) + it('leaves updated.nothing as true if nothing was updated', function () { + const updated = menuUtil.checkForUpdate(null, null) + assert.equal(updated.nothing, true) + }) + it('does not save falsey values', function () { + menuUtil.checkForUpdate(appStateSites, null) + menuUtil.checkForUpdate(null, null) + // NOTE: there should be no change, since last calls values weren't recorded + const updated = menuUtil.checkForUpdate(appStateSites, null) + assert.equal(updated.nothing, true) + }) + }) + + describe('createBookmarkMenuItems', function () { + it('returns an array of items w/ the bookmark tag', function () { + const appStateSites = Immutable.fromJS({ + sites: [ + { tags: [siteTags.BOOKMARK], title: 'my website', location: 'https://brave.com' } + ] + }) + + menuUtil.checkForUpdate(appStateSites, null) + const menuItems = menuUtil.createBookmarkMenuItems() + + assert.equal(Array.isArray(menuItems), true) + assert.equal(menuItems.length, 1) + assert.equal(menuItems[0].label, 'my website') + }) + it('prefers the customTitle field for the bookmark title (over the page title)', function () { + const appStateSites = Immutable.fromJS({ + sites: [ + { tags: [siteTags.BOOKMARK], customTitle: 'use this', title: 'not this', location: 'https://brave.com' } + ] + }) + + menuUtil.checkForUpdate(appStateSites, null) + const menuItems = menuUtil.createBookmarkMenuItems() + + assert.equal(menuItems[0].label, 'use this') + }) + it('only returns bookmarks that have a location set', function () { + const appStateSites = Immutable.fromJS({ + sites: [ + { tags: [siteTags.BOOKMARK], title: 'not valid', location: '' } + ] + }) + + menuUtil.checkForUpdate(appStateSites, null) + const menuItems = menuUtil.createBookmarkMenuItems() + + assert.deepEqual(menuItems, []) + }) + it('returns empty array if no bookmarks present', function () { + const appStateSites = Immutable.fromJS({ + sites: [ + { tags: [], title: 'this is a history entry', location: 'https://brave.com' } + ] + }) + + menuUtil.checkForUpdate(appStateSites, null) + const menuItems = menuUtil.createBookmarkMenuItems() + + assert.deepEqual(menuItems, []) + }) + it('does not count pinned tabs as bookmarks', function () { + const appStateSites = Immutable.fromJS({ + sites: [ + { tags: [siteTags.PINNED], title: 'pinned site', location: 'https://pinned-website.com' }, + { tags: [siteTags.BOOKMARK], title: 'my website', location: 'https://brave.com' } + ] + }) + menuUtil.checkForUpdate(appStateSites, null) + const menuItems = menuUtil.createBookmarkMenuItems() + + assert.equal(menuItems.length, 1) + assert.equal(menuItems[0].label, 'my website') + }) + it('processes folders', function () { + const appStateSites = Immutable.fromJS({ + sites: [ + { tags: [siteTags.BOOKMARK_FOLDER], title: 'my folder', folderId: 123 }, + { tags: [siteTags.BOOKMARK], title: 'my website', location: 'https://brave.com', parentFolderId: 123 } + ] + }) + + menuUtil.checkForUpdate(appStateSites, null) + const menuItems = menuUtil.createBookmarkMenuItems() + + assert.equal(menuItems.length, 1) + assert.equal(menuItems[0].label, 'my folder') + assert.equal(menuItems[0].submenu.length, 1) + assert.equal(menuItems[0].submenu[0].label, 'my website') + }) + it('considers customTitle when processing folders', function () { + const appStateSites = Immutable.fromJS({ + sites: [ + { tags: [siteTags.BOOKMARK_FOLDER], customTitle: 'use this', title: 'not this', folderId: 123 }, + { tags: [siteTags.BOOKMARK], title: 'my website', location: 'https://brave.com', parentFolderId: 123 } + ] + }) + + menuUtil.checkForUpdate(appStateSites, null) + const menuItems = menuUtil.createBookmarkMenuItems() + + assert.equal(menuItems.length, 1) + assert.equal(menuItems[0].label, 'use this') + }) + }) + + describe('createRecentlyClosedMenuItems', function () { + it('returns an array of closedFrames preceded by a separator and "Recently Closed" items', function () { + const windowStateClosedFrames = { + closedFrames: [{ + title: 'sample', + location: 'https://brave.com' + }] + } + + menuUtil.checkForUpdate(null, windowStateClosedFrames) + const menuItems = menuUtil.createRecentlyClosedMenuItems() + + assert.equal(Array.isArray(menuItems), true) + assert.equal(menuItems.length, 3) + assert.equal(menuItems[0].type, 'separator') + assert.equal(menuItems[1].label, 'RECENTLYCLOSED') + assert.equal(menuItems[1].enabled, false) + assert.equal(menuItems[2].label, windowStateClosedFrames.closedFrames[0].title) + assert.equal(typeof menuItems[2].click === 'function', true) + }) + }) +}) From c56f3f6082fbdb7d1642bd5e40f52424ee0e92a7 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Tue, 16 Aug 2016 11:28:55 -0700 Subject: [PATCH 2/6] Remove console logs --- app/menu.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/menu.js b/app/menu.js index bb8c7c543cf..b8e6d27b0aa 100644 --- a/app/menu.js +++ b/app/menu.js @@ -584,7 +584,6 @@ const updateMenu = (CommonMenu, appState, windowState) => { // Only update menu when necessary if (updated.settings || updated.closedFrames) { - console.log('rebuilding history menu') let historyMenu = menuUtil.getParentMenuDetails(appMenu, locale.translation('history')) if (historyMenu && historyMenu.menu && historyMenu.menu.submenu && historyMenu.index !== -1) { const menu = historyMenu.menu.submenu @@ -595,7 +594,6 @@ const updateMenu = (CommonMenu, appState, windowState) => { } if (updated.sites) { - console.log('rebuilding bookmarks menu') let bookmarksMenu = menuUtil.getParentMenuDetails(appMenu, locale.translation('bookmarks')) if (bookmarksMenu && bookmarksMenu.menu && bookmarksMenu.menu.submenu && bookmarksMenu.index !== -1) { const menu = bookmarksMenu.menu.submenu From 9774bfb91397a6834c71f33815ddf35fa5a44325 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Tue, 16 Aug 2016 17:37:03 -0700 Subject: [PATCH 3/6] Feedback part 1; note-worthy items: - Renamed IPC message names to be more specific - Only closedFrames is sent to menu update now (and it's immutable, matching the other param) - Added .get('location') to the bookmarks/history items for a fallback title - "Show History" is now always present on back/forward nav button long press menu (thanks for reviewing, @bradleyrichter) - Fixed bug where only 9 items would show on back/forward nav button long press - Noticed something I missed; when closedFrames changes, it needs to trigger a menu update. Put placeholder TODO. --- .../brave/locales/en-US/menu.properties | 2 +- app/index.js | 16 +++--- app/menu.js | 14 +++--- js/components/navigationBar.js | 4 +- js/components/window.js | 2 + js/constants/messages.js | 6 +-- js/contextMenus.js | 50 +++++++++---------- js/entry.js | 8 ++- js/lib/menuUtil.js | 20 ++++---- test/unit/lib/menuUtilTest.js | 34 +++++++++++-- 10 files changed, 92 insertions(+), 64 deletions(-) diff --git a/app/extensions/brave/locales/en-US/menu.properties b/app/extensions/brave/locales/en-US/menu.properties index a29832e1479..e3db72f153f 100644 --- a/app/extensions/brave/locales/en-US/menu.properties +++ b/app/extensions/brave/locales/en-US/menu.properties @@ -43,7 +43,7 @@ home=Home back=Back forward=Forward reopenLastClosedWindow=Reopen Last Closed Window -showAllHistory=Show Full History +showAllHistory=Show History clearHistory=Clear History... clearCache=Clear Cache... clearSiteData=Clear All Cookies and Site Data... diff --git a/app/index.js b/app/index.js index 8ca9268a911..2c7b65e0917 100644 --- a/app/index.js +++ b/app/index.js @@ -295,9 +295,9 @@ app.on('ready', () => { }) // Window state must be fetched from main process; this is fired once it's retrieved - ipcMain.on(messages.RESPONSE_WINDOW_STATE_FOR_MENU, (wnd, data) => { - if (data) { - Menu.init(AppStore.getState(), data) + ipcMain.on(messages.RESPONSE_MENU_DATA_FOR_WINDOW, (wnd, windowData) => { + if (windowData) { + Menu.init(AppStore.getState(), Immutable.fromJS(windowData)) } }) @@ -375,7 +375,7 @@ app.on('ready', () => { // not yet configured. locale.init(initialState.settings[settings.LANGUAGE], (strings) => { if (BrowserWindow.getFocusedWindow()) { - BrowserWindow.getFocusedWindow().webContents.send(messages.REQUEST_WINDOW_STATE_FOR_MENU) + BrowserWindow.getFocusedWindow().webContents.send(messages.REQUEST_MENU_DATA_FOR_WINDOW) } else { Menu.init(AppStore.getState(), null) } @@ -449,9 +449,9 @@ app.on('ready', () => { } }) - ipcMain.on(messages.UPDATE_APP_MENU, (e, args) => { - if (args && typeof args.bookmarked === 'boolean') { - Menu.updateBookmarkedStatus(args.bookmarked) + ipcMain.on(messages.UPDATE_MENU_BOOKMARKED_STATUS, (e, isBookmarked) => { + if (typeof isBookmarked === 'boolean') { + Menu.updateBookmarkedStatus(isBookmarked) } }) @@ -534,7 +534,7 @@ app.on('ready', () => { setInterval(initiateSessionStateSave, 1000 * 60 * 5) AppStore.addChangeListener(() => { if (BrowserWindow.getFocusedWindow()) { - BrowserWindow.getFocusedWindow().webContents.send(messages.REQUEST_WINDOW_STATE_FOR_MENU) + BrowserWindow.getFocusedWindow().webContents.send(messages.REQUEST_MENU_DATA_FOR_WINDOW) } else { Menu.init(AppStore.getState(), null) } diff --git a/app/menu.js b/app/menu.js index b8e6d27b0aa..2686a15f426 100644 --- a/app/menu.js +++ b/app/menu.js @@ -354,7 +354,7 @@ const createHistorySubmenu = (CommonMenu) => { // label: locale.translation('recentlyVisited'), // enabled: false // }, - // CommonMenu.separatorMenuItem, + CommonMenu.separatorMenuItem, CommonMenu.historyMenuItem()) return submenu @@ -575,8 +575,8 @@ const createMenu = (CommonMenu) => { } } -const updateMenu = (CommonMenu, appState, windowState) => { - const updated = menuUtil.checkForUpdate(appState, windowState) +const updateMenu = (CommonMenu, appState, windowData) => { + const updated = menuUtil.checkForUpdate(appState, windowData) if (updated.nothingUpdated) { return } @@ -608,10 +608,10 @@ const updateMenu = (CommonMenu, appState, windowState) => { /** * Sets up the menu. - * @param {Object} appState - Application state - * @param {Object} windowState - Current window state + * @param {Object} appState - Application state. Used to fetch bookmarks and settings (like homepage) + * @param {Object} windowData - Information specific to the current window (recently closed tabs, etc) */ -module.exports.init = (appState, windowState) => { +module.exports.init = (appState, windowData) => { // The menu will always be called once localization is done // so don't bother loading anything until it is done. if (!locale.initialized) { @@ -623,7 +623,7 @@ module.exports.init = (appState, windowState) => { if (appMenu.items.length === 0) { createMenu(CommonMenu) } else { - updateMenu(CommonMenu, appState, windowState) + updateMenu(CommonMenu, appState, windowData) } } diff --git a/js/components/navigationBar.js b/js/components/navigationBar.js index 555f831f165..3c989960960 100644 --- a/js/components/navigationBar.js +++ b/js/components/navigationBar.js @@ -97,7 +97,7 @@ class NavigationBar extends ImmutableComponent { ipc.on(messages.SHORTCUT_ACTIVE_FRAME_BOOKMARK, () => this.onToggleBookmark(false)) ipc.on(messages.SHORTCUT_ACTIVE_FRAME_REMOVE_BOOKMARK, () => this.onToggleBookmark(true)) // Set initial bookmark status in menu - ipc.send(messages.UPDATE_APP_MENU, {bookmarked: this.bookmarked}) + ipc.send(messages.UPDATE_MENU_BOOKMARKED_STATUS, this.bookmarked) } get showNoScriptInfo () { @@ -119,7 +119,7 @@ class NavigationBar extends ImmutableComponent { if (this.bookmarked !== prevBookmarked) { // Used to update the Bookmarks menu (the checked status next to "Bookmark Page") - ipc.send(messages.UPDATE_APP_MENU, {bookmarked: this.bookmarked}) + ipc.send(messages.UPDATE_MENU_BOOKMARKED_STATUS, this.bookmarked) } if (this.props.noScriptIsVisible && !this.showNoScriptInfo) { // There are no blocked scripts, so hide the noscript dialog. diff --git a/js/components/window.js b/js/components/window.js index a8f7f945bbc..3d16976b735 100644 --- a/js/components/window.js +++ b/js/components/window.js @@ -83,6 +83,8 @@ class Window extends React.Component { appState: this.appState } }) + + // TODO: fire off menu refresh (only if closedFrames changed) } onAppStateChange () { diff --git a/js/constants/messages.js b/js/constants/messages.js index f313dc5f9f1..40ddef4acdc 100644 --- a/js/constants/messages.js +++ b/js/constants/messages.js @@ -46,7 +46,6 @@ const messages = { QUIT_APPLICATION: _, OPEN_BRAVERY_PANEL: _, PREFS_RESTART: _, - UPDATE_APP_MENU: _, /** @arg {Object} args menu args to update */ CERT_ERROR: _, /** @arg {Object} details of certificate error */ LOGIN_REQUIRED: _, /** @arg {Object} details of the login required request */ LOGIN_RESPONSE: _, @@ -111,8 +110,9 @@ const messages = { LAST_WINDOW_STATE: _, UNDO_CLOSED_WINDOW: _, // Menu rebuilding - REQUEST_WINDOW_STATE_FOR_MENU: _, - RESPONSE_WINDOW_STATE_FOR_MENU: _, + REQUEST_MENU_DATA_FOR_WINDOW: _, + RESPONSE_MENU_DATA_FOR_WINDOW: _, + UPDATE_MENU_BOOKMARKED_STATUS: _, /** @arg {Object} currently only has a boolean "bookmarked" */ // Ad block, safebrowsing, and tracking protection BLOCKED_RESOURCE: _, BLOCKED_PAGE: _, diff --git a/js/contextMenus.js b/js/contextMenus.js index 3a1d11b7cf0..533aebd70d9 100644 --- a/js/contextMenus.js +++ b/js/contextMenus.js @@ -1055,8 +1055,8 @@ function onMoreBookmarksMenu (activeFrame, allBookmarkItems, overflowItems, e) { function onBackButtonHistoryMenu (activeFrame, history, rect) { const menuTemplate = [] - if (activeFrame && history) { - const stopIndex = Math.max((history.currentIndex - config.navigationBar.maxHistorySites), -1) + if (activeFrame && history && history.entries.length > 0) { + const stopIndex = Math.max(((history.currentIndex - config.navigationBar.maxHistorySites) - 1), -1) for (let index = (history.currentIndex - 1); index > stopIndex; index--) { const url = history.entries[index].url @@ -1076,17 +1076,16 @@ function onBackButtonHistoryMenu (activeFrame, history, rect) { }) } - if (stopIndex !== -1) { - menuTemplate.push( - CommonMenu.separatorMenuItem, - { - label: locale.translation('showAllHistory'), - click: (e, focusedWindow) => { - windowActions.newFrame({ location: 'about:history' }) - windowActions.setContextMenuDetail() - } - }) - } + // Always display "Show History" link + menuTemplate.push( + CommonMenu.separatorMenuItem, + { + label: locale.translation('showAllHistory'), + click: (e, focusedWindow) => { + windowActions.newFrame({ location: 'about:history' }) + windowActions.setContextMenuDetail() + } + }) } windowActions.setContextMenuDetail(Immutable.fromJS({ @@ -1099,8 +1098,8 @@ function onBackButtonHistoryMenu (activeFrame, history, rect) { function onForwardButtonHistoryMenu (activeFrame, history, rect) { const menuTemplate = [] - if (activeFrame && history) { - const stopIndex = Math.min((history.currentIndex + config.navigationBar.maxHistorySites), history.entries.length) + if (activeFrame && history && history.entries.length > 0) { + const stopIndex = Math.min(((history.currentIndex + config.navigationBar.maxHistorySites) + 1), history.entries.length) for (let index = (history.currentIndex + 1); index < stopIndex; index++) { const url = history.entries[index].url @@ -1120,17 +1119,16 @@ function onForwardButtonHistoryMenu (activeFrame, history, rect) { }) } - if (stopIndex !== history.entries.length) { - menuTemplate.push( - CommonMenu.separatorMenuItem, - { - label: locale.translation('showAllHistory'), - click: (e, focusedWindow) => { - windowActions.newFrame({ location: 'about:history' }) - windowActions.setContextMenuDetail() - } - }) - } + // Always display "Show History" link + menuTemplate.push( + CommonMenu.separatorMenuItem, + { + label: locale.translation('showAllHistory'), + click: (e, focusedWindow) => { + windowActions.newFrame({ location: 'about:history' }) + windowActions.setContextMenuDetail() + } + }) } windowActions.setContextMenuDetail(Immutable.fromJS({ diff --git a/js/entry.js b/js/entry.js index 45bed601ab7..1a173fc3beb 100644 --- a/js/entry.js +++ b/js/entry.js @@ -53,8 +53,12 @@ ipc.on(messages.REQUEST_WINDOW_STATE, () => { ipc.send(messages.RESPONSE_WINDOW_STATE, windowStore.getState().toJS()) }) -ipc.on(messages.REQUEST_WINDOW_STATE_FOR_MENU, () => { - ipc.send(messages.RESPONSE_WINDOW_STATE_FOR_MENU, windowStore.getState().toJS()) +ipc.on(messages.REQUEST_MENU_DATA_FOR_WINDOW, () => { + const windowData = { + closedFrames: windowStore.getState().get('closedFrames').toJS() + } + + ipc.send(messages.RESPONSE_MENU_DATA_FOR_WINDOW, windowData) }) if (process.env.NODE_ENV === 'test') { diff --git a/js/lib/menuUtil.js b/js/lib/menuUtil.js index 29a5af22573..1cc8f896971 100644 --- a/js/lib/menuUtil.js +++ b/js/lib/menuUtil.js @@ -61,10 +61,10 @@ module.exports.getMenuItem = (appMenu, label) => { * and also causes menu clicks to not work. So we don't want to update it a lot. * Should only be updated when appState or windowState change (for history or bookmarks) * NOTE: settingsState is not used directly; it gets used indirectly via getSetting() - * @param {Object} appState - Application state - * @param {Object} windowState - Current window state + * @param {Object} appState - Application state. Used to fetch bookmarks and settings (like homepage) + * @param {Object} windowData - Information specific to the current window (recently closed tabs, etc) */ -module.exports.checkForUpdate = (appState, windowState) => { +module.exports.checkForUpdate = (appState, windowData) => { const updated = { nothing: true, settings: false, @@ -85,8 +85,8 @@ module.exports.checkForUpdate = (appState, windowState) => { updated.sites = true } - if (windowState && windowState.closedFrames !== lastClosedFrames) { - lastClosedFrames = windowState.closedFrames + if (windowData && windowData.get('closedFrames') !== lastClosedFrames) { + lastClosedFrames = windowData.get('closedFrames') updated.nothing = false updated.closedFrames = true } @@ -110,7 +110,7 @@ const createBookmarkMenuItems = (bookmarks, parentFolderId) => { // and as such there may need to be another mechanism or cache // // see: https://github.com/brave/browser-laptop/issues/3050 - label: site.get('customTitle') || site.get('title'), + label: site.get('customTitle') || site.get('title') || site.get('location'), click: (item, focusedWindow, e) => { if (eventUtil.isForSecondaryAction(e)) { CommonMenu.sendToFocusedWindow(focusedWindow, [messages.SHORTCUT_NEW_FRAME, site.get('location'), { openInForeground: !!e.shiftKey }]) @@ -140,7 +140,7 @@ module.exports.createBookmarkMenuItems = () => { module.exports.createRecentlyClosedMenuItems = () => { const payload = [] - if (lastClosedFrames && lastClosedFrames.length > 0) { + if (lastClosedFrames && lastClosedFrames.size > 0) { payload.push( CommonMenu.separatorMenuItem, { @@ -151,12 +151,12 @@ module.exports.createRecentlyClosedMenuItems = () => { const lastTen = (lastClosedFrames.size < 10) ? lastClosedFrames : lastClosedFrames.slice(-10) lastTen.forEach((closedFrame) => { payload.push({ - label: closedFrame.title, + label: closedFrame.get('title') || closedFrame.get('location'), click: (item, focusedWindow, e) => { if (eventUtil.isForSecondaryAction(e)) { - CommonMenu.sendToFocusedWindow(focusedWindow, [messages.SHORTCUT_NEW_FRAME, closedFrame.location, { openInForeground: !!e.shiftKey }]) + CommonMenu.sendToFocusedWindow(focusedWindow, [messages.SHORTCUT_NEW_FRAME, closedFrame.get('location'), { openInForeground: !!e.shiftKey }]) } else { - CommonMenu.sendToFocusedWindow(focusedWindow, [messages.SHORTCUT_ACTIVE_FRAME_LOAD_URL, closedFrame.location]) + CommonMenu.sendToFocusedWindow(focusedWindow, [messages.SHORTCUT_ACTIVE_FRAME_LOAD_URL, closedFrame.get('location')]) } } }) diff --git a/test/unit/lib/menuUtilTest.js b/test/unit/lib/menuUtilTest.js index 1044e30d8cb..c956dcdae1b 100644 --- a/test/unit/lib/menuUtilTest.js +++ b/test/unit/lib/menuUtilTest.js @@ -104,11 +104,11 @@ describe('menuUtil', function () { key: 'value' } }) - const windowStateClosedFrames = { + const windowStateClosedFrames = Immutable.fromJS({ closedFrames: [{ frameId: 1 }] - } + }) it('sets updated.nothing to false if `settings` was updated', function () { const updated = menuUtil.checkForUpdate(appStateSettings, null) assert.equal(updated.nothing, false) @@ -233,12 +233,12 @@ describe('menuUtil', function () { describe('createRecentlyClosedMenuItems', function () { it('returns an array of closedFrames preceded by a separator and "Recently Closed" items', function () { - const windowStateClosedFrames = { + const windowStateClosedFrames = Immutable.fromJS({ closedFrames: [{ title: 'sample', location: 'https://brave.com' }] - } + }) menuUtil.checkForUpdate(null, windowStateClosedFrames) const menuItems = menuUtil.createRecentlyClosedMenuItems() @@ -248,8 +248,32 @@ describe('menuUtil', function () { assert.equal(menuItems[0].type, 'separator') assert.equal(menuItems[1].label, 'RECENTLYCLOSED') assert.equal(menuItems[1].enabled, false) - assert.equal(menuItems[2].label, windowStateClosedFrames.closedFrames[0].title) + assert.equal(menuItems[2].label, windowStateClosedFrames.get('closedFrames').first().get('title')) assert.equal(typeof menuItems[2].click === 'function', true) }) + it('only shows the last 10 items', function () { + const windowStateClosedFrames = Immutable.fromJS({ + closedFrames: [ + { title: 'site01', location: 'https://brave01.com' }, + { title: 'site02', location: 'https://brave02.com' }, + { title: 'site03', location: 'https://brave03.com' }, + { title: 'site04', location: 'https://brave04.com' }, + { title: 'site05', location: 'https://brave05.com' }, + { title: 'site06', location: 'https://brave06.com' }, + { title: 'site07', location: 'https://brave07.com' }, + { title: 'site08', location: 'https://brave08.com' }, + { title: 'site09', location: 'https://brave09.com' }, + { title: 'site10', location: 'https://brave10.com' }, + { title: 'site11', location: 'https://brave11.com' } + ] + }) + + menuUtil.checkForUpdate(null, windowStateClosedFrames) + const menuItems = menuUtil.createRecentlyClosedMenuItems() + + assert.equal(menuItems.length, 12) + assert.equal(menuItems[2].label, windowStateClosedFrames.get('closedFrames').get(1).get('title')) + assert.equal(menuItems[11].label, windowStateClosedFrames.get('closedFrames').get(10).get('title')) + }) }) }) From 36081c0f164e26644c2044b8daf30db7734b19de Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Tue, 16 Aug 2016 23:57:57 -0700 Subject: [PATCH 4/6] Feedback part 2; note-worthy items: - Menu now rebuilds on BrowserWindow focus - Fixed bug where "Bookmark Page" menu item's check status could be out of sync (got updated via AppStore) --- app/index.js | 21 +++++++++++---------- app/menu.js | 11 +++++++++-- js/components/navigationBar.js | 7 +++++-- js/components/window.js | 2 +- js/entry.js | 6 +----- js/stores/appStore.js | 4 ++++ 6 files changed, 31 insertions(+), 20 deletions(-) diff --git a/app/index.js b/app/index.js index 2c7b65e0917..f6394c4479a 100644 --- a/app/index.js +++ b/app/index.js @@ -59,6 +59,7 @@ const siteSettings = require('../js/state/siteSettings') const spellCheck = require('./spellCheck') const flash = require('../js/flash') const contentSettings = require('../js/state/contentSettings') +const FrameStateUtil = require('../js/state/frameStateUtil') // Used to collect the per window state when shutting down the application let perWindowState = [] @@ -295,9 +296,15 @@ app.on('ready', () => { }) // Window state must be fetched from main process; this is fired once it's retrieved - ipcMain.on(messages.RESPONSE_MENU_DATA_FOR_WINDOW, (wnd, windowData) => { - if (windowData) { - Menu.init(AppStore.getState(), Immutable.fromJS(windowData)) + ipcMain.on(messages.RESPONSE_MENU_DATA_FOR_WINDOW, (wnd, windowState) => { + if (windowState) { + const activeFrame = FrameStateUtil.getActiveFrame(Immutable.fromJS(windowState)) + const windowData = Immutable.fromJS({ + location: activeFrame.get('location'), + closedFrames: windowState.closedFrames + }) + + Menu.init(AppStore.getState(), windowData) } }) @@ -373,13 +380,7 @@ app.on('ready', () => { // Initiate the translation for a configured language and // reset the browser window. This will default to en-US if // not yet configured. - locale.init(initialState.settings[settings.LANGUAGE], (strings) => { - if (BrowserWindow.getFocusedWindow()) { - BrowserWindow.getFocusedWindow().webContents.send(messages.REQUEST_MENU_DATA_FOR_WINDOW) - } else { - Menu.init(AppStore.getState(), null) - } - }) + locale.init(initialState.settings[settings.LANGUAGE]) // Do this after loading the state // For tests we always want to load default app state diff --git a/app/menu.js b/app/menu.js index 2686a15f426..b12ae2ccd8c 100644 --- a/app/menu.js +++ b/app/menu.js @@ -4,6 +4,7 @@ 'use strict' +const Immutable = require('immutable') const electron = require('electron') const appConfig = require('../js/constants/appConfig') const Menu = electron.Menu @@ -15,6 +16,7 @@ const appActions = require('../js/actions/appActions') const menuUtil = require('../js/lib/menuUtil') const getSetting = require('../js/settings').getSetting const locale = require('./locale') +const {isSiteBookmarked} = require('../js/state/siteUtil') const isDarwin = process.platform === 'darwin' const aboutUrl = 'https://brave.com/' @@ -22,7 +24,7 @@ const aboutUrl = 'https://brave.com/' let appMenu = Menu.buildFromTemplate([]) Menu.setApplicationMenu(appMenu) -// Used to hold the default value for "isBookmarked" (see createBookmarksSubmenu) +// Value for history menu's "Bookmark Page" menu item; see createBookmarksSubmenu() let isBookmarkChecked = false // Submenu initialization @@ -581,7 +583,12 @@ const updateMenu = (CommonMenu, appState, windowData) => { return } - // Only update menu when necessary + // When bookmarks are removed via AppStore (context menu, etc), `isBookmarkChecked` needs to be recalculated + if (windowData.get('location')) { + isBookmarkChecked = isSiteBookmarked(appState.get('sites'), Immutable.fromJS({location: windowData.get('location')})) + } + + // Only rebuild menus when necessary if (updated.settings || updated.closedFrames) { let historyMenu = menuUtil.getParentMenuDetails(appMenu, locale.translation('history')) diff --git a/js/components/navigationBar.js b/js/components/navigationBar.js index 3c989960960..630c4595a1d 100644 --- a/js/components/navigationBar.js +++ b/js/components/navigationBar.js @@ -51,8 +51,12 @@ class NavigationBar extends ImmutableComponent { if (!isBookmarked) { appActions.addSite(siteDetail, siteTags.BOOKMARK) } + // Show bookmarks toolbar after first bookmark is saved appActions.changeSetting(settings.SHOW_BOOKMARKS_TOOLBAR, !hasBookmark || showBookmarksToolbar) + // trigger the AddEditBookmark modal windowActions.setBookmarkDetail(siteDetail, siteDetail) + // Update checked/unchecked status in the Bookmarks menu + ipc.send(messages.UPDATE_MENU_BOOKMARKED_STATUS, this.bookmarked) } onReload (e) { @@ -96,7 +100,7 @@ class NavigationBar extends ImmutableComponent { componentDidMount () { ipc.on(messages.SHORTCUT_ACTIVE_FRAME_BOOKMARK, () => this.onToggleBookmark(false)) ipc.on(messages.SHORTCUT_ACTIVE_FRAME_REMOVE_BOOKMARK, () => this.onToggleBookmark(true)) - // Set initial bookmark status in menu + // Set initial checked/unchecked status in Bookmarks menu ipc.send(messages.UPDATE_MENU_BOOKMARKED_STATUS, this.bookmarked) } @@ -118,7 +122,6 @@ class NavigationBar extends ImmutableComponent { })) if (this.bookmarked !== prevBookmarked) { - // Used to update the Bookmarks menu (the checked status next to "Bookmark Page") ipc.send(messages.UPDATE_MENU_BOOKMARKED_STATUS, this.bookmarked) } if (this.props.noScriptIsVisible && !this.showNoScriptInfo) { diff --git a/js/components/window.js b/js/components/window.js index 3d16976b735..d0e9f04c15f 100644 --- a/js/components/window.js +++ b/js/components/window.js @@ -84,7 +84,7 @@ class Window extends React.Component { } }) - // TODO: fire off menu refresh (only if closedFrames changed) + // TODO: fire off menu refresh; or find a frame closed event } onAppStateChange () { diff --git a/js/entry.js b/js/entry.js index 1a173fc3beb..753706086b5 100644 --- a/js/entry.js +++ b/js/entry.js @@ -54,11 +54,7 @@ ipc.on(messages.REQUEST_WINDOW_STATE, () => { }) ipc.on(messages.REQUEST_MENU_DATA_FOR_WINDOW, () => { - const windowData = { - closedFrames: windowStore.getState().get('closedFrames').toJS() - } - - ipc.send(messages.RESPONSE_MENU_DATA_FOR_WINDOW, windowData) + ipc.send(messages.RESPONSE_MENU_DATA_FOR_WINDOW, windowStore.getState().toJS()) }) if (process.env.NODE_ENV === 'test') { diff --git a/js/stores/appStore.js b/js/stores/appStore.js index b54b60c9fab..064e1d6ed3d 100644 --- a/js/stores/appStore.js +++ b/js/stores/appStore.js @@ -146,6 +146,10 @@ const createWindow = (browserOpts, defaults, frameOpts, windowState) => { mainWindow.setFullScreen(true) } + mainWindow.on('focus', function () { + mainWindow.webContents.send(messages.REQUEST_MENU_DATA_FOR_WINDOW) + }) + mainWindow.on('resize', function (evt) { // the default window size is whatever the last window resize was appActions.setDefaultWindowSize(evt.sender.getSize()) From cf083e7f9953fb6df4140c6a9f6fbd03d6ff3e32 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Wed, 17 Aug 2016 00:46:18 -0700 Subject: [PATCH 5/6] Feedback part 3; note-worthy items: - Fixed issue where empty session would not initialize menu - Closing frames will now fire Menu update (for the History > Recently Closed) --- app/index.js | 5 ++++- js/components/window.js | 2 -- js/stores/windowStore.js | 3 +++ 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/index.js b/app/index.js index f6394c4479a..e7ee7aef3a7 100644 --- a/app/index.js +++ b/app/index.js @@ -380,7 +380,9 @@ app.on('ready', () => { // Initiate the translation for a configured language and // reset the browser window. This will default to en-US if // not yet configured. - locale.init(initialState.settings[settings.LANGUAGE]) + locale.init(initialState.settings[settings.LANGUAGE], (strings) => { + Menu.init(AppStore.getState(), null) + }) // Do this after loading the state // For tests we always want to load default app state @@ -533,6 +535,7 @@ app.on('ready', () => { // save app state every 5 minutes regardless of update frequency setInterval(initiateSessionStateSave, 1000 * 60 * 5) + AppStore.addChangeListener(() => { if (BrowserWindow.getFocusedWindow()) { BrowserWindow.getFocusedWindow().webContents.send(messages.REQUEST_MENU_DATA_FOR_WINDOW) diff --git a/js/components/window.js b/js/components/window.js index d0e9f04c15f..a8f7f945bbc 100644 --- a/js/components/window.js +++ b/js/components/window.js @@ -83,8 +83,6 @@ class Window extends React.Component { appState: this.appState } }) - - // TODO: fire off menu refresh; or find a frame closed event } onAppStateChange () { diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js index 75abc2f675e..68e9105e842 100644 --- a/js/stores/windowStore.js +++ b/js/stores/windowStore.js @@ -434,6 +434,9 @@ const doAction = (action) => { activeFrameKey)) let totalOpenTabs = windowState.get('frames').filter((frame) => !frame.get('pinnedLocation')).size + // History menu needs update (since it shows "Recently Closed" items) + ipc.send(messages.RESPONSE_MENU_DATA_FOR_WINDOW, windowState.toJS()) + // If we reach the limit of opened tabs per page while closing tabs, switch to // the active tab's page otherwise the user will hang on empty page if ((totalOpenTabs % getSetting(settings.TABS_PER_PAGE)) === 0) { From 8a044fc91707bbb5dd3835ce31a7c37b2d709314 Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Wed, 17 Aug 2016 08:07:40 -0700 Subject: [PATCH 6/6] Small fix- forgot to check if value is initialized before using --- app/menu.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/menu.js b/app/menu.js index b12ae2ccd8c..4226ae49520 100644 --- a/app/menu.js +++ b/app/menu.js @@ -584,7 +584,7 @@ const updateMenu = (CommonMenu, appState, windowData) => { } // When bookmarks are removed via AppStore (context menu, etc), `isBookmarkChecked` needs to be recalculated - if (windowData.get('location')) { + if (windowData && windowData.get('location')) { isBookmarkChecked = isSiteBookmarked(appState.get('sites'), Immutable.fromJS({location: windowData.get('location')})) }