From 87c8b5b11ceb763ad892015efa4de5fd0cc5a7c5 Mon Sep 17 00:00:00 2001 From: petemill Date: Mon, 7 May 2018 12:35:55 -0700 Subject: [PATCH] Dragging to blank area of tab strip sets correct destination index Fix #14046 Avoid a tab being moved to a new window with a frame index higher than it should have. This occurred because when dropping on the blank area of a tab strip, there is no tab being dragged over. The last tab being dragged over could have been in another window, or could be the dragged tab itself, causing a frame to be created in the new window with an index that is too high, corrupting the "frame index <-> tabId" cache. Instead, we insert the tab at the beginning, and rely on muon to provide an updated tab index (if provided with an index of -1, muon will add the tab to the end of the strip). --- app/browser/reducers/tabsReducer.js | 14 ++++++++++---- app/browser/tabs.js | 8 ++++---- app/renderer/reducers/frameReducer.js | 7 ++++++- js/stores/windowStore.js | 8 +++++++- test/unit/app/browser/reducers/tabsReducerTest.js | 6 ++++-- 5 files changed, 31 insertions(+), 12 deletions(-) diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index 0089f96b09e..9fffa7698a3 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/app/browser/reducers/tabsReducer.js @@ -528,15 +528,21 @@ const tabsReducer = (state, action, immutableAction) => { if (dragData && dragData.get('type') === dragTypes.TAB) { const frame = dragData.get('data') let frameOpts = frameOptsFromFrame(frame) + const draggingTabId = frameOpts.get('tabId') const browserOpts = { positionByMouseCursor: true, checkMaximized: true } const tabIdForIndex = dragData.getIn(['dragOverData', 'draggingOverKey']) - const tabForIndex = tabState.getByTabId(state, tabIdForIndex) + const tabForIndex = tabIdForIndex !== draggingTabId && tabState.getByTabId(state, tabIdForIndex) const dropWindowId = dragData.get('dropWindowId') - if (dropWindowId != null && dropWindowId !== -1 && tabForIndex) { + let newIndex = -1 + // Set new index for new window if last dragged-over tab is in new window. + // Otherwise, could be over another tab's tab strip, but most recently dragged-over a tab in another window. + if (dropWindowId != null && dropWindowId !== -1 && tabForIndex && tabForIndex.get('windowId') === dropWindowId) { const prependIndexByTabId = dragData.getIn(['dragOverData', 'draggingOverLeftHalf']) - frameOpts = frameOpts.set('index', tabForIndex.get('index') + (prependIndexByTabId ? 0 : 1)) + newIndex = tabForIndex.get('index') + (prependIndexByTabId ? 0 : 1) } - tabs.moveTo(state, frame.get('tabId'), frameOpts, browserOpts, dragData.get('dropWindowId')) + // ensure the tab never moves window with its original index + frameOpts = frameOpts.set('index', newIndex) + tabs.moveTo(state, draggingTabId, frameOpts, browserOpts, dragData.get('dropWindowId')) } break } diff --git a/app/browser/tabs.js b/app/browser/tabs.js index d92c2d4cf86..95ff65f86ed 100644 --- a/app/browser/tabs.js +++ b/app/browser/tabs.js @@ -1069,12 +1069,11 @@ const api = { }, moveTo: (state, tabId, frameOpts, browserOpts, toWindowId) => { - - if (shouldDebugTabEvents) { - console.log(`Tab [${tabId}] tabs.moveTo(window: ${toWindowId})`) - } frameOpts = makeImmutable(frameOpts) browserOpts = makeImmutable(browserOpts) + if (shouldDebugTabEvents) { + console.log(`Tab [${tabId}] tabs.moveTo(window: ${toWindowId}, index: ${frameOpts.get('index')})`) + } const tab = webContentsCache.getWebContents(tabId) if (!tab || tab.isDestroyed()) { return @@ -1131,6 +1130,7 @@ const api = { // invalid index? add to end of tab strip by specifying -1 index if (toIndex == null || toIndex === -1) { toIndex = -1 + frameOpts = frameOpts.set('index', -1) } const win = getWindow(toWindowId) if (!win || win.isDestroyed()) { diff --git a/app/renderer/reducers/frameReducer.js b/app/renderer/reducers/frameReducer.js index 225d8f19728..e9317bfdf73 100644 --- a/app/renderer/reducers/frameReducer.js +++ b/app/renderer/reducers/frameReducer.js @@ -84,7 +84,12 @@ const frameReducer = (state, action, immutableAction) => { const tabId = immutableAction.get('tabId') const index = frameStateUtil.getIndexByTabId(state, tabId) if (index === -1) { - console.error('frame not found for tab inserted to tab strip', tabId, state.get('frames').toJS()) + console.error( + 'frame not found for tab inserted to tab strip', + tabId, + (state.get('frames') || Immutable.Map()).toJS(), + (state.get('framesInternal') || Immutable.Map()).toJS() + ) break } state = state.mergeIn(['frames', index], { diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js index d7315bfec68..6544cabe60a 100644 --- a/js/stores/windowStore.js +++ b/js/stores/windowStore.js @@ -105,10 +105,16 @@ const newFrame = (state, frameOpts) => { } frameOpts = frameOpts.toJS ? frameOpts.toJS() : frameOpts // handle tabs.create properties - let insertionIndex = frameOpts.index !== undefined + + // Ensure valid index + let insertionIndex = frameOpts.index != null ? frameOpts.index : 0 + if (insertionIndex === -1) { + frameOpts.index = insertionIndex = 0 + } + if (frameOpts.partition) { frameOpts.isPrivate = frameStateUtil.isPrivatePartition(frameOpts.partition) if (frameStateUtil.isSessionPartition(frameOpts.partition)) { diff --git a/test/unit/app/browser/reducers/tabsReducerTest.js b/test/unit/app/browser/reducers/tabsReducerTest.js index 7e95907b2b5..9d90adc56b0 100644 --- a/test/unit/app/browser/reducers/tabsReducerTest.js +++ b/test/unit/app/browser/reducers/tabsReducerTest.js @@ -596,10 +596,12 @@ 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].toJS(), { tabId: 1, + assert.deepEqual(args[2].toJS(), { + tabId: 1, windowId: 1, pinned: false, - active: true + active: true, + index: -1 // -1 specifies should go at end of tab strip }) // Passes browser options for position by mouse cursor assert.deepEqual(args[3], {