diff --git a/app/browser/reducers/sitesReducer.js b/app/browser/reducers/sitesReducer.js index 54149a55032..7a5bb8a20d9 100644 --- a/app/browser/reducers/sitesReducer.js +++ b/app/browser/reducers/sitesReducer.js @@ -40,8 +40,13 @@ const sitesReducer = (state, action, immutableAction) => { state = state.set('sites', siteUtil.addSite(sites, action.siteDetail, action.tag, action.originalSiteDetail, action.skipSync)) } if (action.destinationDetail) { - state = state.set('sites', siteUtil.moveSite(state.get('sites'), - action.siteDetail, action.destinationDetail, false, false, true)) + const sourceKey = siteUtil.getSiteKey(action.siteDetail) + const destinationKey = siteUtil.getSiteKey(action.destinationDetail) + + if (sourceKey != null) { + state = state.set('sites', siteUtil.moveSite(state.get('sites'), + sourceKey, destinationKey, false, false, true)) + } } if (syncEnabled()) { state = syncUtil.updateSiteCache(state, action.destinationDetail || action.siteDetail) @@ -56,10 +61,11 @@ const sitesReducer = (state, action, immutableAction) => { break case appConstants.APP_MOVE_SITE: state = state.set('sites', siteUtil.moveSite(state.get('sites'), - action.sourceDetail, action.destinationDetail, action.prepend, + action.sourceKey, action.destinationKey, action.prepend, action.destinationIsParent, false)) if (syncEnabled()) { - state = syncUtil.updateSiteCache(state, action.destinationDetail) + const destinationDetail = state.getIn(['sites', action.destinationKey]) + state = syncUtil.updateSiteCache(state, destinationDetail) } break case appConstants.APP_APPLY_SITE_RECORDS: diff --git a/app/renderer/components/bookmarks/bookmarksToolbar.js b/app/renderer/components/bookmarks/bookmarksToolbar.js index cdcca5e122c..2a271c1d469 100644 --- a/app/renderer/components/bookmarks/bookmarksToolbar.js +++ b/app/renderer/components/bookmarks/bookmarksToolbar.js @@ -64,7 +64,10 @@ class BookmarksToolbar extends ImmutableComponent { if (droppedOn.selectedRef) { const isLeftSide = dnd.isLeftSide(ReactDOM.findDOMNode(droppedOn.selectedRef), e.clientX) const droppedOnSiteDetail = droppedOn.selectedRef.props.bookmark || droppedOn.selectedRef.props.bookmarkFolder - appActions.moveSite(bookmark, droppedOnSiteDetail, isLeftSide, droppedOnSiteDetail.get('tags').includes(siteTags.BOOKMARK_FOLDER) && droppedOn && droppedOn.isDroppedOn) + const isDestinationParent = droppedOnSiteDetail.get('tags').includes(siteTags.BOOKMARK_FOLDER) && droppedOn && droppedOn.isDroppedOn + const bookmarkSiteKey = siteUtil.getSiteKey(bookmark) + const droppedOnSiteKey = siteUtil.getSiteKey(droppedOnSiteDetail) + appActions.moveSite(bookmarkSiteKey, droppedOnSiteKey, isLeftSide, isDestinationParent) } return } diff --git a/app/renderer/components/tabs/pinnedTabs.js b/app/renderer/components/tabs/pinnedTabs.js index cdc4d8e019e..8933bda5124 100644 --- a/app/renderer/components/tabs/pinnedTabs.js +++ b/app/renderer/components/tabs/pinnedTabs.js @@ -13,9 +13,6 @@ const Tab = require('./tab') const appActions = require('../../../../js/actions/appActions') const windowActions = require('../../../../js/actions/windowActions') -// Store -const windowStore = require('../../../../js/stores/windowStore') - // Constants const siteTags = require('../../../../js/constants/siteTags') const dragTypes = require('../../../../js/constants/dragTypes') @@ -48,13 +45,14 @@ class PinnedTabs extends ImmutableComponent { let droppedOnTab = dnd.closestFromXOffset(this.tabRefs.filter((node) => node && node.props.frame.get('key') !== key), clientX).selectedRef if (droppedOnTab) { const isLeftSide = dnd.isLeftSide(ReactDOM.findDOMNode(droppedOnTab), clientX) - const droppedOnFrameProps = windowStore.getFrame(droppedOnTab.props.frame.get('key')) - windowActions.moveTab(sourceDragData, droppedOnFrameProps, isLeftSide) + windowActions.moveTab(key, droppedOnTab.props.frame.get('key'), isLeftSide) if (!sourceDragData.get('pinnedLocation')) { appActions.tabPinned(sourceDragData.get('tabId'), true) } else { - appActions.moveSite(siteUtil.getDetailFromFrame(sourceDragData, siteTags.PINNED), - siteUtil.getDetailFromFrame(droppedOnFrameProps, siteTags.PINNED), + const sourceDetails = siteUtil.getDetailFromFrame(sourceDragData, siteTags.PINNED) + const destinationDetails = siteUtil.getDetailFromFrame(droppedOnTab.props.frame, siteTags.PINNED) + appActions.moveSite(siteUtil.getSiteKey(sourceDetails), + siteUtil.getSiteKey(destinationDetails), isLeftSide) } } diff --git a/app/renderer/components/tabs/tab.js b/app/renderer/components/tabs/tab.js index c1b3ac1874d..c8ac18f859e 100644 --- a/app/renderer/components/tabs/tab.js +++ b/app/renderer/components/tabs/tab.js @@ -168,7 +168,7 @@ class Tab extends ImmutableComponent { window.clearTimeout(this.hoverTimeout) windowActions.setPreviewFrame(null) } - windowActions.setTabHoverState(this.frame, false) + windowActions.setTabHoverState(this.props.frame.get('key'), false) } onMouseEnter (e) { @@ -181,9 +181,9 @@ class Tab extends ImmutableComponent { // as reported here: https://github.com/brave/browser-laptop/issues/1434 if (this.props.previewTabs) { this.hoverTimeout = - window.setTimeout(windowActions.setPreviewFrame.bind(null, this.frame), previewMode ? 0 : 200) + window.setTimeout(windowActions.setPreviewFrame.bind(null, this.props.frame.get('key')), previewMode ? 0 : 200) } - windowActions.setTabHoverState(this.frame, true) + windowActions.setTabHoverState(this.props.frame.get('key'), true) } onAuxClick (e) { @@ -233,7 +233,7 @@ class Tab extends ImmutableComponent { setImmediate(() => { const currentSize = getTabBreakpoint(this.tabSize) // Avoid updating breakpoint when user enters fullscreen (see #7301) - !this.props.hasTabInFullScreen && windowActions.setTabBreakpoint(this.frame, currentSize) + !this.props.hasTabInFullScreen && windowActions.setTabBreakpoint(this.props.frame.get('key'), currentSize) }) } diff --git a/app/renderer/components/tabs/tabPage.js b/app/renderer/components/tabs/tabPage.js index a0c0963fc7b..3ead0076179 100644 --- a/app/renderer/components/tabs/tabPage.js +++ b/app/renderer/components/tabs/tabPage.js @@ -58,7 +58,7 @@ class TabPage extends ImmutableComponent { setTimeout(() => { // If we're moving to a right page, then we're already shifting everything to the left by one, so we want // to drop it on the right. - windowActions.moveTab(sourceDragData, moveToFrame, + windowActions.moveTab(sourceDragData.get('key'), moveToFrame.get('key'), // Has -1 value for pinned tabs sourceDragFromPageIndex === -1 || sourceDragFromPageIndex >= this.props.index) diff --git a/app/renderer/components/tabs/tabs.js b/app/renderer/components/tabs/tabs.js index 4c47e35bb8a..7425be6bc1f 100644 --- a/app/renderer/components/tabs/tabs.js +++ b/app/renderer/components/tabs/tabs.js @@ -14,9 +14,6 @@ const Tab = require('./tab') const appActions = require('../../../../js/actions/appActions') const windowActions = require('../../../../js/actions/windowActions') -// Store -const windowStore = require('../../../../js/stores/windowStore') - // Constants const dragTypes = require('../../../../js/constants/dragTypes') @@ -75,6 +72,12 @@ class Tabs extends ImmutableComponent { const clientX = e.clientX const sourceDragData = dndData.getDragData(e.dataTransfer, dragTypes.TAB) if (sourceDragData) { + // If this is a different window ID than where the drag started, then + // the tear off will be done by tab.js + if (this.props.dragData.get('windowId') !== getCurrentWindowId()) { + return + } + // This must be executed async because the state change that this causes // will cause the onDragEnd to never run setTimeout(() => { @@ -82,15 +85,8 @@ class Tabs extends ImmutableComponent { let droppedOnTab = dnd.closestFromXOffset(this.tabRefs.filter((node) => node && node.props.frame.get('key') !== key), clientX).selectedRef if (droppedOnTab) { const isLeftSide = dnd.isLeftSide(ReactDOM.findDOMNode(droppedOnTab), clientX) - const droppedOnFrameProps = windowStore.getFrame(droppedOnTab.props.frame.get('key')) - // If this is a different window ID than where the drag started, then - // the tear off will be done by tab.js - if (this.props.dragData.get('windowId') !== getCurrentWindowId()) { - return - } - - windowActions.moveTab(sourceDragData, droppedOnFrameProps, isLeftSide) + windowActions.moveTab(key, droppedOnTab.props.frame.get('key'), isLeftSide) if (sourceDragData.get('pinnedLocation')) { appActions.tabPinned(sourceDragData.get('tabId'), false) } @@ -98,6 +94,7 @@ class Tabs extends ImmutableComponent { }, 0) return } + if (e.dataTransfer.files) { Array.from(e.dataTransfer.files).forEach((file) => { const path = encodeURI(file.path) diff --git a/app/renderer/reducers/frameReducer.js b/app/renderer/reducers/frameReducer.js index cbca8268fe6..23789485aa5 100644 --- a/app/renderer/reducers/frameReducer.js +++ b/app/renderer/reducers/frameReducer.js @@ -24,7 +24,7 @@ const setFullScreen = (state, action) => { const closeFrame = (state, action) => { // Use the frameProps we passed in, or default to the active frame - const frameProps = action.frameProps + const frameProps = frameStateUtil.getFrameByKey(state, action.frameKey) const index = frameStateUtil.getFramePropsIndex(state.get('frames'), frameProps) if (index === -1) { return state @@ -51,7 +51,7 @@ const closeFrame = (state, action) => { // Copy the hover state if tab closed with mouse as long as we have a next frame // This allow us to have closeTab button visible for sequential frames closing, until onMouseLeave event happens. if (hoverState && nextFrame) { - windowActions.setTabHoverState(nextFrame, hoverState) + windowActions.setTabHoverState(nextFrame.get('key'), hoverState) } return state @@ -75,7 +75,7 @@ const frameReducer = (state, action) => { }) break case windowConstants.WINDOW_CLOSE_FRAME: - if (!action.frameProps) { + if (action.frameKey < 0) { break } // Unless a caller explicitly specifies to close a pinned frame, then diff --git a/docs/appActions.md b/docs/appActions.md index 7e595bbeed0..9e2c2707382 100644 --- a/docs/appActions.md +++ b/docs/appActions.md @@ -192,15 +192,15 @@ Removes a site from the site list -### moveSite(sourceDetail, destinationDetail, prepend, destinationIsParent) +### moveSite(sourceKey, destinationKey, prepend, destinationIsParent) Dispatches a message to move a site locations. **Parameters** -**sourceDetail**: `string`, the location, partitionNumber, etc of the source moved site +**sourceKey**: `string`, the source key of the source moved site -**destinationDetail**: `string`, the location, partitionNumber, etc of the destination moved site +**destinationKey**: `string`, the destination key of the destination moved site **prepend**: `boolean`, Whether or not to prepend to the destinationLocation If false, the destinationDetail is considered a sibling. diff --git a/docs/windowActions.md b/docs/windowActions.md index 29e830957eb..73bbec44898 100644 --- a/docs/windowActions.md +++ b/docs/windowActions.md @@ -179,15 +179,13 @@ Dispatches a message to the store to indicate that the webview entered full scre -### closeFrame(frames, frameProps) +### closeFrame(frameKey) Dispatches a message to close a frame **Parameters** -**frames**: `Array.<Object>`, Immutable list of of all the frames - -**frameProps**: `Object`, The properties of the frame to close +**frameKey**: `Object`, Frame key of the frame to close @@ -224,14 +222,14 @@ Dispatches a message to the store when the frame is active and the window is foc -### setPreviewFrame(frameProps) +### setPreviewFrame(frameKey) Dispatches a message to the store to set a preview frame. This is done when hovering over a tab. **Parameters** -**frameProps**: `Object`, the frame properties for the webview in question. +**frameKey**: `Object`, the frame key for the webview in question. @@ -245,25 +243,25 @@ Dispatches a message to the store to set the tab page index. -### setTabBreakpoint(frameProps, breakpoint) +### setTabBreakpoint(frameKey, breakpoint) Dispatches a message to the store to set the tab breakpoint. **Parameters** -**frameProps**: `Object`, the frame properties for the webview in question. +**frameKey**: `Object`, the frame key for the webview in question. **breakpoint**: `string`, the tab breakpoint to change to -### setTabHoverState(frameProps, hoverState) +### setTabHoverState(frameKey, hoverState) Dispatches a message to the store to set the current tab hover state. **Parameters** -**frameProps**: `Object`, the frame properties for the webview in question. +**frameKey**: `Object`, the frame key for the webview in question. **hoverState**: `boolean`, whether or not mouse is over tab @@ -289,15 +287,15 @@ Dispatches a message to the store to set the tab page index. -### moveTab(sourceFrameProps, destinationFrameProps, prepend) +### moveTab(sourceFrameKey, destinationFrameKey, prepend) Dispatches a message to the store to indicate that the specified frame should move locations. **Parameters** -**sourceFrameProps**: `Object`, the frame properties for the webview to move. +**sourceFrameKey**: `Object`, the frame key for the webview to move. -**destinationFrameProps**: `Object`, the frame properties for the webview to move to. +**destinationFrameKey**: `Object`, the frame key for the webview to move to. **prepend**: `boolean`, Whether or not to prepend to the destinationFrameProps diff --git a/js/about/aboutActions.js b/js/about/aboutActions.js index b4f6aca9e64..67c7786fec4 100644 --- a/js/about/aboutActions.js +++ b/js/about/aboutActions.js @@ -188,11 +188,11 @@ const aboutActions = { ipc.sendToHost(messages.CONTEXT_MENU_OPENED, nodeProps, contextMenuType) }, - moveSite: function (sourceDetail, destinationDetail, prepend, destinationIsParent) { + moveSite: function (sourceKey, destinationKey, prepend, destinationIsParent) { aboutActions.dispatchAction({ actionType: appConstants.APP_MOVE_SITE, - sourceDetail, - destinationDetail, + sourceKey, + destinationKey, prepend, destinationIsParent }) diff --git a/js/about/bookmarks.js b/js/about/bookmarks.js index 8c09e8cde3b..e78105b6298 100644 --- a/js/about/bookmarks.js +++ b/js/about/bookmarks.js @@ -59,8 +59,11 @@ class BookmarkFolderItem extends React.Component { */ moveBookmark (e, bookmark) { if (siteUtil.isMoveAllowed(this.props.allBookmarkFolders, bookmark, this.props.bookmarkFolder)) { - aboutActions.moveSite(bookmark.toJS(), - this.props.bookmarkFolder.toJS(), + const bookmarkSiteKey = siteUtil.getSiteKey(bookmark.toJS()) + const bookmarkFolderSiteKey = siteUtil.getSiteKey(this.props.bookmarkFolder.toJS()) + + aboutActions.moveSite(bookmarkSiteKey, + bookmarkFolderSiteKey, dndData.shouldPrependVerticalItem(e.target, e.clientY), true) } @@ -296,8 +299,11 @@ class BookmarksList extends ImmutableComponent { } if (siteUtil.isMoveAllowed(this.props.allBookmarkFolders, bookmark, siteDetail)) { - aboutActions.moveSite(bookmark.toJS(), - siteDetail.toJS(), + const bookmarkSiteKey = siteUtil.getSiteKey(bookmark.toJS()) + const siteKey = siteUtil.getSiteKey(siteDetail.toJS()) + + aboutActions.moveSite(bookmarkSiteKey, + siteKey, dndData.shouldPrependVerticalItem(e.target, e.clientY), destinationIsParent) } diff --git a/js/actions/appActions.js b/js/actions/appActions.js index 6db5975f197..44ea4beb247 100644 --- a/js/actions/appActions.js +++ b/js/actions/appActions.js @@ -254,17 +254,17 @@ const appActions = { /** * Dispatches a message to move a site locations. * - * @param {string} sourceDetail - the location, partitionNumber, etc of the source moved site - * @param {string} destinationDetail - the location, partitionNumber, etc of the destination moved site + * @param {string} sourceKey - the source key of the source moved site + * @param {string} destinationKey - the destination key of the destination moved site * @param {boolean} prepend - Whether or not to prepend to the destinationLocation * If false, the destinationDetail is considered a sibling. * @param {boolean} destinationIsParent - Whether or not the destinationDetail should be considered the new parent. */ - moveSite: function (sourceDetail, destinationDetail, prepend, destinationIsParent) { + moveSite: function (sourceKey, destinationKey, prepend, destinationIsParent) { AppDispatcher.dispatch({ actionType: appConstants.APP_MOVE_SITE, - sourceDetail, - destinationDetail, + sourceKey, + destinationKey, prepend, destinationIsParent }) diff --git a/js/actions/windowActions.js b/js/actions/windowActions.js index d753a0bca42..3d1563ed6ae 100644 --- a/js/actions/windowActions.js +++ b/js/actions/windowActions.js @@ -210,13 +210,12 @@ const windowActions = { /** * Dispatches a message to close a frame * - * @param {Object[]} frames - Immutable list of of all the frames - * @param {Object} frameProps - The properties of the frame to close + * @param {Object} frameKey - Frame key of the frame to close */ - closeFrame: function (frameProps) { + closeFrame: function (frameKey) { dispatch({ actionType: windowConstants.WINDOW_CLOSE_FRAME, - frameProps + frameKey }) }, @@ -275,12 +274,12 @@ const windowActions = { * Dispatches a message to the store to set a preview frame. * This is done when hovering over a tab. * - * @param {Object} frameProps - the frame properties for the webview in question. + * @param {Object} frameKey - the frame key for the webview in question. */ - setPreviewFrame: function (frameProps) { + setPreviewFrame: function (frameKey) { dispatch({ actionType: windowConstants.WINDOW_SET_PREVIEW_FRAME, - frameProps: frameProps + frameKey }) }, @@ -299,13 +298,13 @@ const windowActions = { /** * Dispatches a message to the store to set the tab breakpoint. * - * @param {Object} frameProps - the frame properties for the webview in question. + * @param {Object} frameKey - the frame key for the webview in question. * @param {string} breakpoint - the tab breakpoint to change to */ - setTabBreakpoint: function (frameProps, breakpoint) { + setTabBreakpoint: function (frameKey, breakpoint) { dispatch({ actionType: windowConstants.WINDOW_SET_TAB_BREAKPOINT, - frameProps, + frameKey, breakpoint }) }, @@ -313,13 +312,13 @@ const windowActions = { /** * Dispatches a message to the store to set the current tab hover state. * - * @param {Object} frameProps - the frame properties for the webview in question. + * @param {Object} frameKey - the frame key for the webview in question. * @param {boolean} hoverState - whether or not mouse is over tab */ - setTabHoverState: function (frameProps, hoverState) { + setTabHoverState: function (frameKey, hoverState) { dispatch({ actionType: windowConstants.WINDOW_SET_TAB_HOVER_STATE, - frameProps, + frameKey, hoverState }) }, @@ -351,15 +350,15 @@ const windowActions = { /** * Dispatches a message to the store to indicate that the specified frame should move locations. * - * @param {Object} sourceFrameProps - the frame properties for the webview to move. - * @param {Object} destinationFrameProps - the frame properties for the webview to move to. + * @param {Object} sourceFrameKey - the frame key for the webview to move. + * @param {Object} destinationFrameKey - the frame key for the webview to move to. * @param {boolean} prepend - Whether or not to prepend to the destinationFrameProps */ - moveTab: function (sourceFrameProps, destinationFrameProps, prepend) { + moveTab: function (sourceFrameKey, destinationFrameKey, prepend) { dispatch({ actionType: windowConstants.WINDOW_TAB_MOVE, - sourceFrameProps, - destinationFrameProps, + sourceFrameKey, + destinationFrameKey, prepend }) }, diff --git a/js/components/frame.js b/js/components/frame.js index 0835e4e7e4b..a4452f2fd93 100644 --- a/js/components/frame.js +++ b/js/components/frame.js @@ -91,7 +91,7 @@ class Frame extends React.Component { } onCloseFrame () { - windowActions.closeFrame(this.frame) + windowActions.closeFrame(this.props.frameKey) } getFrameBraverySettings (props) { diff --git a/js/contextMenus.js b/js/contextMenus.js index eeb5698b1d8..554c4138012 100644 --- a/js/contextMenus.js +++ b/js/contextMenus.js @@ -373,7 +373,9 @@ function showBookmarkFolderInit (allBookmarkItems, parentBookmarkFolder, activeF e.preventDefault() const bookmark = dnd.prepareBookmarkDataFromCompatible(e.dataTransfer) if (bookmark) { - appActions.moveSite(bookmark, parentBookmarkFolder, false, true) + const bookmarkSiteKey = siteUtil.getSiteKey(bookmark) + const parentBookmarkFolderKey = siteUtil.getSiteKey(parentBookmarkFolder) + appActions.moveSite(bookmarkSiteKey, parentBookmarkFolderKey, false, true) } } }] @@ -414,7 +416,10 @@ function bookmarkItemsInit (allBookmarkItems, items, activeFrame) { e.preventDefault() const bookmarkItem = dnd.prepareBookmarkDataFromCompatible(e.dataTransfer) if (bookmarkItem) { - appActions.moveSite(bookmarkItem, site, dndData.shouldPrependVerticalItem(e.target, e.clientY)) + const bookmarkItemSiteKey = siteUtil.getSiteKey(bookmarkItem) + const siteKey = siteUtil.getSiteKey(site) + + appActions.moveSite(bookmarkItemSiteKey, siteKey, dndData.shouldPrependVerticalItem(e.target, e.clientY)) } }, click: function (e) { diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js index febea1e1457..32c0ab6f901 100644 --- a/js/state/siteUtil.js +++ b/js/state/siteUtil.js @@ -366,7 +366,7 @@ const getAncestorFolderIds = (parentFolderIds, bookmarkFolder, allBookmarks) => * Determine if a proposed move is valid * * @param sites The application state's Immutable sites list - * @param siteDetail The site detail to move + * @param sourceDetail The site detail to move * @param destinationDetail The site detail to move to */ module.exports.isMoveAllowed = (sites, sourceDetail, destinationDetail) => { @@ -389,41 +389,41 @@ module.exports.isMoveAllowed = (sites, sourceDetail, destinationDetail) => { * Moves the specified site from one location to another * * @param sites The application state's Immutable sites map - * @param siteDetail The site detail to move - * @param destinationDetail The site detail to move to + * @param sourceKey The site key to move + * @param destinationKey The site key to move to * @param prepend Whether the destination detail should be prepended or not, not used if destinationIsParent is true * @param destinationIsParent Whether the item should be moved inside of the destinationDetail. * @param disallowReparent If set to true, parent folder will not be set * @return The new sites Immutable object */ -module.exports.moveSite = function (sites, sourceDetail, destinationDetail, prepend, +module.exports.moveSite = function (sites, sourceKey, destinationKey, prepend, destinationIsParent, disallowReparent) { - if (!module.exports.isMoveAllowed(sites, sourceDetail, destinationDetail)) { + let sourceSite = sites.get(sourceKey) || Immutable.Map() + const destinationSite = sites.get(destinationKey) || Immutable.Map() + + if (sourceSite.isEmpty()) { return sites } - const sourceKey = module.exports.getSiteKey(sourceDetail) - const destinationKey = module.exports.getSiteKey(destinationDetail) + if (!module.exports.isMoveAllowed(sites, sourceSite, destinationSite)) { + return sites + } - const sourceSiteIndex = sites.getIn([sourceKey, 'order']) + const sourceSiteIndex = sourceSite.get('order') let destinationSiteIndex if (destinationIsParent) { // When the destination is the parent we want to put it at the end destinationSiteIndex = sites.size - 1 prepend = true } else { - destinationSiteIndex = sites.getIn([destinationKey, 'order']) + destinationSiteIndex = destinationSite.get('order') } - let sourceSite = sites.get(sourceKey) - if (!sourceSite) { - return sites - } let newIndex = destinationSiteIndex + (prepend ? 0 : 1) if (destinationSiteIndex > sourceSiteIndex) { --newIndex } - const destinationSite = sites.get(destinationKey) + sites = sites.delete(sourceKey) sites = sites.map((site) => { const siteOrder = site.get('order') @@ -437,9 +437,9 @@ module.exports.moveSite = function (sites, sourceDetail, destinationDetail, prep sourceSite = sourceSite.set('order', newIndex) if (!disallowReparent) { - if (destinationIsParent && destinationDetail.get('folderId') !== sourceSite.get('folderId')) { - sourceSite = sourceSite.set('parentFolderId', destinationDetail.get('folderId')) - } else if (!destinationSite.get('parentFolderId')) { + if (destinationIsParent && destinationSite.get('folderId') !== sourceSite.get('folderId')) { + sourceSite = sourceSite.set('parentFolderId', destinationSite.get('folderId')) + } else if (destinationSite.get('parentFolderId') == null) { sourceSite = sourceSite.set('parentFolderId', 0) } else if (destinationSite.get('parentFolderId') !== sourceSite.get('parentFolderId')) { sourceSite = sourceSite.set('parentFolderId', destinationSite.get('parentFolderId')) diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js index 7bf00fcc1f4..24521df0343 100644 --- a/js/stores/windowStore.js +++ b/js/stores/windowStore.js @@ -377,8 +377,8 @@ const doAction = (action) => { break case windowConstants.WINDOW_SET_PREVIEW_FRAME: windowState = windowState.merge({ - previewFrameKey: action.frameProps && action.frameProps.get('key') !== windowState.get('activeFrameKey') - ? action.frameProps.get('key') : null + previewFrameKey: action.frameKey != null && action.frameKey !== windowState.get('activeFrameKey') + ? action.frameKey : null }) break case windowConstants.WINDOW_SET_PREVIEW_TAB_PAGE_INDEX: @@ -397,23 +397,32 @@ const doAction = (action) => { } break case windowConstants.WINDOW_SET_TAB_BREAKPOINT: - windowState = windowState.setIn(['frames', frameStateUtil.getFramePropsIndex(frameStateUtil.getFrames(windowState), action.frameProps), 'breakpoint'], action.breakpoint) - break + { + const frameIndex = frameStateUtil.findIndexForFrameKey(frameStateUtil.getFrames(windowState), action.frameKey) + windowState = windowState.setIn(['frames', frameIndex, 'breakpoint'], action.breakpoint) + break + } case windowConstants.WINDOW_SET_TAB_HOVER_STATE: - windowState = windowState.setIn(['frames', frameStateUtil.getFramePropsIndex(frameStateUtil.getFrames(windowState), action.frameProps), 'hoverState'], action.hoverState) - break + { + const frameIndex = frameStateUtil.findIndexForFrameKey(frameStateUtil.getFrames(windowState), action.frameKey) + windowState = windowState.setIn(['frames', frameIndex, 'hoverState'], action.hoverState) + break + } case windowConstants.WINDOW_TAB_MOVE: - const sourceFramePropsIndex = frameStateUtil.getFramePropsIndex(frameStateUtil.getFrames(windowState), action.sourceFrameProps) - let newIndex = frameStateUtil.getFramePropsIndex(frameStateUtil.getFrames(windowState), action.destinationFrameProps) + (action.prepend ? 0 : 1) - let frames = frameStateUtil.getFrames(windowState).splice(sourceFramePropsIndex, 1) - if (newIndex > sourceFramePropsIndex) { - newIndex-- + { + const sourceFrameProps = frameStateUtil.getFrameByKey(windowState, action.sourceFrameKey) + const sourceFrameIndex = frameStateUtil.findIndexForFrameKey(frameStateUtil.getFrames(windowState), action.sourceFrameKey) + let newIndex = frameStateUtil.findIndexForFrameKey(frameStateUtil.getFrames(windowState), action.destinationFrameKey) + (action.prepend ? 0 : 1) + let frames = frameStateUtil.getFrames(windowState).splice(sourceFrameIndex, 1) + if (newIndex > sourceFrameIndex) { + newIndex-- + } + frames = frames.splice(newIndex, 0, sourceFrameProps) + windowState = windowState.set('frames', frames) + // Since the tab could have changed pages, update the tab page as well + windowState = updateTabPageIndex(windowState, frameStateUtil.getActiveFrame(windowState)) + break } - frames = frames.splice(newIndex, 0, action.sourceFrameProps) - windowState = windowState.set('frames', frames) - // Since the tab could have changed pages, update the tab page as well - windowState = updateTabPageIndex(windowState, frameStateUtil.getActiveFrame(windowState)) - break case windowConstants.WINDOW_SET_LINK_HOVER_PREVIEW: windowState = windowState.mergeIn(activeFrameStatePath(windowState), { hrefPreview: action.href, diff --git a/test/unit/app/browser/reducers/sitesReducerTest.js b/test/unit/app/browser/reducers/sitesReducerTest.js index bf597fe60dd..4212ae678b0 100644 --- a/test/unit/app/browser/reducers/sitesReducerTest.js +++ b/test/unit/app/browser/reducers/sitesReducerTest.js @@ -112,8 +112,9 @@ describe('sitesReducerTest', function () { describe('APP_MOVE_SITE', function () { it('Moves the specified site', function () { const url = 'https://www.brave.com' + const url2 = 'https://www.brave.com/3' let state = initState - let action = { + let addAction = { actionType: appConstants.APP_ADD_SITE, siteDetail: Immutable.fromJS({ location: url, @@ -121,29 +122,33 @@ describe('sitesReducerTest', function () { }), skipSync: true } - let newState = sitesReducer(state, action) - action.siteDetail = action.siteDetail.set('location', 'https://www.brave.com/2') - newState = sitesReducer(newState, action) - action.siteDetail = action.siteDetail.set('location', 'https://www.brave.com/3') - newState = sitesReducer(newState, action) + + let moveAction = { + actionType: appConstants.APP_MOVE_SITE, + sourceKey: `${url}|0|0`, + destinationKey: `${url2}|0|0` + } + + // Add sites + let newState = sitesReducer(state, addAction) + addAction.siteDetail = addAction.siteDetail.set('location', 'https://www.brave.com/2') + newState = sitesReducer(newState, addAction) + addAction.siteDetail = addAction.siteDetail.set('location', 'https://www.brave.com/3') + newState = sitesReducer(newState, addAction) assert.equal(Object.keys(newState.get('sites').toJS()).length, 3) // Move the site to the 2nd position - action.actionType = appConstants.APP_MOVE_SITE - action.sourceDetail = action.siteDetail - action.destinationDetail = action.siteDetail.set('location', 'https://www.brave.com') - action.siteDetail = undefined - newState = sitesReducer(newState, action).toJS() + newState = sitesReducer(newState, moveAction).toJS() assert.equal(Object.keys(newState.sites).length, 3) - assert.equal(Object.values(newState.sites)[2].location, 'https://www.brave.com/3') - assert.equal(Object.values(newState.sites)[2].order, 1) + assert.equal(Object.values(newState.sites)[2].location, url) + assert.equal(Object.values(newState.sites)[2].order, 2) // Move the site to the 3rd position - action.prepend = true - newState = sitesReducer(Immutable.fromJS(newState), action).toJS() + moveAction.prepend = true + newState = sitesReducer(Immutable.fromJS(newState), moveAction).toJS() assert.equal(Object.keys(newState.sites).length, 3) - assert.equal(Object.values(newState.sites)[2].location, 'https://www.brave.com/3') - assert.equal(Object.values(newState.sites)[2].order, 0) + assert.equal(Object.values(newState.sites)[2].location, url) + assert.equal(Object.values(newState.sites)[2].order, 1) }) }) }) diff --git a/test/unit/state/siteUtilTest.js b/test/unit/state/siteUtilTest.js index b9035f7b398..1e31ac6367a 100644 --- a/test/unit/state/siteUtilTest.js +++ b/test/unit/state/siteUtilTest.js @@ -779,20 +779,15 @@ describe('siteUtil', function () { describe('moveSite', function () { describe('order test', function () { describe('back to front', function () { - const destinationDetail = { - location: testUrl1, - partitionNumber: 0, - parentFolderId: 0, - order: 0 - } - const sourceDetail = { - location: testUrl2 + '4', - partitionNumber: 0, - parentFolderId: 0, - order: 3 - } + const destinationKey = 'https://brave.com/|0|0' + const sourceKey = 'http://example.com/4|0|0' const sites = { - 'https://brave.com/|0|0': destinationDetail, + 'https://brave.com/|0|0': { + location: testUrl1, + partitionNumber: 0, + parentFolderId: 0, + order: 0 + }, 'http://example.com/0|0': { location: testUrl2, partitionNumber: 0, @@ -805,7 +800,12 @@ describe('siteUtil', function () { parentFolderId: 0, order: 2 }, - 'http://example.com/4|0|0': sourceDetail + 'http://example.com/4|0|0': { + location: testUrl2 + '4', + partitionNumber: 0, + parentFolderId: 0, + order: 3 + } } it('prepend target', function () { @@ -836,8 +836,8 @@ describe('siteUtil', function () { } } const processedSites = siteUtil.moveSite(Immutable.fromJS(sites), - Immutable.fromJS(sourceDetail), - Immutable.fromJS(destinationDetail), true, false, true) + sourceKey, + destinationKey, true, false, true) assert.deepEqual(processedSites.toJS(), expectedSites) }) it('not prepend target', function () { @@ -868,26 +868,21 @@ describe('siteUtil', function () { } } const processedSites = siteUtil.moveSite(Immutable.fromJS(sites), - Immutable.fromJS(sourceDetail), - Immutable.fromJS(destinationDetail), false, false, true) + sourceKey, + destinationKey, false, false, true) assert.deepEqual(processedSites.toJS(), expectedSites) }) }) describe('front to back', function () { - const sourceDetail = { - location: testUrl1, - partitionNumber: 0, - parentFolderId: 0, - order: 0 - } - const destinationDetail = { - location: testUrl2 + '4', - partitionNumber: 0, - parentFolderId: 0, - order: 3 - } + const sourceKey = 'https://brave.com/|0|0' + const destinationKey = 'http://example.com/4|0|0' const sites = { - 'https://brave.com/|0|0': sourceDetail, + 'https://brave.com/|0|0': { + location: testUrl1, + partitionNumber: 0, + parentFolderId: 0, + order: 0 + }, 'http://example.com/0|0': { location: testUrl2, partitionNumber: 0, @@ -900,7 +895,12 @@ describe('siteUtil', function () { parentFolderId: 0, order: 2 }, - 'http://example.com/4|0|0': destinationDetail + 'http://example.com/4|0|0': { + location: testUrl2 + '4', + partitionNumber: 0, + parentFolderId: 0, + order: 3 + } } it('prepend target', function () { @@ -931,8 +931,8 @@ describe('siteUtil', function () { } } const processedSites = siteUtil.moveSite(Immutable.fromJS(sites), - Immutable.fromJS(sourceDetail), - Immutable.fromJS(destinationDetail), true, false, true) + sourceKey, + destinationKey, true, false, true) assert.deepEqual(processedSites.toJS(), expectedSites) }) it('not prepend target', function () { @@ -963,13 +963,14 @@ describe('siteUtil', function () { } } const processedSites = siteUtil.moveSite(Immutable.fromJS(sites), - Immutable.fromJS(sourceDetail), - Immutable.fromJS(destinationDetail), false, false, true) + sourceKey, + destinationKey, false, false, true) assert.deepEqual(processedSites.toJS(), expectedSites) }) }) }) it('destination is parent', function () { + const sourceKey = 'https://brave.com/|0|0' const sourceDetail = { location: testUrl1, partitionNumber: 0, @@ -996,8 +997,8 @@ describe('siteUtil', function () { } } const processedSites = siteUtil.moveSite(Immutable.fromJS(sites), - Immutable.fromJS(sourceDetail), - Immutable.fromJS(destinationDetail), false, true, false) + sourceKey, + '1', false, true, false) assert.deepEqual(processedSites.toJS(), expectedSites) }) it('reparent', function () { @@ -1027,8 +1028,8 @@ describe('siteUtil', function () { } } const processedSites = siteUtil.moveSite(Immutable.fromJS(sites), - Immutable.fromJS(sourceDetail), - Immutable.fromJS(destinationDetail), false, false, false) + 'https://brave.com/|0|1', + '1', false, false, false) assert.deepEqual(processedSites.toJS(), expectedSites) }) })