Skip to content

Commit

Permalink
Refactors window actions, remove the whole frame prop (related to tabs)
Browse files Browse the repository at this point in the history
Resolves brave#8857

Auditors: @bsclifton @bridiver

Test Plan:
- try closing a tab
- hover over the tab and close button should be displayed
- resize window and check if tabs are changing (when really small, text should be hidden)
- reorder tabs with dnd
- try tabs tear off
- try moving tabs from one window to another
- try moving tab into next tab page, by dropping it on the tab page indicator
- try moving bookmark into bookmark folder
- try reordering bookmarks
- check if tab preview is working
  • Loading branch information
NejcZdovc committed May 12, 2017
1 parent 7ac9acf commit 571f92f
Show file tree
Hide file tree
Showing 19 changed files with 197 additions and 170 deletions.
14 changes: 10 additions & 4 deletions app/browser/reducers/sitesReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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:
Expand Down
5 changes: 4 additions & 1 deletion app/renderer/components/bookmarks/bookmarksToolbar.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
12 changes: 5 additions & 7 deletions app/renderer/components/tabs/pinnedTabs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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)
}
}
Expand Down
8 changes: 4 additions & 4 deletions app/renderer/components/tabs/tab.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
})
}

Expand Down
2 changes: 1 addition & 1 deletion app/renderer/components/tabs/tabPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
19 changes: 8 additions & 11 deletions app/renderer/components/tabs/tabs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')

Expand Down Expand Up @@ -75,29 +72,29 @@ 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(() => {
const key = sourceDragData.get('key')
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)
}
}
}, 0)
return
}

if (e.dataTransfer.files) {
Array.from(e.dataTransfer.files).forEach((file) => {
const path = encodeURI(file.path)
Expand Down
6 changes: 3 additions & 3 deletions app/renderer/reducers/frameReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
6 changes: 3 additions & 3 deletions docs/appActions.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
24 changes: 11 additions & 13 deletions docs/windowActions.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.&lt;Object&gt;`, 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



Expand Down Expand Up @@ -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.



Expand All @@ -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

Expand All @@ -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

Expand Down
6 changes: 3 additions & 3 deletions js/about/aboutActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
Expand Down
14 changes: 10 additions & 4 deletions js/about/bookmarks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
10 changes: 5 additions & 5 deletions js/actions/appActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
Expand Down
Loading

0 comments on commit 571f92f

Please sign in to comment.