diff --git a/app/browser/menu.js b/app/browser/menu.js index eb38bb04735..3bd8014dcfd 100644 --- a/app/browser/menu.js +++ b/app/browser/menu.js @@ -22,6 +22,7 @@ const dialog = electron.dialog const app = electron.app const BrowserWindow = electron.BrowserWindow const {fileUrl} = require('../../js/lib/appUrlUtil') +const {isValidClosedFrame} = require('../../js/state/frameStateUtil') const menuUtil = require('../common/lib/menuUtil') const {getByTabId} = require('../common/state/tabState') const getSetting = require('../../js/settings').getSetting @@ -33,7 +34,7 @@ const isLinux = process.platform === 'linux' let appMenu = null // TODO(bridiver) - these should be handled in the appStore -let closedFrames = {} +let closedFrames = new Immutable.OrderedMap() let lastClosedUrl = null let currentLocation = null @@ -335,21 +336,32 @@ const createHistorySubmenu = () => { } } ] - submenu = submenu.concat(menuUtil.createRecentlyClosedTemplateItems(Immutable.fromJS(Object.keys(closedFrames).map(key => closedFrames[key])))) + const recentlyClosedItems = menuUtil.createRecentlyClosedTemplateItems(closedFrames) + submenu = submenu.concat(recentlyClosedItems) submenu.push( - // TODO: recently visited - // CommonMenu.separatorMenuItem, - // { - // label: locale.translation('recentlyVisited'), - // enabled: false - // }, CommonMenu.separatorMenuItem, - CommonMenu.historyMenuItem()) + CommonMenu.historyMenuItem() + ) return submenu } +const updateRecentlyClosedMenuItems = () => { + // Update electron menu (Mac / Linux) + menuUtil.updateRecentlyClosedMenuItems(appMenu, closedFrames) + Menu.setApplicationMenu(appMenu) + + // Update in-memory menu template (Windows) + const oldTemplate = appStore.getState().getIn(['menu', 'template']) + const historySubmenuKey = oldTemplate.findKey(value => + value.get('label') === locale.translation('history') + ) + const newSubmenu = Immutable.fromJS(createHistorySubmenu()) + const newTemplate = oldTemplate.set(historySubmenuKey, newSubmenu) + appActions.setMenubarTemplate(newTemplate) +} + const isCurrentLocationBookmarked = () => { return isLocationBookmarked(appStore.getState(), currentLocation) } @@ -618,27 +630,31 @@ const doAction = (action) => { break case windowConstants.WINDOW_UNDO_CLOSED_FRAME: appDispatcher.waitFor([appStore.dispatchToken], () => { - delete closedFrames[lastClosedUrl] - createMenu() + if (!lastClosedUrl) { + return + } + closedFrames = closedFrames.delete(lastClosedUrl) + const nextLastFrame = closedFrames.last() + lastClosedUrl = nextLastFrame ? nextLastFrame.get('location') : null + updateRecentlyClosedMenuItems() }) break case windowConstants.WINDOW_CLEAR_CLOSED_FRAMES: appDispatcher.waitFor([appStore.dispatchToken], () => { - closedFrames = {} + closedFrames = new Immutable.OrderedMap() lastClosedUrl = null - createMenu() + updateRecentlyClosedMenuItems() }) break case appConstants.APP_TAB_CLOSE_REQUESTED: appDispatcher.waitFor([appStore.dispatchToken], () => { action = makeImmutable(action) const tab = getByTabId(appStore.getState(), action.get('tabId')) - if (tab && !tab.get('incognito') && tab.get('url') !== 'about:newtab') { - if (tab.get('frame')) { - lastClosedUrl = tab.get('url') - closedFrames[tab.get('url')] = tab.get('frame') - createMenu() - } + const frame = tab && tab.get('frame') + if (tab && !tab.get('incognito') && frame && isValidClosedFrame(frame)) { + lastClosedUrl = tab.get('url') + closedFrames = closedFrames.set(tab.get('url'), tab.get('frame')) + updateRecentlyClosedMenuItems() } }) break diff --git a/app/common/lib/menuUtil.js b/app/common/lib/menuUtil.js index 3ed14bc5b28..65f24d89987 100644 --- a/app/common/lib/menuUtil.js +++ b/app/common/lib/menuUtil.js @@ -3,6 +3,7 @@ 'use strict' +const MenuItem = require('electron').MenuItem const {makeImmutable} = require('../../common/state/immutableUtil') const CommonMenu = require('../../common/commonMenu') const siteTags = require('../../../js/constants/siteTags') @@ -10,6 +11,7 @@ const eventUtil = require('../../../js/lib/eventUtil') const siteUtil = require('../../../js/state/siteUtil') const locale = require('../../locale') const appActions = require('../../../js/actions/appActions') +const config = require('../../../js/constants/config') const {separatorMenuItem} = require('../../common/commonMenu') const windowActions = require('../../../js/actions/windowActions') @@ -122,42 +124,154 @@ module.exports.createBookmarkTemplateItems = (sites) => { } /** - * Create "recently closed" history entries for the "History" menu + * @param {string} key within closedFrames, i.e. a URL + * @return {string} */ -module.exports.createRecentlyClosedTemplateItems = (lastClosedFrames) => { - const payload = [] +const getRecentlyClosedMenuId = function (key) { + return `recentlyClosedFrame|${key}` +} +module.exports.getRecentlyClosedMenuId = getRecentlyClosedMenuId - lastClosedFrames = makeImmutable(lastClosedFrames) +/** + * @param {string} menuId + * @return {string} key within closedFrames, i.e. a URL + */ +const getRecentlyClosedMenuKey = function (menuId) { + if (typeof menuId !== 'string' || menuId.indexOf('recentlyClosedFrame|') === -1) { + return undefined + } + return menuId.split('|')[1] +} - if (lastClosedFrames && lastClosedFrames.size > 0) { - payload.push( - CommonMenu.separatorMenuItem, - { - label: locale.translation('recentlyClosed'), - enabled: false +const recentlyClosedClickHandler = (frame) => { + return (item, focusedWindow, e) => { + const location = frame.get('location') + if (eventUtil.isForSecondaryAction(e)) { + appActions.createTabRequested({ + url: location, + windowId: focusedWindow.id, + active: !!e.shiftKey }) + } else { + appActions.loadURLInActiveTabRequested(focusedWindow.id, location) + } + } +} - const lastTen = ((lastClosedFrames.size < 10) ? lastClosedFrames : lastClosedFrames.slice(-10)).reverse() - lastTen.forEach((closedFrame) => { - payload.push({ - label: closedFrame.get('title') || closedFrame.get('location'), - click: (item, focusedWindow, e) => { - if (eventUtil.isForSecondaryAction(e)) { - appActions.createTabRequested({ - url: closedFrame.get('location'), - windowId: focusedWindow.id, - active: !!e.shiftKey - }) - } else { - appActions.loadURLInActiveTabRequested(focusedWindow.id, closedFrame.get('location')) - } - } - }) +const getFrameMenuLabel = (frame) => { + return frame.get('title') || frame.get('location') +} + +const recentlyClosedTemplate = (key, frame) => { + return { + id: getRecentlyClosedMenuId(key), + click: recentlyClosedClickHandler(frame), + label: getFrameMenuLabel(frame) + } +} + +module.exports.recentlyClosedHeadingTemplates = () => { + return [ + { + id: 'recentlyClosedSeparator', + type: 'separator' + }, + { + id: 'recentlyClosedHeading', + label: locale.translation('recentlyClosed'), + enabled: false + } + ] +} + +/** + * Create "recently closed" history entries for the "History" menu. + * Labels and visibility change dynamically in updateRecentlyClosedMenuItems. + * @param {Immutable.OrderedMap} closedFrames + */ +module.exports.createRecentlyClosedTemplateItems = (closedFrames) => { + let payload = module.exports.recentlyClosedHeadingTemplates() + if (!closedFrames || !closedFrames.size) { + return payload.map((item) => { + item.visible = false + return item }) } + let n = 0 + closedFrames.reverse().forEach((frame) => { + payload.push(recentlyClosedTemplate(n, frame)) + n = n + 1 + if (n >= config.menu.maxClosedFrames) { + return false + } + }) return payload } +/** + * Update display of History menu "Recently closed" menu items by + * inserting MenuItems or hiding existing MenuItems. + * @param {electron.Menu} appMenu + * @param {Immutable.OrderedMap} closedFrames + */ +module.exports.updateRecentlyClosedMenuItems = (appMenu, closedFrames) => { + const headingVisible = closedFrames.size > 0 + const maxMenuItems = config.menu.maxClosedFrames + const historyLabel = locale.translation('history') + const historyMenu = module.exports.getMenuItem(appMenu, historyLabel).submenu + let insertPosition = 0 + + const historyMenuIndicesByOrder = {} + for (let i = 0; i < historyMenu.items.length; i++) { + const item = historyMenu.items[i] + // New items go after "Recently closed" + if (!insertPosition && item.id === 'recentlyClosedHeading') { + insertPosition = i + 1 + item.visible = headingVisible + continue + } else if (item.id === 'recentlyClosedSeparator') { + item.visible = headingVisible + continue + } + + // Find existing items + const key = getRecentlyClosedMenuKey(item.id) + if (typeof key !== 'string') { + continue + } + // Undo close tab removes closed frames. + if (!closedFrames.get(key)) { + item.visible = false + continue + } + historyMenuIndicesByOrder[key] = i + } + + let visibleItems = 0 + closedFrames.reverse().forEach((frame, url) => { + const menuIndex = historyMenuIndicesByOrder[url] + if (visibleItems >= maxMenuItems) { + if (menuIndex) { + historyMenu.items[menuIndex].visible = false + } + return + } + if (menuIndex) { + historyMenu.items[menuIndex].visible = true + visibleItems += 1 + } else { + const template = recentlyClosedTemplate(url, frame) + const item = new MenuItem(template) + // XXX: Can't set this with MenuItem constructor + item.id = template.id + historyMenu.insert(insertPosition, item) + visibleItems += 1 + insertPosition = insertPosition + 1 + } + }) + return appMenu +} + const isItemValid = (currentItem, previousItem) => { if (previousItem && previousItem === CommonMenu.separatorMenuItem) { if (currentItem === CommonMenu.separatorMenuItem) { diff --git a/app/renderer/reducers/frameReducer.js b/app/renderer/reducers/frameReducer.js index 247b0a9ccfa..ae58bee5f44 100644 --- a/app/renderer/reducers/frameReducer.js +++ b/app/renderer/reducers/frameReducer.js @@ -106,7 +106,7 @@ const frameReducer = (state, action, immutableAction) => { case windowConstants.WINDOW_CLOSE_FRAMES: let closedFrames = new Immutable.List() action.framePropsList.forEach((frameProps) => { - if (!frameProps.get('isPrivate') && frameProps.get('location') !== 'about:newtab') { + if (frameStateUtil.isValidClosedFrame(frameProps)) { closedFrames = closedFrames.push(frameProps) if (closedFrames.size > config.maxClosedFrames) { closedFrames = closedFrames.shift() diff --git a/js/constants/config.js b/js/constants/config.js index 78819da43c5..2829e225f25 100644 --- a/js/constants/config.js +++ b/js/constants/config.js @@ -20,6 +20,10 @@ module.exports = { }, fingerprintingInfoUrl: 'https://github.com/brave/browser-laptop/wiki/Fingerprinting-Protection-Mode', maxClosedFrames: 100, + menu: { + // History -> Recently closed frame list + maxClosedFrames: 10 + }, thumbnail: { width: 160, height: 100 diff --git a/js/state/frameStateUtil.js b/js/state/frameStateUtil.js index 14d7d071184..1670211b8f7 100644 --- a/js/state/frameStateUtil.js +++ b/js/state/frameStateUtil.js @@ -377,7 +377,7 @@ function removeFrame (state, frameProps, framePropsIndex) { let closedFrames = state.get('closedFrames') const newFrames = frames.splice(framePropsIndex, 1) - if (!frameProps.get('isPrivate') && frameProps.get('location') !== 'about:newtab') { + if (isValidClosedFrame(frameProps)) { frameProps = frameProps.set('isFullScreen', false) closedFrames = closedFrames.push(frameProps) if (frameProps.get('thumbnailBlob')) { @@ -484,6 +484,11 @@ const updateFramesInternalIndex = (state, fromIndex) => { return state.set('framesInternal', framesInternal) } +const isValidClosedFrame = (frame) => { + const location = frame.get('location') + return !frame.get('isPrivate') && location.indexOf('about:newtab') !== 0 && location.indexOf('about:blank') !== 0 +} + module.exports = { deleteTabInternalIndex, deleteFrameInternalIndex, @@ -530,5 +535,6 @@ module.exports = { activeFrameStatePath, getLastCommittedURL, onFindBarHide, - getTotalBlocks + getTotalBlocks, + isValidClosedFrame } diff --git a/test/unit/app/common/lib/menuUtilTest.js b/test/unit/app/common/lib/menuUtilTest.js index 711b97b5bac..d58a0fd6478 100644 --- a/test/unit/app/common/lib/menuUtilTest.js +++ b/test/unit/app/common/lib/menuUtilTest.js @@ -1,8 +1,11 @@ /* global describe, before, after, it */ const siteTags = require('../../../../../js/constants/siteTags') const mockery = require('mockery') +const sinon = require('sinon') const assert = require('assert') const Immutable = require('immutable') +// This is required; commonMenu is included by menuUtil (and references electron) +const fakeElectron = require('../../../lib/fakeElectron') require('../../../braveUnit') @@ -16,8 +19,7 @@ describe('menuUtil tests', function () { warnOnUnregistered: false, useCleanCache: true }) - // This is required; commonMenu is included by menuUtil (and references electron) - mockery.registerMock('electron', require('../../../lib/fakeElectron')) + mockery.registerMock('electron', fakeElectron) menuUtil = require('../../../../../app/common/lib/menuUtil') separator = require('../../../../../app/common/commonMenu').separatorMenuItem }) @@ -236,20 +238,110 @@ describe('menuUtil tests', function () { assert.equal(menuItems[2].label, windowStateClosedFrames.get(10).get('title')) }) - it('returns an empty array if lastClosedFrames is null, empty, or undefined', function () { - assert.deepEqual(menuUtil.createRecentlyClosedTemplateItems(), []) - assert.deepEqual(menuUtil.createRecentlyClosedTemplateItems(null), []) - assert.deepEqual(menuUtil.createRecentlyClosedTemplateItems(Immutable.fromJS([])), []) + it('returns hidden heading menu items if lastClosedFrames is null, empty, or undefined', function () { + const hiddenItems = menuUtil.recentlyClosedHeadingTemplates().map((item) => { + item.visible = false + return item + }) + assert.deepEqual(menuUtil.createRecentlyClosedTemplateItems(), hiddenItems) + assert.deepEqual(menuUtil.createRecentlyClosedTemplateItems(null), hiddenItems) + assert.deepEqual(menuUtil.createRecentlyClosedTemplateItems(Immutable.fromJS({})), hiddenItems) }) + }) - it('makes the input immutable if passed as mutable', function () { - const windowStateClosedFrames = [{ - title: 'sample', - location: 'https://brave.com' - }] - const menuItems = menuUtil.createRecentlyClosedTemplateItems(windowStateClosedFrames) - assert.equal(Array.isArray(menuItems), true) - assert.equal(menuItems.length, 3) + describe('updateRecentlyClosedMenuItems', function () { + const url1 = 'https://brave01.com' + const url2 = 'https://brave02.com' + const url3 = 'https://brave03.com' + const frame1 = new Immutable.Map({title: 'site1', location: url1}) + const frame2 = new Immutable.Map({title: 'site2', location: url2}) + const frame3 = new Immutable.Map({title: 'site3', location: url3}) + const frameMatcher = (frame) => { + return (menuItem) => { + return menuItem.id === menuUtil.getRecentlyClosedMenuId(frame.get('location')) + } + } + + before(function () { + this.historyMenu = { + submenu: { + insert: sinon.spy(), + items: [] + } + } + sinon.stub(menuUtil, 'getMenuItem').returns(this.historyMenu) + }) + + after(function () { + menuUtil.getMenuItem.restore() + }) + + it('inserts new closed frames, with more recent frames appearing first', function () { + let closedFrames = new Immutable.OrderedMap() + closedFrames = closedFrames.set(frame1.get('location'), frame1) + closedFrames = closedFrames.set(frame2.get('location'), frame2) + closedFrames = closedFrames.set(frame3.get('location'), frame3) + menuUtil.updateRecentlyClosedMenuItems({}, closedFrames) + sinon.assert.calledWith( + this.historyMenu.submenu.insert.getCall(0), + 0, sinon.match(frameMatcher(frame3)) + ) + sinon.assert.calledWith( + this.historyMenu.submenu.insert.getCall(1), + 1, sinon.match(frameMatcher(frame2)) + ) + sinon.assert.calledWith( + this.historyMenu.submenu.insert.getCall(2), + 2, sinon.match(frameMatcher(frame1)) + ) + }) + + it('does not insert duplicate frames', function () { + let closedFrames = new Immutable.OrderedMap() + closedFrames = closedFrames.set(frame1.get('location'), frame1) + this.historyMenu = { + submenu: { + insert: sinon.spy(), + items: [ + { + id: menuUtil.getRecentlyClosedMenuId(frame1.get('location')), + label: 'site1', + visible: true + } + ] + } + } + menuUtil.updateRecentlyClosedMenuItems({}, closedFrames) + assert(this.historyMenu.submenu.insert.notCalled) + }) + + it('hides closed frames which have been reopened', function () { + let closedFrames = new Immutable.OrderedMap() + closedFrames = closedFrames.set(frame1.get('location'), frame1) + closedFrames = closedFrames.set(frame2.get('location'), frame2) + this.historyMenu = { + submenu: { + insert: sinon.spy(), + items: [ + { + id: menuUtil.getRecentlyClosedMenuId(frame2.get('location')), + label: 'site2', + visible: true + }, + { + id: menuUtil.getRecentlyClosedMenuId(frame1.get('location')), + label: 'site1', + visible: true + } + ] + } + } + menuUtil.updateRecentlyClosedMenuItems({}, closedFrames) + sinon.assert.notCalled(this.historyMenu.submenu.insert) + closedFrames.delete(frame2.get('location')) + menuUtil.updateRecentlyClosedMenuItems({}, closedFrames) + assert(this.historyMenu.submenu.items[0].visible, false) + assert(this.historyMenu.submenu.items[1].visible, true) }) }) diff --git a/test/unit/lib/fakeElectron.js b/test/unit/lib/fakeElectron.js index a0df159f1d1..703a415b2e0 100644 --- a/test/unit/lib/fakeElectron.js +++ b/test/unit/lib/fakeElectron.js @@ -11,6 +11,11 @@ const fakeElectron = { } } }, + MenuItem: class { + constructor (template) { + this.template = template + } + }, ipcMain: { on: function () { }, send: function () { }