From 95455e729f62747391162ce044acfb115ef967f0 Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Sat, 22 Jul 2017 22:55:18 -0400 Subject: [PATCH] Use muon's tab_strip_model for driving index and active Fix #10436 Fix #9385 Fix #9722 Fix #10561 Fix #9083 Fix #9671 Fix #10038 Fix #10384 Fix #10532 and probably several others. --- app/browser/reducers/tabsReducer.js | 103 +++++------- app/browser/reducers/windowsReducer.js | 9 +- app/browser/tabs.js | 148 ++++++++++++++++-- app/browser/windows.js | 15 +- app/common/state/tabState.js | 31 +--- app/renderer/components/frame/frame.js | 8 +- app/renderer/components/tabs/tab.js | 12 +- app/renderer/reducers/frameReducer.js | 9 +- app/sessionStore.js | 29 ++-- js/actions/appActions.js | 48 +++++- js/actions/windowActions.js | 7 - js/constants/appConstants.js | 4 + js/contextMenus.js | 2 +- js/state/frameStateUtil.js | 32 ++-- js/stores/windowStore.js | 46 ++---- test/lib/brave.js | 2 +- .../app/browser/reducers/tabsReducerTest.js | 47 +++--- test/unit/app/browser/tabsTest.js | 5 +- test/unit/js/stores/windowStoreTest.js | 25 ++- 19 files changed, 356 insertions(+), 226 deletions(-) diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index 47f608205c3..c70f1c3f101 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/app/browser/reducers/tabsReducer.js @@ -11,7 +11,6 @@ const windows = require('../windows') const {getWebContents} = require('../webContentsCache') const {BrowserWindow} = require('electron') const tabState = require('../../common/state/tabState') -const windowState = require('../../common/state/windowState') const tabActions = require('../../common/actions/tabActions') const siteSettings = require('../../../js/state/siteSettings') const siteSettingsState = require('../../common/state/siteSettingsState') @@ -22,62 +21,8 @@ const {getFlashResourceId} = require('../../../js/flash') const {l10nErrorText} = require('../../common/lib/httpUtil') const Immutable = require('immutable') const dragTypes = require('../../../js/constants/dragTypes') -const getSetting = require('../../../js/settings').getSetting -const settings = require('../../../js/constants/settings') -const {tabCloseAction} = require('../../common/constants/settingsEnums') const {frameOptsFromFrame} = require('../../../js/state/frameStateUtil') -const updateActiveTab = (state, closeTabId) => { - if (!tabState.getByTabId(state, closeTabId)) { - return - } - - const index = tabState.getIndex(state, closeTabId) - if (index === -1) { - return - } - - const windowId = tabState.getWindowId(state, closeTabId) - if (windowId === windowState.WINDOW_ID_NONE) { - return - } - - const lastActiveTabId = tabState.getTabsByLastActivated(state, windowId).last() - if (lastActiveTabId !== closeTabId && !tabState.isActive(state, closeTabId)) { - return - } - - let nextTabId = tabState.TAB_ID_NONE - switch (getSetting(settings.TAB_CLOSE_ACTION)) { - case tabCloseAction.LAST_ACTIVE: - nextTabId = tabState.getLastActiveTabId(state, windowId) - break - case tabCloseAction.PARENT: - { - const openerTabId = tabState.getOpenerTabId(state, closeTabId) - if (openerTabId !== tabState.TAB_ID_NONE) { - nextTabId = openerTabId - } - break - } - } - - // DEFAULT: always fall back to NEXT - if (nextTabId === tabState.TAB_ID_NONE) { - nextTabId = tabState.getNextTabIdByIndex(state, windowId, index) - if (nextTabId === tabState.TAB_ID_NONE) { - // no unpinned tabs so find the next pinned tab - nextTabId = tabState.getNextTabIdByIndex(state, windowId, index, true) - } - } - - if (nextTabId !== tabState.TAB_ID_NONE) { - setImmediate(() => { - tabs.setActive(nextTabId) - }) - } -} - const WEBRTC_DEFAULT = 'default' const WEBRTC_DISABLE_NON_PROXY = 'disable_non_proxied_udp' @@ -125,7 +70,23 @@ const tabsReducer = (state, action, immutableAction) => { case appConstants.APP_TAB_CREATED: state = tabState.maybeCreateTab(state, action) break - case appConstants.APP_TAB_MOVED: { + case appConstants.APP_TAB_ATTACHED: + tabs.updateTabsStateForAttachedTab(state, action.get('tabId')) + break + case appConstants.APP_TAB_WILL_ATTACH: { + const tabId = action.get('tabId') + const tabValue = tabState.getByTabId(state, tabId) + if (!tabValue) { + break + } + const oldWindowId = tabState.getWindowId(state, tabId) + tabs.updateTabsStateForWindow(state, oldWindowId) + break + } + case appConstants.APP_TAB_MOVED: + tabs.updateTabsStateForAttachedTab(state, action.get('tabId')) + break + case appConstants.APP_TAB_DETACH_MENU_ITEM_CLICKED: { setImmediate(() => { const tabId = action.get('tabId') const frameOpts = frameOptsFromFrame(action.get('frameOpts')) @@ -153,6 +114,7 @@ const tabsReducer = (state, action, immutableAction) => { break case appConstants.APP_TAB_UPDATED: state = tabState.maybeCreateTab(state, action) + // tabs.debugTabs(state) break case appConstants.APP_TAB_CLOSE_REQUESTED: { @@ -189,7 +151,7 @@ const tabsReducer = (state, action, immutableAction) => { tabs.closeTab(tabId, action.get('forceClosePinned')) }) } else { - state = windows.closeWindow(state, windowId) + windows.closeWindow(windowId) } } } @@ -201,8 +163,21 @@ const tabsReducer = (state, action, immutableAction) => { if (tabId === tabState.TAB_ID_NONE) { break } - updateActiveTab(state, tabId) + const nextActiveTabId = tabs.getNextActiveTab(state, tabId) + + // Must be called before tab is removed + // But still check for no tabId because on tab detach there's a dummy tabId + const tabValue = tabState.getByTabId(state, tabId) + if (tabValue) { + const windowIdOfTabBeingRemoved = tabState.getWindowId(state, tabId) + tabs.updateTabsStateForWindow(state, windowIdOfTabBeingRemoved) + } state = tabState.removeTabByTabId(state, tabId) + setImmediate(() => { + if (nextActiveTabId !== tabState.TAB_ID_NONE) { + tabs.setActive(nextActiveTabId) + } + }) } break case appConstants.APP_ALLOW_FLASH_ONCE: @@ -362,10 +337,16 @@ const tabsReducer = (state, action, immutableAction) => { const dragData = state.get('dragData') if (dragData && dragData.get('type') === dragTypes.TAB) { const frame = dragData.get('data') - const frameOpts = frameOptsFromFrame(frame).toJS() + let frameOpts = frameOptsFromFrame(frame) const browserOpts = { positionByMouseCursor: true } - frameOpts.indexByFrameKey = dragData.getIn(['dragOverData', 'draggingOverKey']) - frameOpts.prependIndexByFrameKey = dragData.getIn(['dragOverData', 'draggingOverLeftHalf']) + const tabIdForIndex = dragData.getIn(['dragOverData', 'draggingOverKey']) + const tabForIndex = tabState.getByTabId(state, tabIdForIndex) + const dropWindowId = dragData.get('dropWindowId') + if (dropWindowId != null && dropWindowId !== -1 && tabForIndex) { + const prependIndexByTabId = dragData.getIn(['dragOverData', 'draggingOverLeftHalf']) + frameOpts = frameOpts.set('index', tabForIndex.get('index') + (prependIndexByTabId ? 0 : 1)) + } + tabs.moveTo(state, frame.get('tabId'), frameOpts, browserOpts, dragData.get('dropWindowId')) } break diff --git a/app/browser/reducers/windowsReducer.js b/app/browser/reducers/windowsReducer.js index 358ca04b709..f830d61136e 100644 --- a/app/browser/reducers/windowsReducer.js +++ b/app/browser/reducers/windowsReducer.js @@ -28,11 +28,13 @@ const windowsReducer = (state, action, immutableAction) => { } break case appConstants.APP_CLOSE_WINDOW: - state = windows.closeWindow(state, action.get('windowId')) + windows.closeWindow(action.get('windowId')) break case appConstants.APP_WINDOW_CLOSED: state = windowState.removeWindow(state, action) - sessionStoreShutdown.removeWindowFromCache(action.getIn(['windowValue', 'windowId'])) + const windowId = action.getIn(['windowValue', 'windowId']) + sessionStoreShutdown.removeWindowFromCache(windowId) + windows.cleanupWindow(windowId) break case appConstants.APP_WINDOW_CREATED: state = windowState.maybeCreateWindow(state, action) @@ -40,6 +42,9 @@ const windowsReducer = (state, action, immutableAction) => { case appConstants.APP_WINDOW_UPDATED: state = windowState.maybeCreateWindow(state, action) break + case appConstants.APP_TAB_STRIP_EMPTY: + windows.closeWindow(action.get('windowId')) + break case appConstants.APP_DEFAULT_WINDOW_PARAMS_CHANGED: if (action.get('size')) { state = state.setIn(['defaultWindowParams', 'width'], action.getIn(['size', 0])) diff --git a/app/browser/tabs.js b/app/browser/tabs.js index a2d897312e3..51b41226ac7 100644 --- a/app/browser/tabs.js +++ b/app/browser/tabs.js @@ -8,6 +8,7 @@ const tabActions = require('../common/actions/tabActions') const config = require('../../js/constants/config') const Immutable = require('immutable') const tabState = require('../common/state/tabState') +const windowState = require('../common/state/windowState') const {app, BrowserWindow, extensions, session, ipcMain} = require('electron') const {makeImmutable} = require('../common/state/immutableUtil') const {getTargetAboutUrl, getSourceAboutUrl, isSourceAboutUrl, newFrameUrl, isTargetAboutUrl, isIntermediateAboutPage, isTargetMagnetUrl, getSourceMagnetUrl} = require('../../js/lib/appUrlUtil') @@ -24,6 +25,7 @@ const appStore = require('../../js/stores/appStore') const appConfig = require('../../js/constants/appConfig') const siteTags = require('../../js/constants/siteTags') const {newTabMode} = require('../common/constants/settingsEnums') +const {tabCloseAction} = require('../common/constants/settingsEnums') const {cleanupWebContents, currentWebContents, getWebContents, updateWebContents} = require('./webContentsCache') const {FilterOptions} = require('ad-block') const {isResourceEnabled} = require('../filtering') @@ -436,8 +438,7 @@ const api = { if (tab.isBackgroundPage() || !tab.isGuest()) { return } - let tabId = tab.getId() - + const tabId = tab.getId() tab.on('did-start-navigation', (e, navigationHandle) => { if (!tab.isDestroyed() && navigationHandle.isValid() && navigationHandle.isInMainFrame()) { const controller = tab.controller() @@ -473,10 +474,35 @@ const api = { tab.on('unresponsive', () => { console.log('unresponsive') }) + tab.on('responsive', () => { console.log('responsive') }) + tab.on('tab-changed-at', () => { + updateTab(tabId) + }) + + tab.on('tab-moved', () => { + appActions.tabMoved(tabId) + }) + + tab.on('will-attach', () => { + appActions.tabWillAttach(tab.getId()) + }) + + tab.on('tab-strip-empty', () => { + tab.once('will-attach', () => { + const tabValue = getTabValue(tabId) + const windowId = tabValue.get('windowId') + appActions.tabStripEmpty(windowId) + }) + }) + + tab.on('did-attach', () => { + appActions.tabAttached(tab.getId()) + }) + tab.on('save-password', (e, username, origin) => { appActions.savePassword(username, origin, tabId) }) @@ -611,8 +637,11 @@ const api = { const tabId = action.get('tabId') let options = action.get('options') || Immutable.Map() const tabValue = getTabValue(tabId) - if (tabValue && tabValue.get('index') !== undefined) { - options = options.set('index', tabValue.get('index') + 1) + if (tabValue) { + const index = tabValue.get('index') + if (index !== undefined) { + options = options.set('index', index + 1) + } } const tab = getWebContents(tabId) if (tab && !tab.isDestroyed()) { @@ -723,7 +752,7 @@ const api = { win.loadURL('about:blank') }, - moveTo: (state, tabId, frameOpts, browserOpts, windowId) => { + moveTo: (state, tabId, frameOpts, browserOpts, toWindowId) => { frameOpts = makeImmutable(frameOpts) browserOpts = makeImmutable(browserOpts) const tab = getWebContents(tabId) @@ -735,11 +764,11 @@ const api = { } const currentWindowId = tabValue && tabValue.get('windowId') - if (windowId != null && currentWindowId === windowId) { + if (toWindowId != null && currentWindowId === toWindowId) { return } - if (windowId == null || windowId === -1) { + if (toWindowId == null || toWindowId === -1) { // If there's only one tab and we're dragging outside the window, then disallow // a new window to be created. const windowTabCount = tabState.getTabsByWindowId(state, currentWindowId).size @@ -752,13 +781,27 @@ const api = { // If the current tab is pinned, then don't allow to drag out return } - + const nextActiveTabIdForOldWindow = api.getNextActiveTab(state, tabId) tab.detach(() => { - if (windowId == null || windowId === -1) { - appActions.newWindow(makeImmutable(frameOpts), browserOpts) + if (toWindowId == null || toWindowId === -1) { + frameOpts = frameOpts.set('index', 0) + appActions.newWindow(frameOpts, browserOpts) } else { - appActions.newWebContentsAdded(windowId, frameOpts, tabValue) + appActions.newWebContentsAdded(toWindowId, frameOpts, tabValue) } + + // Setting the next active tab for the old window must happen after re-attach of the new tab. + // This is because muon's tab_strip index for the tab would not be consistent with browser-laptop's + // expectation and it would try to set an invalid index as active, possibly leaivng nothing active. + tab.once('did-attach', () => { + if (nextActiveTabIdForOldWindow !== tabState.TAB_ID_NONE) { + api.setActive(nextActiveTabIdForOldWindow) + } + const index = frameOpts.get('index') + if (index !== undefined) { + api.setTabIndex(tabId, frameOpts.get('index')) + } + }) }) } }, @@ -857,6 +900,89 @@ const api = { } return null + }, + + getNextActiveTab: (state, closeTabId) => { + if (!tabState.getByTabId(state, closeTabId)) { + return + } + + const index = tabState.getIndex(state, closeTabId) + if (index === -1) { + return + } + + const windowId = tabState.getWindowId(state, closeTabId) + if (windowId === windowState.WINDOW_ID_NONE) { + return + } + + const lastActiveTabId = tabState.getTabsByLastActivated(state, windowId).last() + if (lastActiveTabId !== closeTabId && !tabState.isActive(state, closeTabId)) { + return + } + + let nextTabId = tabState.TAB_ID_NONE + switch (getSetting(settings.TAB_CLOSE_ACTION)) { + case tabCloseAction.LAST_ACTIVE: + nextTabId = tabState.getLastActiveTabId(state, windowId) + break + case tabCloseAction.PARENT: + { + const openerTabId = tabState.getOpenerTabId(state, closeTabId) + if (openerTabId !== tabState.TAB_ID_NONE) { + nextTabId = openerTabId + } + break + } + } + + // DEFAULT: always fall back to NEXT + if (nextTabId === tabState.TAB_ID_NONE) { + nextTabId = tabState.getNextTabIdByIndex(state, windowId, index) + if (nextTabId === tabState.TAB_ID_NONE) { + // no unpinned tabs so find the next pinned tab + nextTabId = tabState.getNextTabIdByIndex(state, windowId, index, true) + } + } + + return nextTabId + }, + debugTabs: (state) => { + console.log(tabState.getTabs(state) + .toJS() + .map((tab) => { + return { + tabId: tab.tabId, + index: tab.index, + windowId: tab.windowId, + active: tab.active + } + }) + .sort((tab1, tab2) => { + if (tab1.windowId !== tab2.windowId) { + return tab1.windowId - tab2.windowId + } + if (tab1.index !== tab2.index) { + return tab1.index - tab2.index + } + return 0 + })) + }, + updateTabsStateForAttachedTab: (state, tabId) => { + const tabValue = getTabValue(tabId) + if (!tabValue) { + return + } + api.updateTabsStateForWindow(state, tabValue.get('windowId')) + }, + updateTabsStateForWindow: (state, windowId) => { + tabState.getTabsByWindowId(state, windowId).forEach((tabValue) => { + const newTabValue = getTabValue(tabValue.get('tabId')) + if (newTabValue) { + state = tabState.updateTabValue(state, newTabValue, false) + } + }) } } diff --git a/app/browser/windows.js b/app/browser/windows.js index 809faac8858..962c900f7c8 100644 --- a/app/browser/windows.js +++ b/app/browser/windows.js @@ -21,11 +21,6 @@ const Immutable = require('immutable') // TODO(bridiver) - set window uuid let currentWindows = {} -const cleanupWindow = (windowId) => { - delete currentWindows[windowId] - appActions.windowClosed({ windowId }) -} - const getWindowState = (win) => { if (win.isFullScreen()) { return 'fullscreen' @@ -134,6 +129,7 @@ const api = { win.webContents.once('close', () => { LocalShortcuts.unregister(win) }) + win.once('close', () => { LocalShortcuts.unregister(win) }) @@ -197,7 +193,6 @@ const api = { appActions.windowCreated(windowValue) }) win.once('closed', () => { - cleanupWindow(windowId) }) win.on('blur', () => { appActions.windowBlurred(windowId) @@ -329,7 +324,7 @@ const api = { }) }, - closeWindow: (state, windowId) => { + closeWindow: (windowId) => { let win = api.getWindow(windowId) try { setImmediate(() => { @@ -340,7 +335,6 @@ const api = { } catch (e) { // ignore } - return windowState.removeWindowByWindowId(state, windowId) }, getWindow: (windowId) => { @@ -351,8 +345,11 @@ const api = { if (BrowserWindow.getFocusedWindow()) { return BrowserWindow.getFocusedWindow().id } - return windowState.WINDOW_ID_NONE + }, + + cleanupWindow: (windowId) => { + delete currentWindows[windowId] } } diff --git a/app/common/state/tabState.js b/app/common/state/tabState.js index 3938cf8ccb5..28632076929 100644 --- a/app/common/state/tabState.js +++ b/app/common/state/tabState.js @@ -97,27 +97,11 @@ const updateLastActive = (state, oldTabValue, newTabValue) => { const getTabIdByDisplayIndex = (state, windowId, index) => { index = validateIndex(index) windowId = validateId('windowId', windowId) - - const tabId = state.getIn(['tabsInternal', 'displayIndex', windowId.toString(), index.toString()]) - return tabId == null ? tabState.TAB_ID_NONE : tabId -} - -const updateTabsDisplayIndex = (state, oldTabValue, newTabValue) => { - const oldTabId = validateId('tabId', oldTabValue.get('tabId')) - const oldWindowId = validateId('windowId', oldTabValue.get('windowId')) - const oldIndex = validateIndex(oldTabValue.get('index')) - if (oldIndex !== -1 && oldTabId !== tabState.TAB_ID_NONE && oldWindowId !== windowState.WINDOW_ID_NONE) { - state = state.deleteIn(['tabsInternal', 'displayIndex', oldWindowId.toString(), oldIndex.toString()]) - } - - const tabId = validateId('tabId', newTabValue.get('tabId')) - const windowId = validateId('windowId', newTabValue.get('windowId')) - const index = validateIndex(newTabValue.get('index')) - if (index !== -1 && tabId !== tabState.TAB_ID_NONE && windowId !== windowState.WINDOW_ID_NONE) { - return state.setIn(['tabsInternal', 'displayIndex', windowId.toString(), index.toString()], tabId) + const tabValue = tabState.queryTab(state, {windowId, index}) + if (!tabValue) { + return tabState.TAB_ID_NONE } - - return state + return tabValue.get('tabId') } // The internal index is the location of the tabId in the tabs array @@ -134,12 +118,7 @@ const deleteTabsInternalIndex = (state, tabValue) => { if (tabId === tabState.TAB_ID_NONE) { return state } - const windowId = validateId('windowId', tabValue.get('windowId')) - const displayIndex = validateIndex(tabValue.get('index')) - if (displayIndex !== -1 && windowId !== windowState.WINDOW_ID_NONE) { - state = state.deleteIn(['tabsInternal', 'displayIndex', windowId.toString(), displayIndex.toString()]) - } if (windowId !== windowState.WINDOW_ID_NONE) { let activeList = state.getIn(['tabsInternal', 'lastActive', windowId.toString()], Immutable.OrderedSet()) activeList = activeList.remove(tabId) @@ -211,7 +190,6 @@ const tabState = { let tabValue = validateTabValue(action.get('tabValue')) assert.ok(!tabState.getTab(state, tabValue), 'Tab already exists') state = state.set('tabs', state.get('tabs').push(tabValue)) - state = updateTabsDisplayIndex(state, tabValue, tabValue) state = updateLastActive(state, tabValue, tabValue) return updateTabsInternalIndex(state, state.get('tabs').size - 1) }, @@ -416,7 +394,6 @@ const tabState = { tabValue = currentTabValue.mergeDeep(tabValue) } - state = updateTabsDisplayIndex(state, currentTabValue, tabValue) state = updateLastActive(state, currentTabValue, tabValue) return state.set('tabs', tabs.delete(index).insert(index, tabValue)) }, diff --git a/app/renderer/components/frame/frame.js b/app/renderer/components/frame/frame.js index 499222e18e0..2043cfd518a 100644 --- a/app/renderer/components/frame/frame.js +++ b/app/renderer/components/frame/frame.js @@ -161,12 +161,14 @@ class Frame extends React.Component { this.addEventListeners() if (cb) { this.runOnDomReady = cb - let eventCallback = (e) => { - this.webview.removeEventListener(e.type, eventCallback) + let didAttachCallback = (e) => { + this.webview.removeEventListener(e.type, didAttachCallback) this.runOnDomReady() delete this.runOnDomReady } - this.webview.addEventListener('did-attach', eventCallback, { passive: true }) + this.webview.addEventListener('will-attach', () => { + }) + this.webview.addEventListener('did-attach', didAttachCallback, { passive: true }) } if (!this.props.guestInstanceId || !this.webview.attachGuest(this.props.guestInstanceId)) { diff --git a/app/renderer/components/tabs/tab.js b/app/renderer/components/tabs/tab.js index 311f89924d6..392862aebd1 100644 --- a/app/renderer/components/tabs/tab.js +++ b/app/renderer/components/tabs/tab.js @@ -66,7 +66,7 @@ class Tab extends React.Component { get draggingOverData () { const draggingOverData = this.props.dragData && this.props.dragData.get('dragOverData') if (!draggingOverData || - draggingOverData.get('draggingOverKey') !== this.props.frameKey || + draggingOverData.get('draggingOverKey') !== this.props.tabId || draggingOverData.get('draggingOverWindowId') !== getCurrentWindowId()) { return } @@ -76,8 +76,8 @@ class Tab extends React.Component { return } const location = sourceDragData.get('location') - const key = draggingOverData.get('draggingOverKey') - const draggingOverFrame = windowStore.getFrame(key) + const tabId = draggingOverData.get('draggingOverKey') + const draggingOverFrame = windowStore.getFrameByTabId(tabId) if ((location === 'about:blank' || location === 'about:newtab' || isIntermediateAboutPage(location)) && (draggingOverFrame && draggingOverFrame.get('pinnedLocation'))) { return @@ -89,7 +89,7 @@ class Tab extends React.Component { get isDragging () { const sourceDragData = dnd.getInterBraveDragData() return sourceDragData && - sourceDragData.get('key') === this.props.frameKey && + sourceDragData.get('tabId') === this.props.tabId && sourceDragData.get('draggingOverWindowId') === getCurrentWindowId() } @@ -99,7 +99,7 @@ class Tab extends React.Component { if (!draggingOverData || !sourceDragData) { return false } - return draggingOverData.get('draggingOverKey') === sourceDragData.get('key') + return draggingOverData.get('draggingOverKey') === sourceDragData.get('tabId') } get isDraggingOverLeft () { @@ -125,7 +125,7 @@ class Tab extends React.Component { } onDragOver (e) { - dnd.onDragOver(dragTypes.TAB, this.tabNode.getBoundingClientRect(), this.props.frameKey, this.draggingOverData, e) + dnd.onDragOver(dragTypes.TAB, this.tabNode.getBoundingClientRect(), this.props.tabId, this.draggingOverData, e) } onMouseLeave () { diff --git a/app/renderer/reducers/frameReducer.js b/app/renderer/reducers/frameReducer.js index cec4b5ab2ab..831df7d4f9c 100644 --- a/app/renderer/reducers/frameReducer.js +++ b/app/renderer/reducers/frameReducer.js @@ -16,7 +16,6 @@ const appActions = require('../../../js/actions/appActions') // Utils const frameStateUtil = require('../../../js/state/frameStateUtil') -const {getCurrentWindowId} = require('../currentWindow') const {getSourceAboutUrl, getSourceMagnetUrl} = require('../../../js/lib/appUrlUtil') const {isURL, isPotentialPhishingUrl, getUrlFromInput} = require('../../../js/lib/urlutil') @@ -39,16 +38,14 @@ const closeFrame = (state, action) => { state = state.merge(frameStateUtil.removeFrame( state, - frameProps.set('closedAtIndex', index), + frameProps + .set('closedAtIndex', index) + .delete('openerTabId'), index )) state = frameStateUtil.deleteFrameInternalIndex(state, frameProps) state = frameStateUtil.updateFramesInternalIndex(state, index) - if (state.get('frames', Immutable.List()).size === 0) { - appActions.closeWindow(getCurrentWindowId()) - } - const nextFrame = frameStateUtil.getFrameByIndex(state, index) if (nextFrame) { diff --git a/app/sessionStore.js b/app/sessionStore.js index df1415c1339..c1bb195120f 100644 --- a/app/sessionStore.js +++ b/app/sessionStore.js @@ -154,11 +154,15 @@ module.exports.cleanPerWindowData = (immutablePerWindowData, isShutdown) => { } let newKey = 0 - const cleanFrame = (immutableFrame) => { + let activeFrameKey = immutablePerWindowData.get('activeFrameKey') + // If adjustActive is set to true then activeFrameKey will be set to the new frame key. + // We re-use this function for both closedFrames and frames, and we only want to adjust the active for frames. + const cleanFrame = (immutableFrame, adjustActive) => { newKey++ // Reset the ids back to sequential numbers - if (immutableFrame.get('key') === immutablePerWindowData.get('activeFrameKey')) { - immutablePerWindowData = immutablePerWindowData.set('activeFrameKey', newKey) + if (adjustActive && + immutableFrame.get('key') === immutablePerWindowData.get('activeFrameKey')) { + activeFrameKey = newKey } else { // For now just set everything to unloaded unless it's the active frame immutableFrame = immutableFrame.set('unloaded', true) @@ -193,6 +197,7 @@ module.exports.cleanPerWindowData = (immutablePerWindowData, isShutdown) => { 'guestInstanceId', // Tab ids are per-session and should not be persisted 'tabId', + 'openerTabId', // Do not show the audio indicator until audio starts playing 'audioMuted', 'audioPlaybackActive', @@ -246,20 +251,24 @@ module.exports.cleanPerWindowData = (immutablePerWindowData, isShutdown) => { if (immutablePerWindowData.get('closedFrames')) { immutablePerWindowData = immutablePerWindowData.get('closedFrames').reduce((immutablePerWindowData, immutableFrame, index) => { - const cleanImmutableFrame = cleanFrame(immutableFrame) + const cleanImmutableFrame = cleanFrame(immutableFrame, false) return immutablePerWindowData.setIn(['closedFrames', index], cleanImmutableFrame) }, immutablePerWindowData) } - if (immutablePerWindowData.get('frames')) { + let immutableFrames = immutablePerWindowData.get('frames') + if (immutableFrames) { // Don't restore pinned locations because they will be auto created by the app state change event - immutablePerWindowData = immutablePerWindowData.set('frames', - immutablePerWindowData.get('frames') - .filter((frame) => !frame.get('pinnedLocation'))) + immutableFrames = immutableFrames + .filter((frame) => !frame.get('pinnedLocation')) + immutablePerWindowData = immutablePerWindowData.set('frames', immutableFrames) immutablePerWindowData = - immutablePerWindowData.get('frames').reduce((immutablePerWindowData, immutableFrame, index) => { - const cleanImmutableFrame = cleanFrame(immutableFrame) + immutableFrames.reduce((immutablePerWindowData, immutableFrame, index) => { + const cleanImmutableFrame = cleanFrame(immutableFrame, true) return immutablePerWindowData.setIn(['frames', index], cleanImmutableFrame) }, immutablePerWindowData) + if (activeFrameKey !== undefined) { + immutablePerWindowData = immutablePerWindowData.set('activeFrameKey', activeFrameKey) + } } return immutablePerWindowData } diff --git a/js/actions/appActions.js b/js/actions/appActions.js index 280171f9491..70acfa6fde6 100644 --- a/js/actions/appActions.js +++ b/js/actions/appActions.js @@ -83,6 +83,17 @@ const appActions = { }) }, + /** + * The tab strip is empty + * @param {Number} windowId + */ + tabStripEmpty: function (windowId) { + dispatch({ + actionType: appConstants.APP_TAB_STRIP_EMPTY, + windowId + }) + }, + /** * A new tab has been created * @param {Object} tabValue @@ -94,6 +105,39 @@ const appActions = { }) }, + /** + * Tab moved event fired from muon + * @param {Object} tabValue + */ + tabMoved: function (tabId) { + dispatch({ + actionType: appConstants.APP_TAB_MOVED, + tabId + }) + }, + + /** + * A tab has been attached + * @param {Object} tabValue + */ + tabAttached: function (tabId) { + dispatch({ + actionType: appConstants.APP_TAB_ATTACHED, + tabId + }) + }, + + /** + * A tab will be attached + * @param {Object} tabValue + */ + tabWillAttach: function (tabId) { + dispatch({ + actionType: appConstants.APP_TAB_WILL_ATTACH, + tabId + }) + }, + /** * A tab has been moved to another window * @param {Number} tabId @@ -101,9 +145,9 @@ const appActions = { * @param {Object} browserOpts * @param {Number} windowId */ - tabMoved: function (tabId, frameOpts, browserOpts, windowId) { + tabDetachMenuItemClicked: function (tabId, frameOpts, browserOpts, windowId) { dispatch({ - actionType: appConstants.APP_TAB_MOVED, + actionType: appConstants.APP_TAB_DETACH_MENU_ITEM_CLICKED, tabId, frameOpts, browserOpts, diff --git a/js/actions/windowActions.js b/js/actions/windowActions.js index c4168474368..dbf7e8802b2 100644 --- a/js/actions/windowActions.js +++ b/js/actions/windowActions.js @@ -206,13 +206,6 @@ const windowActions = { }) }, - activeFrameChanged: function (frameProps) { - dispatch({ - actionType: windowConstants.WINDOW_ACTIVE_FRAME_CHANGED, - frameProps: frameProps - }) - }, - /** * Dispatches a message to the store when the frame is active and the window is focused * diff --git a/js/constants/appConstants.js b/js/constants/appConstants.js index 75549b9bd44..68c7beb12ea 100644 --- a/js/constants/appConstants.js +++ b/js/constants/appConstants.js @@ -11,6 +11,7 @@ const appConstants = { APP_WINDOW_CLOSED: _, APP_WINDOW_CREATED: _, APP_WINDOW_UPDATED: _, + APP_TAB_STRIP_EMPTY: _, APP_TAB_CLOSED: _, APP_TAB_UPDATED: _, APP_ADD_SITE: _, @@ -58,6 +59,9 @@ const appConstants = { APP_FRAME_CHANGED: _, APP_TAB_CREATED: _, APP_TAB_MOVED: _, + APP_TAB_DETACH_MENU_ITEM_CLICKED: _, + APP_TAB_ATTACHED: _, + APP_TAB_WILL_ATTACH: _, APP_TAB_ACTIVATE_REQUESTED: _, APP_TAB_INDEX_CHANGED: _, APP_TAB_CLOSE_REQUESTED: _, diff --git a/js/contextMenus.js b/js/contextMenus.js index 2bca66486c9..3d8c51dd311 100644 --- a/js/contextMenus.js +++ b/js/contextMenus.js @@ -547,7 +547,7 @@ function tabTemplateInit (frameProps) { label: locale.translation('detach'), click: (item) => { const browserOpts = { positionByMouseCursor: true } - appActions.tabMoved(tabId, frameProps.toJS(), browserOpts, -1) + appActions.tabDetachMenuItemClicked(tabId, frameProps.toJS(), browserOpts, -1) } }) } diff --git a/js/state/frameStateUtil.js b/js/state/frameStateUtil.js index 33f793ff44a..4bea7eee8a5 100644 --- a/js/state/frameStateUtil.js +++ b/js/state/frameStateUtil.js @@ -11,7 +11,6 @@ const settings = require('../constants/settings') // Actions const windowActions = require('../actions/windowActions') const webviewActions = require('../actions/webviewActions') -const appActions = require('../actions/appActions') // State const {makeImmutable} = require('../../app/common/state/immutableUtil') @@ -313,7 +312,7 @@ const frameOptsFromFrame = (frame) => { * Adds a frame specified by frameOpts and newKey and sets the activeFrameKey * @return Immutable top level application state ready to merge back in */ -function addFrame (state, frameOpts, newKey, partitionNumber, openInForeground, insertionIndex) { +function addFrame (state, frameOpts, newKey, partitionNumber, openInForeground, insertionIndex, active) { const frames = state.get('frames') const location = frameOpts.location // page url @@ -509,18 +508,30 @@ const deleteFrameInternalIndex = (state, frame) => { const updateFramesInternalIndex = (state, fromIndex) => { let framesInternal = state.get('framesInternal') || Immutable.Map() - state.get('frames').slice(fromIndex).forEach((frame, idx) => { + state.get('frames').slice(fromIndex).reduceRight((result, frame, idx) => { + const tabId = frame.get('tabId') + const frameKey = frame.get('key') const realIndex = idx + fromIndex - if (frame.get('key')) { - framesInternal = framesInternal.setIn(['index', frame.get('key').toString()], realIndex) + if (frameKey) { + framesInternal = framesInternal.setIn(['index', frameKey.toString()], realIndex) } - if (frame.get('tabId') !== -1) { - framesInternal = framesInternal.setIn(['tabIndex', frame.get('tabId').toString()], realIndex) + if (tabId !== -1) { + framesInternal = framesInternal.setIn(['tabIndex', tabId.toString()], realIndex) } + }, 0) + return state.set('framesInternal', framesInternal) +} - appActions.tabIndexChanged(frame.get('tabId'), realIndex) - }) - +const moveFrame = (state, tabId, index) => { + let framesInternal = state.get('framesInternal') || Immutable.Map() + const frame = getFrameByTabId(state, tabId) + const frameKey = frame.get('key') + if (frameKey) { + framesInternal = framesInternal.setIn(['index', frameKey.toString()], index) + } + if (tabId !== -1) { + framesInternal = framesInternal.setIn(['tabIndex', tabId.toString()], index) + } return state.set('framesInternal', framesInternal) } @@ -691,6 +702,7 @@ module.exports = { deleteTabInternalIndex, deleteFrameInternalIndex, updateFramesInternalIndex, + moveFrame, query, find, isAncestorFrameKey, diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js index 6b75240dda7..4624f42071b 100644 --- a/js/stores/windowStore.js +++ b/js/stores/windowStore.js @@ -71,6 +71,10 @@ class WindowStore extends EventEmitter { return frameStateUtil.getFrameByKey(windowState, key) } + getFrameByTabId (tabId) { + return frameStateUtil.getFrameByTabId(windowState, tabId) + } + emitChanges () { if (lastEmittedState !== windowState) { lastEmittedState = windowState @@ -104,7 +108,7 @@ const newFrame = (state, frameOpts) => { // handle tabs.create properties let insertionIndex = frameOpts.index !== undefined ? frameOpts.index - : undefined + : 0 if (frameOpts.partition) { frameOpts.isPrivate = frameStateUtil.isPrivatePartition(frameOpts.partition) @@ -116,7 +120,6 @@ const newFrame = (state, frameOpts) => { const active = frameOpts.active delete frameOpts.active - delete frameOpts.openInForeground // clean up any legacy openInForeground props let openInForeground = active if (openInForeground == null && frameOpts.disposition) { @@ -143,45 +146,11 @@ const newFrame = (state, frameOpts) => { frameOpts.location = '' } } - - // TODO: longer term get rid of parentFrameKey completely instead of - // calculating it here. - let parentFrameKey = frameOpts.parentFrameKey - if (frameOpts.openerTabId) { - parentFrameKey = frameStateUtil.getFrameKeyByTabId(state, frameOpts.openerTabId) - } - - // Find the closest index to the current frame's index which has - // a different ancestor frame key. - const frames = frameStateUtil.getFrames(state) - if (insertionIndex === undefined) { - insertionIndex = frameStateUtil.getFrameIndex(state, frameOpts.indexByFrameKey || parentFrameKey) - if (frameOpts.prependIndexByFrameKey === false) { - insertionIndex++ - } - if (insertionIndex === -1) { - insertionIndex = frames.size - // frameOpts.indexByFrameKey is used when the insertionIndex should be used exactly - } else if (!frameOpts.indexByFrameKey) { - while (insertionIndex < frames.size) { - ++insertionIndex - if (!frameStateUtil.isAncestorFrameKey(state, frames.get(insertionIndex), parentFrameKey)) { - break - } - } - } - } - if (frameStateUtil.isFrameKeyPinned(state, parentFrameKey)) { - insertionIndex = 0 - } - const nextKey = incrementNextKey() - state = state.merge( frameStateUtil.addFrame( state, frameOpts, nextKey, frameOpts.partitionNumber, openInForeground, insertionIndex)) - state = frameStateUtil.updateFramesInternalIndex(state, insertionIndex) if (openInForeground) { @@ -212,7 +181,8 @@ const frameTabIdChanged = (state, action) => { const index = frameStateUtil.getFrameIndex(state, action.getIn(['frameProps', 'key'])) state = state.mergeIn(['frames', index], newFrameProps) state = frameStateUtil.deleteTabInternalIndex(state, oldTabId) - return frameStateUtil.updateFramesInternalIndex(state, index) + state = frameStateUtil.updateFramesInternalIndex(state, index) + return state } const frameGuestInstanceIdChanged = (state, action) => { @@ -418,7 +388,9 @@ const doAction = (action) => { windowState = windowState.set('frames', frames) // Since the tab could have changed pages, update the tab page as well windowState = frameStateUtil.updateFramesInternalIndex(windowState, Math.min(sourceFrameIndex, newIndex)) + windowState = frameStateUtil.moveFrame(windowState, sourceFrameProps.get('tabId'), newIndex) windowState = frameStateUtil.updateTabPageIndex(windowState, activeFrame.get('tabId')) + appActions.tabIndexChanged(activeFrame.get('tabId'), newIndex) break } case windowConstants.WINDOW_SET_LINK_HOVER_PREVIEW: diff --git a/test/lib/brave.js b/test/lib/brave.js index bd02552ad73..2cda31b0a69 100644 --- a/test/lib/brave.js +++ b/test/lib/brave.js @@ -613,7 +613,7 @@ var exports = { const frame = val.value.frames[index] return this.execute(function (tabId, windowId, frame) { const browserOpts = { positionByMouseCursor: true } - devTools('appActions').tabMoved(tabId, frame, browserOpts, windowId) + devTools('appActions').tabDetachMenuItemClicked(tabId, frame, browserOpts, windowId) }, frame.tabId, windowId, frame) }) }) diff --git a/test/unit/app/browser/reducers/tabsReducerTest.js b/test/unit/app/browser/reducers/tabsReducerTest.js index 3daf8c7444b..2f0cd1fbac9 100644 --- a/test/unit/app/browser/reducers/tabsReducerTest.js +++ b/test/unit/app/browser/reducers/tabsReducerTest.js @@ -63,17 +63,6 @@ describe('tabsReducer unit tests', function () { 4: 3, 5: 4 }, - displayIndex: { - 1: { - 0: 1, - 1: 2 - }, - 2: { - 2: 3, - 3: 4, - 4: 5 - } - }, lastActive: { 1: [0, 1], 2: [4, 3, 2] @@ -101,6 +90,7 @@ describe('tabsReducer unit tests', function () { setActive: sinon.spy(), moveTo: sinon.mock(), reload: sinon.mock(), + updateTabsStateForWindow: sinon.mock(), create: sinon.mock() } @@ -113,13 +103,16 @@ describe('tabsReducer unit tests', function () { mockery.registerMock('../tabs', this.tabsAPI) mockery.registerMock('../windows', this.windowsAPI) mockery.registerMock('../../common/state/tabState', this.tabStateAPI) - mockery.registerMock('../../../js/settings', { getSetting: (settingKey, settingsCollection, value) => { + mockery.registerMock('../../js/settings', { getSetting: (settingKey, settingsCollection, value) => { if (settingKey === settings.TAB_CLOSE_ACTION) { return tabCloseSetting } return false }}) tabsReducer = require('../../../../../app/browser/reducers/tabsReducer') + + this.realTabsAPI = require('../../../../../app/browser/tabs') + this.tabsAPI.getNextActiveTab = this.realTabsAPI.getNextActiveTab }) after(function () { @@ -224,8 +217,14 @@ describe('tabsReducer unit tests', function () { }) }) + describe.skip('APP_TAB_DETACH_MENU_ITEM_CLICKED', function () { + it('Someone clicked the detach menu item', function () { + // TODO + }) + }) + describe.skip('APP_TAB_MOVED', function () { - it('moves a tab', function () { + it('A tab has moved', function () { // TODO }) }) @@ -273,11 +272,13 @@ describe('tabsReducer unit tests', function () { afterEach(function () { this.removeTabByTabIdSpy.restore() + this.tabsAPI.updateTabsStateForWindow.reset() }) it('calls tabState.removeTabByTabId', function () { tabsReducer(this.state, action) assert(this.tabStateAPI.removeTabByTabId.withArgs(this.state, action.tabId).calledOnce) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) it('does nothing if tabId is TAB_ID_NONE', function () { @@ -302,6 +303,7 @@ describe('tabsReducer unit tests', function () { tabsReducer(this.state, action) this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(3).calledOnce) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) }) @@ -325,6 +327,7 @@ describe('tabsReducer unit tests', function () { tabsReducer(testState, pickNextAction) this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(4).calledOnce) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) it('chooses next unpinned tab', function () { @@ -339,6 +342,7 @@ describe('tabsReducer unit tests', function () { tabsReducer(testState, pickNextAction) this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(5).calledOnce) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) it('chooses previous unpinned tab', function () { @@ -349,6 +353,7 @@ describe('tabsReducer unit tests', function () { tabsReducer(testState, action) this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(3).calledOnce) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) describe('if no unpinned tabs come after this', function () { @@ -365,6 +370,7 @@ describe('tabsReducer unit tests', function () { tabsReducer(testState, pickNextAction) this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(4).calledOnce) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) }) }) @@ -374,6 +380,7 @@ describe('tabsReducer unit tests', function () { tabsReducer(this.state, action) this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(4).calledOnce) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) it('chooses parent tab id (even if parent tab was NOT last active)', function () { @@ -383,6 +390,7 @@ describe('tabsReducer unit tests', function () { tabsReducer(testState, action) this.clock.tick(1510) assert(this.tabsAPI.setActive.withArgs(3).calledOnce) + assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) }) }) }) @@ -578,13 +586,6 @@ describe('tabsReducer unit tests', function () { beforeEach(function () { this.pinnedTabs = sinon.stub(this.tabStateAPI, 'getPinnedTabsByWindowId', (state, windowId) => Immutable.fromJS([])) }) - - it('closes window when there are no tabs left', function () { - tabsReducer(this.state, action) - this.clock.tick(1510) - assert(this.tabsAPI.toggleDevTools.notCalled) - assert(this.windowsAPI.closeWindow.withArgs(this.state, 1).calledOnce) - }) }) }) }) @@ -707,12 +708,10 @@ describe('tabsReducer unit tests', function () { assert.equal(args[0], state) // State is passed in as first arg assert.equal(args[1], 1) // tabId is 1 for first tab // frameOpts being dragged is for the first tab - assert.deepEqual(args[2], { tabId: 1, + assert.deepEqual(args[2].toJS(), { tabId: 1, windowId: 1, pinned: false, - active: true, - indexByFrameKey: undefined, - prependIndexByFrameKey: undefined + active: true }) // Passes browser options for position by mouse cursor assert.deepEqual(args[3], { diff --git a/test/unit/app/browser/tabsTest.js b/test/unit/app/browser/tabsTest.js index 03d2eaa7aed..6397573f5d8 100644 --- a/test/unit/app/browser/tabsTest.js +++ b/test/unit/app/browser/tabsTest.js @@ -82,7 +82,10 @@ describe('tabs API unit tests', function () { tabValue: () => this.state.get('tabs').find((tab) => tab.get('tabId') === tabId), isDestroyed: () => false, - detach: (cb) => cb() + detach: (cb) => cb(), + once: (event, cb) => { + setImmediate(cb) + } } if (tabId === 1) { Object.assign(webContents, this.tabWithDevToolsClosed) diff --git a/test/unit/js/stores/windowStoreTest.js b/test/unit/js/stores/windowStoreTest.js index f161f63186d..f3b258e0ee7 100644 --- a/test/unit/js/stores/windowStoreTest.js +++ b/test/unit/js/stores/windowStoreTest.js @@ -136,7 +136,7 @@ describe('Window store unit tests', function () { describe('APP_NEW_WEB_CONTENTS_ADDED', function () { let windowState - let tabIndexChangedStub + let tabDetachMenuItemClickedStub const demoWindowState = { frames: [{ security: { @@ -183,7 +183,15 @@ describe('Window store unit tests', function () { loading: false, unloaded: true, key: 2 - }] + }], + framesInternal: { + index: { + 8: 0 + }, + tabIndex: { + 8: 0 + } + } } const demoAction = { actionType: appConstants.APP_NEW_WEB_CONTENTS_ADDED, @@ -194,7 +202,8 @@ describe('Window store unit tests', function () { location: 'about:blank', partition: 'persist:default', active: true, - guestInstanceId: 8, + guestInstanceId: 2, + tabId: 8, isPinned: false, openerTabId: 8, disposition: 'foreground-tab', @@ -205,14 +214,14 @@ describe('Window store unit tests', function () { width: 300, active: true, height: 300, - guestInstanceId: 8, + guestInstanceId: 2, autoDiscardable: true, partition: 'persist:default', windowId: -1, incognito: false, canGoForward: false, url: '', - tabId: 33, + tabId: 8, index: -1, status: 'complete', highlighted: false, @@ -223,7 +232,7 @@ describe('Window store unit tests', function () { muted: false, reason: 'user' }, - id: 33, + id: 8, selected: true, discarded: false, canGoBack: false @@ -231,7 +240,7 @@ describe('Window store unit tests', function () { } beforeEach(function () { - tabIndexChangedStub = sinon.stub(appActions, 'tabIndexChanged') + tabDetachMenuItemClickedStub = sinon.stub(appActions, 'tabDetachMenuItemClicked') }) afterEach(function () { @@ -239,7 +248,7 @@ describe('Window store unit tests', function () { mockery.deregisterMock(reducer) }) - tabIndexChangedStub.restore() + tabDetachMenuItemClickedStub.restore() }) describe('when tab being opened is active', function () {