From c5e6255d034394fd01cb07e6db6f35b2cf2006f7 Mon Sep 17 00:00:00 2001 From: NejcZdovc Date: Wed, 19 Apr 2017 18:53:07 +0200 Subject: [PATCH] Review fixes --- app/browser/reducers/tabsReducer.js | 15 +- app/browser/tabs.js | 12 +- app/browser/windows.js | 8 + app/common/state/contextMenuState.js | 132 +-------------- .../components/navigation/navigator.js | 8 +- app/renderer/reducers/contextMenuReducer.js | 155 ++++++++++++++++++ docs/appActions.md | 10 +- js/actions/appActions.js | 21 +-- js/actions/windowActions.js | 4 +- js/components/frame.js | 6 +- js/constants/appConstants.js | 10 +- js/constants/windowConstants.js | 4 +- js/stores/windowStore.js | 11 +- 13 files changed, 213 insertions(+), 183 deletions(-) create mode 100644 app/renderer/reducers/contextMenuReducer.js diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index 21b8f4d43c4..7232f3ee64c 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/app/browser/reducers/tabsReducer.js @@ -12,6 +12,7 @@ const windowAction = require('../../../js/actions/windowActions.js') const {makeImmutable} = require('../../common/state/immutableUtil') const {getFlashResourceId} = require('../../../js/flash') const {l10nErrorText} = require('../../common/lib/httpUtil') +const windows = require('../windows') const tabsReducer = (state, action) => { action = makeImmutable(action) @@ -72,20 +73,20 @@ const tabsReducer = (state, action) => { case appConstants.APP_LOAD_URL_IN_ACTIVE_TAB_REQUESTED: state = tabs.loadURLInActiveTab(state, action) break - case appConstants.APP_ON_NAVIGATE_BACK: + case appConstants.APP_ON_GO_BACK: state = tabs.goBack(state, action) break - case appConstants.APP_ON_NAVIGATE_FORWARD: + case appConstants.APP_ON_GO_FORWARD: state = tabs.goForward(state, action) break - case appConstants.APP_ON_NAVIGATE_INDEX: + case appConstants.APP_ON_GO_TO_INDEX: state = tabs.goToIndex(state, action) break - case appConstants.APP_ON_NAVIGATE_BACK_LONG: + case appConstants.APP_ON_GO_BACK_LONG: { const history = tabs.getHistoryEntries(state, action) const tabValue = tabState.getByTabId(state, action.get('tabId')) - const windowId = tabs.getWindowId() + const windowId = windows.getActiveWindowId() if (history !== null) { windowAction.onLongBackHistory( @@ -99,11 +100,11 @@ const tabsReducer = (state, action) => { } break } - case appConstants.APP_ON_NAVIGATE_FORWARD_LONG: + case appConstants.APP_ON_GO_FORWARD_LONG: { const history = tabs.getHistoryEntries(state, action) const tabValue = tabState.getByTabId(state, action.get('tabId')) - const windowId = tabs.getWindowId() + const windowId = windows.getActiveWindowId() if (history !== null) { windowAction.onLongForwardHistory( diff --git a/app/browser/tabs.js b/app/browser/tabs.js index 5b531a808af..a0392d2ddff 100644 --- a/app/browser/tabs.js +++ b/app/browser/tabs.js @@ -8,7 +8,7 @@ const Immutable = require('immutable') const tabState = require('../common/state/tabState') const {app, BrowserWindow, extensions, session, ipcMain} = require('electron') const {makeImmutable} = require('../common/state/immutableUtil') -const {getTargetAboutUrl, getSourceAboutUrl, isSourceAboutUrl, newFrameUrl} = require('../../js/lib/appUrlUtil') +const {getTargetAboutUrl, getSourceAboutUrl, isSourceAboutUrl, newFrameUrl, isTargetAboutUrl} = require('../../js/lib/appUrlUtil') const {isURL, getUrlFromInput, toPDFJSLocation, getDefaultFaviconUrl} = require('../../js/lib/urlutil') const {isSessionPartition} = require('../../js/state/frameStateUtil') const {getOrigin} = require('../../js/state/siteUtil') @@ -652,14 +652,6 @@ const api = { return state }, - getWindowId: () => { - if (BrowserWindow.getFocusedWindow()) { - return BrowserWindow.getFocusedWindow().id - } - - return -1 - }, - getHistoryEntries: (state, action) => { const tab = api.getWebContents(action.get('tabId')) const sites = state ? state.get('sites') : null @@ -682,7 +674,7 @@ const api = { icon: null } - if (url.startsWith('chrome-extension://')) { + if (isTargetAboutUrl(url)) { // TODO: return brave lion (or better: get icon from extension if possible as data URI) } else { if (sites) { diff --git a/app/browser/windows.js b/app/browser/windows.js index ee336614ba6..c609b6e9a53 100644 --- a/app/browser/windows.js +++ b/app/browser/windows.js @@ -328,6 +328,14 @@ const api = { getWindow: (windowId) => { return currentWindows[windowId] + }, + + getActiveWindowId: () => { + if (BrowserWindow.getFocusedWindow()) { + return BrowserWindow.getFocusedWindow().id + } + + return windowState.WINDOW_ID_NONE } } diff --git a/app/common/state/contextMenuState.js b/app/common/state/contextMenuState.js index 5d7bedf6f87..0f88ce240c1 100644 --- a/app/common/state/contextMenuState.js +++ b/app/common/state/contextMenuState.js @@ -3,20 +3,8 @@ * You can obtain one at http://mozilla.org/MPL/2.0/. */ const assert = require('assert') -const eventUtil = require('../../../js/lib/eventUtil.js') -const appActions = require('../../../js/actions/appActions.js') -const windowAction = require('../../../js/actions/windowActions.js') -const config = require('../../../js/constants/config.js') -const CommonMenu = require('../commonMenu.js') -const locale = require('../../../js/l10n.js') const { makeImmutable, isMap } = require('./immutableUtil') -const validateAction = function (action) { - action = makeImmutable(action) - assert.ok(isMap(action), 'action must be an Immutable.Map') - return action -} - const validateState = function (state) { state = makeImmutable(state) assert.ok(isMap(state), 'state must be an Immutable.Map') @@ -24,11 +12,11 @@ const validateState = function (state) { } const contextMenuState = { - setContextMenu: (state, action) => { - action = validateAction(action) + setContextMenu: (state, detail) => { + detail = makeImmutable(detail) state = validateState(state) - if (!action.get('detail')) { + if (!detail) { if (state.getIn(['contextMenuDetail', 'type']) === 'hamburgerMenu') { state = state.set('hamburgerMenuWasOpen', true) } else { @@ -36,122 +24,12 @@ const contextMenuState = { } state = state.delete('contextMenuDetail') } else { - if (!(action.getIn(['detail', 'type']) === 'hamburgerMenu' && state.get('hamburgerMenuWasOpen'))) { - state = state.set('contextMenuDetail', action.get('detail')) + if (!(detail.get('type') === 'hamburgerMenu' && state.get('hamburgerMenuWasOpen'))) { + state = state.set('contextMenuDetail', detail) } state = state.set('hamburgerMenuWasOpen', false) } - return state - }, - - onLongBacHistory: (state, action) => { - action = validateAction(action) - state = validateState(state) - const history = action.get('history') - - const menuTemplate = [] - - if (action.get('tabId') > -1 && history && history.get('entries').size > 0) { - const stopIndex = Math.max(((history.get('currentIndex') - config.navigationBar.maxHistorySites) - 1), -1) - for (let index = (history.get('currentIndex') - 1); index > stopIndex; index--) { - const entry = history.getIn(['entries', index]) - const url = entry.get('url') - - menuTemplate.push({ - label: entry.get('display'), - icon: entry.get('icon'), - click: function (e) { - if (eventUtil.isForSecondaryAction(e)) { - appActions.createTabRequested({ - url, - partitionNumber: action.get('partitionNumber'), - active: !!e.shiftKey - }) - } else { - appActions.onNavigateIndex(action.get('tabId'), index) - } - } - }) - } - - // Always display "Show History" link - menuTemplate.push( - CommonMenu.separatorMenuItem, - { - label: locale.translation('showAllHistory'), - click: function () { - appActions.createTabRequested({ - url: 'about:history' - }) - windowAction.setContextMenuDetail() - } - }) - - state = contextMenuState.setContextMenu(state, makeImmutable({ - detail: { - left: action.get('left'), - top: action.get('top'), - template: menuTemplate - } - })) - } - - return state - }, - - onLongForwardHistory: (state, action) => { - action = validateAction(action) - state = validateState(state) - const history = action.get('history') - - const menuTemplate = [] - - if (action.get('tabId') > -1 && history && history.get('entries').size > 0) { - const stopIndex = Math.min(((history.get('currentIndex') + config.navigationBar.maxHistorySites) + 1), history.get('entries').size) - for (let index = (history.get('currentIndex') + 1); index < stopIndex; index++) { - const entry = history.getIn(['entries', index]) - const url = entry.get('url') - - menuTemplate.push({ - label: entry.get('display'), - icon: entry.get('icon'), - click: function (e) { - if (eventUtil.isForSecondaryAction(e)) { - appActions.createTabRequested({ - url, - partitionNumber: action.get('partitionNumber'), - active: !!e.shiftKey - }) - } else { - appActions.onNavigateIndex(action.get('tabId'), index) - } - } - }) - } - - // Always display "Show History" link - menuTemplate.push( - CommonMenu.separatorMenuItem, - { - label: locale.translation('showAllHistory'), - click: function () { - appActions.createTabRequested({ - url: 'about:history' - }) - windowAction.setContextMenuDetail() - } - }) - - state = contextMenuState.setContextMenu(state, makeImmutable({ - detail: { - left: action.get('left'), - top: action.get('top'), - template: menuTemplate - } - })) - } - return state } } diff --git a/app/renderer/components/navigation/navigator.js b/app/renderer/components/navigation/navigator.js index a15cb3ac98e..4847fac4dc4 100644 --- a/app/renderer/components/navigation/navigator.js +++ b/app/renderer/components/navigation/navigator.js @@ -121,17 +121,17 @@ class Navigator extends ImmutableComponent { } onBack (e) { - this.onNav(e, 'canGoBack', 'back', appActions.onNavigateBack) + this.onNav(e, 'canGoBack', 'back', appActions.onGoBack) } onForward (e) { - this.onNav(e, 'canGoForward', 'forward', appActions.onNavigateForward) + this.onNav(e, 'canGoForward', 'forward', appActions.onGoForward) } onBackLongPress (target) { const activeTab = this.props.activeTab const rect = target.parentNode.getBoundingClientRect() - appActions.onNavigateBackLong(activeTab.get('tabId'), { + appActions.onGoBackLong(activeTab.get('tabId'), { left: rect.left, bottom: rect.bottom }) @@ -140,7 +140,7 @@ class Navigator extends ImmutableComponent { onForwardLongPress (target) { const activeTab = this.props.activeTab const rect = target.parentNode.getBoundingClientRect() - appActions.onNavigateForwardLong(activeTab.get('tabId'), { + appActions.onGoForwardLong(activeTab.get('tabId'), { left: rect.left, bottom: rect.bottom }) diff --git a/app/renderer/reducers/contextMenuReducer.js b/app/renderer/reducers/contextMenuReducer.js new file mode 100644 index 00000000000..5e878a032fe --- /dev/null +++ b/app/renderer/reducers/contextMenuReducer.js @@ -0,0 +1,155 @@ +/* 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/. */ + +const assert = require('assert') + +// Constants +const config = require('../../../js/constants/config.js') +const windowConstants = require('../../../js/constants/windowConstants') + +// State +const contextMenuState = require('../../common/state/contextMenuState.js') + +// Actions +const appActions = require('../../../js/actions/appActions.js') +const windowAction = require('../../../js/actions/windowActions.js') + +// Utils +const eventUtil = require('../../../js/lib/eventUtil.js') +const CommonMenu = require('../../common/commonMenu.js') +const locale = require('../../../js/l10n.js') +const { makeImmutable, isMap } = require('../../common/state/immutableUtil') + +const validateAction = function (action) { + action = makeImmutable(action) + assert.ok(isMap(action), 'action must be an Immutable.Map') + return action +} + +const validateState = function (state) { + state = makeImmutable(state) + assert.ok(isMap(state), 'state must be an Immutable.Map') + return state +} + +const onLongBackHistory = (state, action) => { + action = validateAction(action) + state = validateState(state) + const history = action.get('history') + + const menuTemplate = [] + + if (action.get('tabId') > -1 && history && history.get('entries').size > 0) { + const stopIndex = Math.max(((history.get('currentIndex') - config.navigationBar.maxHistorySites) - 1), -1) + for (let index = (history.get('currentIndex') - 1); index > stopIndex; index--) { + const entry = history.getIn(['entries', index]) + const url = entry.get('url') + + menuTemplate.push({ + label: entry.get('display'), + icon: entry.get('icon'), + click: function (e) { + if (eventUtil.isForSecondaryAction(e)) { + appActions.createTabRequested({ + url, + partitionNumber: action.get('partitionNumber'), + active: !!e.shiftKey + }) + } else { + appActions.onGoToIndex(action.get('tabId'), index) + } + } + }) + } + + // Always display "Show History" link + menuTemplate.push( + CommonMenu.separatorMenuItem, + { + label: locale.translation('showAllHistory'), + click: function () { + appActions.createTabRequested({ + url: 'about:history' + }) + windowAction.setContextMenuDetail() + } + }) + + state = contextMenuState.setContextMenu(state, makeImmutable({ + left: action.get('left'), + top: action.get('top'), + template: menuTemplate + })) + } + + return state +} + +const onLongForwardHistory = (state, action) => { + action = validateAction(action) + state = validateState(state) + const history = action.get('history') + + const menuTemplate = [] + + if (action.get('tabId') > -1 && history && history.get('entries').size > 0) { + const stopIndex = Math.min(((history.get('currentIndex') + config.navigationBar.maxHistorySites) + 1), history.get('entries').size) + for (let index = (history.get('currentIndex') + 1); index < stopIndex; index++) { + const entry = history.getIn(['entries', index]) + const url = entry.get('url') + + menuTemplate.push({ + label: entry.get('display'), + icon: entry.get('icon'), + click: function (e) { + if (eventUtil.isForSecondaryAction(e)) { + appActions.createTabRequested({ + url, + partitionNumber: action.get('partitionNumber'), + active: !!e.shiftKey + }) + } else { + appActions.onGoToIndex(action.get('tabId'), index) + } + } + }) + } + + // Always display "Show History" link + menuTemplate.push( + CommonMenu.separatorMenuItem, + { + label: locale.translation('showAllHistory'), + click: function () { + appActions.createTabRequested({ + url: 'about:history' + }) + windowAction.setContextMenuDetail() + } + }) + + state = contextMenuState.setContextMenu(state, makeImmutable({ + left: action.get('left'), + top: action.get('top'), + template: menuTemplate + })) + } + + return state +} + +const contextMenuReducer = (windowState, action) => { + switch (action.actionType) { + case windowConstants.WINDOW_ON_GO_BACK_LONG: + windowState = onLongBackHistory(windowState, action) + break + case windowConstants.WINDOW_ON_GO_FORWARD_LONG: + windowState = onLongForwardHistory(windowState, action) + break + } + + return windowState +} + +module.exports = contextMenuReducer diff --git a/docs/appActions.md b/docs/appActions.md index d022aadc7d3..d1099a82cbd 100644 --- a/docs/appActions.md +++ b/docs/appActions.md @@ -955,7 +955,7 @@ Notifies the app that a drop operation occurred -### onNavigateBack(tabId) +### onGoBack(tabId) Go back in a history for a given tab @@ -965,7 +965,7 @@ Go back in a history for a given tab -### onNavigateForward(tabId) +### onGoForward(tabId) Go forward in a history for a given tab @@ -975,7 +975,7 @@ Go forward in a history for a given tab -### onNavigateIndex(tabId, index) +### onGoToIndex(tabId, index) Go to specific item in a history for a given tab @@ -987,7 +987,7 @@ Go to specific item in a history for a given tab -### onNavigateBackLong(tabId, rect) +### onGoBackLong(tabId, rect) Go back in a history for a given tab @@ -999,7 +999,7 @@ Go back in a history for a given tab -### onNavigateForwardLong(tabId, rect) +### onGoForwardLong(tabId, rect) Go forward in a history for a given tab diff --git a/js/actions/appActions.js b/js/actions/appActions.js index 4fe05a28e6a..2cfbece5e28 100644 --- a/js/actions/appActions.js +++ b/js/actions/appActions.js @@ -1204,9 +1204,9 @@ const appActions = { * Go back in a history for a given tab * @param {number} tabId - Tab id used for an action */ - onNavigateBack: function (tabId) { + onGoBack: function (tabId) { AppDispatcher.dispatch({ - actionType: appConstants.APP_ON_NAVIGATE_BACK, + actionType: appConstants.APP_ON_GO_BACK, tabId }) }, @@ -1215,9 +1215,9 @@ const appActions = { * Go forward in a history for a given tab * @param {number} tabId - Tab id used for an action */ - onNavigateForward: function (tabId) { + onGoForward: function (tabId) { AppDispatcher.dispatch({ - actionType: appConstants.APP_ON_NAVIGATE_FORWARD, + actionType: appConstants.APP_ON_GO_FORWARD, tabId }) }, @@ -1227,10 +1227,11 @@ const appActions = { * @param {number} tabId - Tab id used for an action * @param {number} index - Index in the history */ - onNavigateIndex: function (tabId, index) { + onGoToIndex: function (tabId, index) { AppDispatcher.dispatch({ - actionType: appConstants.APP_ON_NAVIGATE_INDEX, + actionType: appConstants.APP_ON_GO_TO_INDEX, tabId, + index }) }, @@ -1240,9 +1241,9 @@ const appActions = { * @param {number} tabId - Tab id used for an action * @param {ClientRect} rect - Parent element position for this action */ - onNavigateBackLong: function (tabId, rect) { + onGoBackLong: function (tabId, rect) { AppDispatcher.dispatch({ - actionType: appConstants.APP_ON_NAVIGATE_BACK_LONG, + actionType: appConstants.APP_ON_GO_BACK_LONG, tabId, rect }) @@ -1253,9 +1254,9 @@ const appActions = { * @param {number} tabId - Tab id used for an action * @param {ClientRect} rect - Parent element position for this action */ - onNavigateForwardLong: function (tabId, rect) { + onGoForwardLong: function (tabId, rect) { AppDispatcher.dispatch({ - actionType: appConstants.APP_ON_NAVIGATE_FORWARD_LONG, + actionType: appConstants.APP_ON_GO_FORWARD_LONG, tabId, rect }) diff --git a/js/actions/windowActions.js b/js/actions/windowActions.js index 070adccd2b2..ef4dfa1e8e1 100644 --- a/js/actions/windowActions.js +++ b/js/actions/windowActions.js @@ -1183,7 +1183,7 @@ const windowActions = { onLongBackHistory: function (history, left, top, partition, tabId, windowId) { dispatch({ - actionType: windowConstants.WINDOW_ON_NAVIGATE_BACK_LONG, + actionType: windowConstants.WINDOW_ON_GO_BACK_LONG, queryInfo: { windowId }, @@ -1197,7 +1197,7 @@ const windowActions = { onLongForwardHistory: function (history, left, top, partition, tabId, windowId) { dispatch({ - actionType: windowConstants.WINDOW_ON_NAVIGATE_FORWARD_LONG, + actionType: windowConstants.WINDOW_ON_GO_FORWARD_LONG, queryInfo: { windowId }, diff --git a/js/components/frame.js b/js/components/frame.js index fafc6756e32..ea2ea4cc1bf 100644 --- a/js/components/frame.js +++ b/js/components/frame.js @@ -629,10 +629,10 @@ class Frame extends ImmutableComponent { method = () => this.webview.stop() break case messages.GO_BACK: - method = () => appActions.onNavigateBack(this.props.tabId) + method = () => appActions.onGoBack(this.props.tabId) break case messages.GO_FORWARD: - method = () => appActions.onNavigateForward(this.props.tabId) + method = () => appActions.onGoForward(this.props.tabId) break case messages.RELOAD: method = () => { @@ -728,7 +728,7 @@ class Frame extends ImmutableComponent { } else if (isTargetAboutUrl(e.validatedURL)) { // open a new tab for other about urls // and send this tab back to wherever it came from - appActions.tabNavigateBack(this.props.tabId) + appActions.onGoBack(this.props.tabId) appActions.createTabRequested({ url: e.validatedURL, active: true diff --git a/js/constants/appConstants.js b/js/constants/appConstants.js index 46460e6e860..5ef90c36054 100644 --- a/js/constants/appConstants.js +++ b/js/constants/appConstants.js @@ -116,11 +116,11 @@ const appConstants = { APP_DRAG_STOPPED: _, APP_DATA_DROPPED: _, APP_DRAGGED_OVER: _, - APP_ON_NAVIGATE_BACK: _, - APP_ON_NAVIGATE_FORWARD: _, - APP_ON_NAVIGATE_INDEX: _, - APP_ON_NAVIGATE_BACK_LONG: _, - APP_ON_NAVIGATE_FORWARD_LONG: _ + APP_ON_GO_BACK: _, + APP_ON_GO_FORWARD: _, + APP_ON_GO_TO_INDEX: _, + APP_ON_GO_BACK_LONG: _, + APP_ON_GO_FORWARD_LONG: _ } module.exports = mapValuesByKeys(appConstants) diff --git a/js/constants/windowConstants.js b/js/constants/windowConstants.js index 1a5f6e5228e..b67fdcf97a2 100644 --- a/js/constants/windowConstants.js +++ b/js/constants/windowConstants.js @@ -102,8 +102,8 @@ const windowConstants = { WINDOW_SHOULD_EXIT_FULL_SCREEN: _, WINDOW_SHOULD_OPEN_DEV_TOOLS: _, WINDOW_SET_ALL_AUDIO_MUTED: _, - WINDOW_ON_NAVIGATE_BACK_LONG: _, - WINDOW_ON_NAVIGATE_FORWARD_LONG: _ + WINDOW_ON_GO_BACK_LONG: _, + WINDOW_ON_GO_FORWARD_LONG: _ } module.exports = mapValuesByKeys(windowConstants) diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js index e35f38fd67c..3f456e15193 100644 --- a/js/stores/windowStore.js +++ b/js/stores/windowStore.js @@ -238,7 +238,8 @@ const emitChanges = debounce(windowStore.emitChanges.bind(windowStore), 5) const applyReducers = (state, action) => [ require('../../app/renderer/reducers/urlBarReducer'), require('../../app/renderer/reducers/urlBarSuggestionsReducer'), - require('../../app/renderer/reducers/frameReducer') + require('../../app/renderer/reducers/frameReducer'), + require('../../app/renderer/reducers/contextMenuReducer') ].reduce( (windowState, reducer) => { const newState = reducer(windowState, action) @@ -473,13 +474,7 @@ const doAction = (action) => { } break case windowConstants.WINDOW_SET_CONTEXT_MENU_DETAIL: - windowState = contextMenuState.setContextMenu(windowState, action) - break - case windowConstants.WINDOW_ON_NAVIGATE_BACK_LONG: - windowState = contextMenuState.onLongBacHistory(windowState, action) - break - case windowConstants.WINDOW_ON_NAVIGATE_FORWARD_LONG: - windowState = contextMenuState.onLongForwardHistory(windowState, action) + windowState = contextMenuState.setContextMenu(windowState, action.detail) break case windowConstants.WINDOW_SET_POPUP_WINDOW_DETAIL: if (!action.detail) {