This repository has been archived by the owner on Dec 11, 2019. It is now read-only.
only update the menu when bookmarks or closed tabs change #3813
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,12 +7,17 @@ | |
| const Immutable = require('immutable') | ||
| const electron = require('electron') | ||
| const appConfig = require('../../js/constants/appConfig') | ||
| const appActions = require('../../js/actions/appActions') | ||
| const appConstants = require('../../js/constants/appConstants') | ||
| const appDispatcher = require('../../js/dispatcher/appDispatcher') | ||
| const appStore = require('../../js/stores/appStore') | ||
| const windowConstants = require('../../js/constants/windowConstants') | ||
| const Menu = electron.Menu | ||
| const CommonMenu = require('../common/commonMenu') | ||
| const messages = require('../../js/constants/messages') | ||
| const settings = require('../../js/constants/settings') | ||
| const siteTags = require('../../js/constants/siteTags') | ||
| const dialog = electron.dialog | ||
| const { fileUrl } = require('../../js/lib/appUrlUtil') | ||
| const menuUtil = require('./lib/menuUtil') | ||
| const getSetting = require('../../js/settings').getSetting | ||
|
|
@@ -21,15 +26,13 @@ const {isSiteBookmarked} = require('../../js/state/siteUtil') | |
| const isDarwin = process.platform === 'darwin' | ||
| const aboutUrl = 'https://brave.com/' | ||
|
|
||
| let appMenu = null | ||
| // TODO(bridiver) - these should be handled in the appStore | ||
| let closedFrames = {} | ||
| let currentLocation = null | ||
|
|
||
| // Submenu initialization | ||
| const createFileSubmenu = () => { | ||
| const submenu = [ | ||
| CommonMenu.newTabMenuItem(), | ||
| CommonMenu.newPrivateTabMenuItem(), | ||
|
|
@@ -121,7 +124,7 @@ const createFileSubmenu = (CommonMenu) => { | |
| return submenu | ||
| } | ||
|
|
||
| const createEditSubmenu = () => { | ||
| const submenu = [ | ||
| { | ||
| label: locale.translation('undo'), | ||
|
|
@@ -186,7 +189,7 @@ const createEditSubmenu = (CommonMenu) => { | |
| return submenu | ||
| } | ||
|
|
||
| const createViewSubmenu = () => { | ||
| return [ | ||
| { | ||
| label: locale.translation('actualSize'), | ||
|
|
@@ -287,7 +290,7 @@ const createViewSubmenu = (CommonMenu) => { | |
| ] | ||
| } | ||
|
|
||
| const createHistorySubmenu = () => { | ||
| let submenu = [ | ||
| { | ||
| label: locale.translation('home'), | ||
|
|
@@ -347,8 +350,7 @@ const createHistorySubmenu = (CommonMenu) => { | |
| } | ||
| } | ||
| ] | ||
| submenu = submenu.concat(menuUtil.createRecentlyClosedMenuItems(Immutable.fromJS(Object.keys(closedFrames).map(key => closedFrames[key])))) | ||
|
|
||
| submenu.push( | ||
| // TODO: recently visited | ||
|
|
@@ -363,13 +365,17 @@ const createHistorySubmenu = (CommonMenu) => { | |
| return submenu | ||
| } | ||
|
|
||
| const isCurrentLocationBookmarked = () => { | ||
| return isSiteBookmarked(appStore.getState().get('sites'), Immutable.fromJS({location: currentLocation})) | ||
| } | ||
|
|
||
| const createBookmarksSubmenu = () => { | ||
| let submenu = [ | ||
| { | ||
| label: locale.translation('bookmarkPage'), | ||
| type: 'checkbox', | ||
| accelerator: 'CmdOrCtrl+D', | ||
| checked: isCurrentLocationBookmarked(), | ||
| click: function (item, focusedWindow) { | ||
| var msg = item.checked | ||
| ? messages.SHORTCUT_ACTIVE_FRAME_REMOVE_BOOKMARK | ||
|
|
@@ -389,7 +395,7 @@ const createBookmarksSubmenu = (CommonMenu) => { | |
| CommonMenu.importBookmarksMenuItem() | ||
| ] | ||
|
|
||
| const bookmarks = menuUtil.createBookmarkMenuItems(appStore.getState().get('sites')) | ||
| if (bookmarks.length > 0) { | ||
| submenu.push(CommonMenu.separatorMenuItem) | ||
| submenu = submenu.concat(bookmarks) | ||
|
|
@@ -398,7 +404,7 @@ const createBookmarksSubmenu = (CommonMenu) => { | |
| return submenu | ||
| } | ||
|
|
||
| const createWindowSubmenu = () => { | ||
| return [ | ||
| { | ||
| label: locale.translation('minimize'), | ||
|
|
@@ -441,7 +447,7 @@ const createWindowSubmenu = (CommonMenu) => { | |
| ] | ||
| } | ||
|
|
||
| const createHelpSubmenu = () => { | ||
| const submenu = [ | ||
| CommonMenu.reportAnIssueMenuItem(), | ||
| CommonMenu.separatorMenuItem, | ||
|
|
@@ -465,7 +471,7 @@ const createHelpSubmenu = (CommonMenu) => { | |
| return submenu | ||
| } | ||
|
|
||
| const createDebugSubmenu = () => { | ||
| return [ | ||
| { | ||
| // Makes future renderer processes pause when they are created until a debugger appears. | ||
|
|
@@ -510,26 +516,26 @@ const createDebugSubmenu = (CommonMenu) => { | |
| * Will only build the initial menu, which is mostly static items | ||
| * Dynamic items (Bookmarks, History) get updated w/ updateMenu | ||
| */ | ||
| const createMenu = () => { | ||
| const template = [ | ||
| { label: locale.translation('file'), submenu: createFileSubmenu() }, | ||
| { label: locale.translation('edit'), submenu: createEditSubmenu() }, | ||
| { label: locale.translation('view'), submenu: createViewSubmenu() }, | ||
| { label: locale.translation('history'), submenu: createHistorySubmenu() }, | ||
| { label: locale.translation('bookmarks'), submenu: createBookmarksSubmenu() }, | ||
| { | ||
| label: locale.translation('bravery'), | ||
| submenu: [ | ||
| CommonMenu.braveryGlobalMenuItem(), | ||
| CommonMenu.braverySiteMenuItem() | ||
| ] | ||
| }, | ||
| { label: locale.translation('window'), submenu: createWindowSubmenu(), role: 'window' }, | ||
| { label: locale.translation('help'), submenu: createHelpSubmenu(), role: 'help' } | ||
| ] | ||
|
|
||
| if (process.env.NODE_ENV === 'development') { | ||
| template.push({ label: 'Debug', submenu: createDebugSubmenu() }) | ||
| } | ||
|
|
||
| if (isDarwin) { | ||
|
|
@@ -572,81 +578,72 @@ const createMenu = (CommonMenu) => { | |
| }) | ||
| } | ||
|
|
||
| let oldMenu = appMenu | ||
| appMenu = Menu.buildFromTemplate(template) | ||
| Menu.setApplicationMenu(appMenu) | ||
| if (oldMenu) { | ||
| oldMenu.destroy() | ||
| } | ||
| } | ||
|
|
||
| const doAction = (action) => { | ||
| switch (action.actionType) { | ||
| case windowConstants.WINDOW_SET_FOCUSED_FRAME: | ||
| // TODO check/uncheck menu item instead of recreating menu | ||
|
There was a problem hiding this comment. Just a ? about this TODO; we solved it already, right? We fetch the item via getMenuItem and then set the checked status There was a problem hiding this comment. yep, nice catch. I added the comment before I changed that |
||
| currentLocation = action.frameProps.get('location') | ||
| let menuItem = menuUtil.getMenuItem(appMenu, locale.translation('bookmarkPage')) | ||
| menuItem.checked = isCurrentLocationBookmarked() | ||
| break | ||
| case windowConstants.WINDOW_UNDO_CLOSED_FRAME: | ||
| appDispatcher.waitFor([appStore.dispatchToken], () => { | ||
|
There was a problem hiding this comment. I had to lookup waitFor and I think I understand this now To make sure: this code is basically registering another handler for these actions on top of the existing ones in windowStore, right? This is really cool 😄 Great job! |
||
| delete closedFrames[action.frameProps.get('location')] | ||
| createMenu() | ||
| }) | ||
| break | ||
| case windowConstants.WINDOW_CLEAR_CLOSED_FRAMES: | ||
| appDispatcher.waitFor([appStore.dispatchToken], () => { | ||
| closedFrames = {} | ||
| createMenu() | ||
| }) | ||
| break | ||
| case windowConstants.WINDOW_CLOSE_FRAME: | ||
| appDispatcher.waitFor([appStore.dispatchToken], () => { | ||
| if (!action.frameProps.get('isPrivate') && action.frameProps.get('location') !== 'about:newtab') { | ||
| closedFrames[action.frameProps.get('location')] = action.frameProps | ||
| createMenu() | ||
| } | ||
| }) | ||
| break | ||
| case appConstants.APP_ADD_SITE: | ||
| if (action.tag === siteTags.BOOKMARK) { | ||
| appDispatcher.waitFor([appStore.dispatchToken], () => { | ||
| createMenu() | ||
| }) | ||
| } | ||
| break | ||
| case appConstants.APP_REMOVE_SITE: | ||
| if (action.tag === siteTags.BOOKMARK) { | ||
| appDispatcher.waitFor([appStore.dispatchToken], () => { | ||
| createMenu() | ||
| }) | ||
| } | ||
| break | ||
| case appConstants.APP_CLEAR_HISTORY: | ||
| if (action.tag === siteTags.BOOKMARK) { | ||
| appDispatcher.waitFor([appStore.dispatchToken], () => { | ||
| createMenu() | ||
| }) | ||
| } | ||
| break | ||
| default: | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Sets up the menu. | ||
| * @param {Object} appState - Application state. Used to fetch bookmarks and settings (like homepage) | ||
| */ | ||
| module.exports.init = (appState) => { | ||
| createMenu() | ||
| appDispatcher.register(doAction) | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nice cleanup with this method 😄 The previous code was hacky (because it could be changed in appStore OR in the navigationBar control) and there were definitely ways to cause it to fail (ex: shows checked but its not bookmarked)