Skip to content

Commit

Permalink
Added close tab page
Browse files Browse the repository at this point in the history
Resolves brave#5489

Auditors: @bbondy @bsclifton

Test Plan:
- open two tab pages
- right click on one and click close tab page
- all tabs in tab pages should be closed
  • Loading branch information
NejcZdovc committed Apr 4, 2017
1 parent 8182478 commit 54cf311
Show file tree
Hide file tree
Showing 10 changed files with 204 additions and 5 deletions.
1 change: 1 addition & 0 deletions app/extensions/brave/locales/en-US/menu.properties
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ openLocation=Open Location…
openSearch=Search for "{{selectedVariable}}"
importFrom=Import from…
closeWindow=Close Window
closeTabPage=Close tab page
savePageAs=Save Page as…
spreadTheWord=Spread the Word About Brave…
share=Share…
Expand Down
1 change: 1 addition & 0 deletions app/locale.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ var rendererIdentifiers = function () {
'closeOtherTabs',
'closeTabsToRight',
'closeTabsToLeft',
'closeTabPage',
'bookmarkPage',
'bookmarkLink',
'openFile',
Expand Down
10 changes: 10 additions & 0 deletions docs/windowActions.md
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,16 @@ Dispatches a message to close a frame



### closeFrames(framePropsList)

Dispatches a message to close multiple frames

**Parameters**

**framePropsList**: `Array.<Object>`, The properties of all frames to close



### undoClosedFrame()

Dispatches a message to the store to undo a closed frame
Expand Down
28 changes: 28 additions & 0 deletions js/actions/windowActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const appActions = require('../actions/appActions')
const messages = require('../constants/messages')
const siteTags = require('../constants/siteTags')
const siteUtil = require('../state/siteUtil')
const frameStateUtil = require('../state/frameStateUtil')
const UrlUtil = require('../lib/urlutil')
const {currentWindow} = require('../../app/renderer/currentWindow')
const windowStore = require('../stores/windowStore')
Expand Down Expand Up @@ -346,6 +347,33 @@ const windowActions = {
}
},

/**
* Dispatches a message to close multiple frames
* @param {Object[]} framePropsList - The properties of all frames to close
*/
closeFrames: function (framePropsList) {
const activeFrameKey = windowStore.getState().get('activeFrameKey')
const activeFrame = frameStateUtil.findFrameInList(framePropsList, activeFrameKey)

if (activeFrame) {
const origin = siteUtil.getOrigin(activeFrame.get('location'))
if (origin) {
appActions.clearMessageBoxes(origin)
}

// If the frame was full screen, exit
if (activeFrame && activeFrame.get('isFullScreen')) {
this.setFullScreen(activeFrame, false)
}
}

dispatch({
actionType: windowConstants.WINDOW_CLOSE_FRAMES,
framePropsList,
activeFrameRemoved: activeFrame
})
},

/**
* Dispatches a message to the store to undo a closed frame
* The new frame is expected to appear at the index it was last closed at
Expand Down
10 changes: 5 additions & 5 deletions js/components/tabPages.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ class TabPage extends ImmutableComponent {
}

onDrop (e) {
if (this.props.frames.size === 0) {
if (this.props.tabPageFrames.size === 0) {
return
}
const moveToFrame = this.props.frames.get(0)
const moveToFrame = this.props.tabPageFrames.get(0)
const sourceDragData = dndData.getDragData(e.dataTransfer, dragTypes.TAB)
const sourceDragFromPageIndex = this.props.sourceDragFromPageIndex
// This must be executed async because the state change that this causes
Expand All @@ -63,7 +63,7 @@ class TabPage extends ImmutableComponent {
}

render () {
const audioPlaybackActive = this.props.frames.find((frame) =>
const audioPlaybackActive = this.props.tabPageFrames.find((frame) =>
frame.get('audioPlaybackActive') && !frame.get('audioMuted'))
return <span data-tab-page={this.props.index}
onDragOver={this.onDragOver.bind(this)}
Expand All @@ -74,7 +74,7 @@ class TabPage extends ImmutableComponent {
tabPage: true,
audioPlaybackActive,
active: this.props.active})}
onContextMenu={onTabPageContextMenu.bind(this, this.props.frames)}
onContextMenu={onTabPageContextMenu.bind(this, this.props.tabPageFrames)}
onClick={windowActions.setTabPageIndex.bind(this, this.props.index)
} />
}
Expand All @@ -97,7 +97,7 @@ class TabPages extends ImmutableComponent {
Array.from(new Array(tabPageCount)).map((x, i) =>
<TabPage
key={`tabPage-${i}`}
frames={this.props.frames.slice(i * this.props.tabsPerTabPage, (i * this.props.tabsPerTabPage) + this.props.tabsPerTabPage)}
tabPageFrames={this.props.frames.slice(i * this.props.tabsPerTabPage, (i * this.props.tabsPerTabPage) + this.props.tabsPerTabPage)}
previewTabPage={this.props.previewTabPage}
index={i}
sourceDragFromPageIndex={sourceDragFromPageIndex}
Expand Down
1 change: 1 addition & 0 deletions js/constants/windowConstants.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const windowConstants = {
WINDOW_NEW_FRAME: _,
WINDOW_VIEW_KEY: _,
WINDOW_CLOSE_FRAME: _,
WINDOW_CLOSE_FRAMES: _,
WINDOW_SET_ACTIVE_FRAME: _,
WINDOW_SET_FOCUSED_FRAME: _,
WINDOW_SET_PREVIEW_FRAME: _,
Expand Down
5 changes: 5 additions & 0 deletions js/contextMenus.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@ function tabPageTemplateInit (framePropsList) {
click: (item, focusedWindow) => {
windowActions.muteAllAudio(framePropsList, true)
}
}, {
label: locale.translation('closeTabPage'),
click: () => {
windowActions.closeFrames(framePropsList)
}
}]
}

Expand Down
68 changes: 68 additions & 0 deletions js/state/frameStateUtil.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const Immutable = require('immutable')
const config = require('../constants/config')
const {tabCloseAction} = require('../../app/common/constants/settingsEnums')
const urlParse = require('../../app/common/urlParse')
const {makeImmutable} = require('../../app/common/state/immutableUtil')

const matchFrame = (queryInfo, frame) => {
queryInfo = queryInfo.toJS ? queryInfo.toJS() : queryInfo
Expand Down Expand Up @@ -49,6 +50,10 @@ function getFrameByIndex (windowState, i) {
return windowState.getIn(['frames', i])
}

function findFrameInList (frames, key) {
return frames.find(matchFrame.bind(null, {key}))
}

// This will eventually go away fully when we replace frameKey by tabId
function getFrameKeyByTabId (windowState, tabId) {
let parentFrameKey
Expand Down Expand Up @@ -493,6 +498,67 @@ function removeFrame (frames, tabs, closedFrames, frameProps, activeFrameKey, fr
frames: newFrames
}
}
/**
* Removes a frames specified by framePropsList
* @return Immutable top level application state ready to merge back in
*/
function removeFrames (frames, tabs, closedFrames, framePropsList, activeFrameRemoved, activeFrameKey, closeAction) {
function getLastTab (newFrames) {
const sorted = newFrames
.filter((frame) => !frame.get('pinnedLocation'))
.sortBy((item) => item.get('key'))

return (sorted.size === 0) ? 0 : sorted.last().get('key')
}

function getLastActiveTab (newFrames) {
const sorted = newFrames
.filter((frame) => !frame.get('pinnedLocation'))
.sortBy((item) => item.get('lastAccessedTime') || 0)

return (sorted.size === 0) ? 0 : sorted.last().get('key')
}

let newFrames = makeImmutable(frames)
let newTabs = makeImmutable(tabs)

framePropsList.forEach((frameProps) => {
if (!frameProps.get('isPrivate') && frameProps.get('location') !== 'about:newtab') {
frameProps = frameProps.set('isFullScreen', false)
closedFrames = closedFrames.push(frameProps)
if (frameProps.get('thumbnailBlob')) {
window.URL.revokeObjectURL(frameProps.get('thumbnailBlob'))
}
if (closedFrames.size > config.maxClosedFrames) {
closedFrames = closedFrames.shift()
}
}

let framePropsIndex = getFramePropsIndex(newFrames, frameProps)
newFrames = newFrames.splice(framePropsIndex, 1)
newTabs = newTabs.splice(framePropsIndex, 1)
})

// return last non pinned frame index if active frame was removed
if (activeFrameRemoved) {
switch (closeAction) {
case tabCloseAction.LAST_ACTIVE:
activeFrameKey = getLastActiveTab(newFrames)
break
default:
activeFrameKey = getLastTab(newFrames)
break
}
}

return {
previewFrameKey: null,
activeFrameKey,
closedFrames,
tabs: newTabs,
frames: newFrames
}
}

/**
* Removes all but the specified frameProps
Expand Down Expand Up @@ -568,12 +634,14 @@ module.exports = {
getFramePropPath,
findIndexForFrameKey,
findDisplayIndexForFrameKey,
findFrameInList,
getFramePropsIndex,
getFrameKeysByDisplayIndex,
getPartition,
addFrame,
undoCloseFrame,
removeFrame,
removeFrames,
removeOtherFrames,
tabFromFrame,
getFrameKeyByTabId,
Expand Down
14 changes: 14 additions & 0 deletions js/stores/windowStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,20 @@ const doAction = (action) => {
})
}
break
case windowConstants.WINDOW_CLOSE_FRAMES:
windowState = windowState.merge(frameStateUtil.removeFrames(
windowState.get('frames'),
windowState.get('tabs'),
windowState.get('closedFrames'),
action.framePropsList,
action.activeFrameRemoved,
windowState.get('activeFrameKey'),
getSetting(settings.TAB_CLOSE_ACTION)
))

updateTabPageIndex(frameStateUtil.getActiveFrame(windowState))
windowState = windowState.deleteIn(['ui', 'tabs', 'fixTabWidth'])
break
case windowConstants.WINDOW_UNDO_CLOSED_FRAME:
windowState = windowState.merge(frameStateUtil.undoCloseFrame(windowState, windowState.get('closedFrames')))
focusWebview(activeFrameStatePath(windowState))
Expand Down
71 changes: 71 additions & 0 deletions test/unit/state/frameStateUtilTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -283,4 +283,75 @@ describe('frameStateUtil', function () {
})
})
})

describe('removeFrames', function () {
let frames, tabs, closedFrames, framePropsList, activeFrameKey, activeFrameRemoved, activeFrameKeyKeep

beforeEach(function () {
frames = Immutable.fromJS([
{ key: 2 },
{ key: 3, parentFrameKey: 2 },
{ key: 4 },
{ key: 5, pinnedLocation: 'https://twitter.com/', lastAccessedTime: 1488184050731 },
{ key: 6, lastAccessedTime: 1488184050721 },
{ key: 7, lastAccessedTime: undefined },
{ key: 8, lastAccessedTime: 1488184050711 },
{ key: 9, pinnedLocation: 'https://brave.com/' }
])
tabs = Immutable.fromJS([
{ key: 2 },
{ key: 3 },
{ key: 4 },
{ key: 5, pinnedLocation: 'https://twitter.com/' },
{ key: 6 },
{ key: 7 },
{ key: 8 },
{ key: 9, pinnedLocation: 'https://brave.com/' }
])
closedFrames = Immutable.fromJS([
{ key: 1 }
])
framePropsList = Immutable.fromJS([
{ key: 2 },
{ key: 3 },
{ key: 4 }
])
activeFrameRemoved = Immutable.fromJS({ key: 2 })
activeFrameKey = 2
activeFrameKeyKeep = 6
})

it('removes frames from `frames`', function () {
const result = frameStateUtil.removeFrames(frames, tabs, closedFrames, framePropsList, undefined, activeFrameKeyKeep)
assert.equal(5, result.frames.size)
})

it('removed frames are in `closedFrame`', function () {
const result = frameStateUtil.removeFrames(frames, tabs, closedFrames, framePropsList, activeFrameRemoved, activeFrameKey)
assert.equal((framePropsList.size + closedFrames.size), result.closedFrames.size)
})

it('active frame is the same', function () {
const result = frameStateUtil.removeFrames(frames, tabs, closedFrames, framePropsList, undefined, activeFrameKeyKeep)
assert.equal(activeFrameKeyKeep, result.activeFrameKey)
})

describe('when active frame is removed', function () {
it('removes frames from `frames`', function () {
const result = frameStateUtil.removeFrames(frames, tabs, closedFrames, framePropsList, activeFrameRemoved, activeFrameKey)
const inFrames = result.frames.find((frame) => frame.get('key') === activeFrameRemoved.get('key'))
assert.equal(true, inFrames === undefined)
})

it('new active frame (last tab)', function () {
const result = frameStateUtil.removeFrames(frames, tabs, closedFrames, framePropsList, activeFrameRemoved, activeFrameKey)
assert.equal(8, result.activeFrameKey)
})

it('new active frame (last active tab)', function () {
const result = frameStateUtil.removeFrames(frames, tabs, closedFrames, framePropsList, activeFrameRemoved, activeFrameKey, tabCloseAction.LAST_ACTIVE)
assert.equal(6, result.activeFrameKey)
})
})
})
})

1 comment on commit 54cf311

@bsclifton
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

Please sign in to comment.