Skip to content

Commit

Permalink
Fix deleting history entry
Browse files Browse the repository at this point in the history
fix brave#8761

test plan:
1. open a new tab, go to some site
2. close the tab
3. open about:history. right-click to delete the site. it should disappear from the list
4. open the History menu. you should no longer see the site under 'Recently Closed'.
5. repeat above steps but opening/closing two tabs and selecting both of them to delete instead of only one
  • Loading branch information
diracdeltas authored and dfperry5 committed Aug 18, 2017
1 parent 57115cc commit a732ed5
Show file tree
Hide file tree
Showing 8 changed files with 33 additions and 17 deletions.
13 changes: 10 additions & 3 deletions app/browser/menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -673,8 +673,15 @@ const doAction = (state, action) => {
}
case windowConstants.WINDOW_CLEAR_CLOSED_FRAMES:
{
closedFrames = new Immutable.OrderedMap()
lastClosedUrl = null
if (!action.location) {
closedFrames = new Immutable.OrderedMap()
lastClosedUrl = null
} else {
closedFrames = closedFrames.delete(action.location)
if (lastClosedUrl === action.location) {
lastClosedUrl = null
}
}
updateRecentlyClosedMenuItems(state)
break
}
Expand Down Expand Up @@ -720,7 +727,7 @@ const doAction = (state, action) => {
}
case appConstants.APP_REMOVE_SITE:
{
if (action.tag === siteTags.BOOKMARK || action.tag === siteTags.BOOKMARK_FOLDER) {
if (!action.tag || action.tag === siteTags.BOOKMARK || action.tag === siteTags.BOOKMARK_FOLDER) {
createMenu(state)
}
break
Expand Down
7 changes: 5 additions & 2 deletions js/actions/windowActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -214,10 +214,13 @@ const windowActions = {

/**
* Dispatches a message to the store to clear closed frames
* @param {string=} location - If specified, only clear frames with this
* location.
*/
clearClosedFrames: function () {
clearClosedFrames: function (location) {
dispatch({
actionType: windowConstants.WINDOW_CLEAR_CLOSED_FRAMES
actionType: windowConstants.WINDOW_CLEAR_CLOSED_FRAMES,
location
})
},

Expand Down
4 changes: 2 additions & 2 deletions js/entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ ipc.on(messages.APP_STATE_CHANGE, (e, action) => {
: appStoreRenderer.state = Immutable.fromJS(action.state)
})

ipc.on(messages.CLEAR_CLOSED_FRAMES, () => {
windowActions.clearClosedFrames()
ipc.on(messages.CLEAR_CLOSED_FRAMES, (e, location) => {
windowActions.clearClosedFrames(location)
})

window.addEventListener('beforeunload', function (e) {
Expand Down
5 changes: 5 additions & 0 deletions js/state/siteUtil.js
Original file line number Diff line number Diff line change
Expand Up @@ -417,6 +417,11 @@ module.exports.removeSite = function (state, siteDetail, tag, reorder = true, sy
site = site.set('tags', tags)
return state.setIn(stateKey, site)
} else {
const siteDetailTags = siteDetail.get('tags')
if (!tag && (!siteDetailTags || siteDetailTags.size === 0)) {
// Delete the site from history
return state.deleteIn(stateKey)
}
site = site.set('lastAccessedTime', undefined)
return state.setIn(stateKey, site)
}
Expand Down
3 changes: 3 additions & 0 deletions js/stores/appStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,9 @@ const handleAppAction = (action) => {
case appConstants.APP_REMOVE_SITE:
calculateTopSites(true)
appState = aboutHistoryState.setHistory(appState, action)
if (!action.tag && siteUtil.isHistoryEntry(action.siteDetail)) {
BrowserWindow.getAllWindows().forEach((wnd) => wnd.webContents.send(messages.CLEAR_CLOSED_FRAMES, action.siteDetail.get('location')))
}
break
case appConstants.APP_SET_DATA_FILE_ETAG:
appState = appState.setIn([action.resourceName, 'etag'], action.etag)
Expand Down
7 changes: 6 additions & 1 deletion js/stores/windowStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,12 @@ const doAction = (action) => {
}
break
case windowConstants.WINDOW_CLEAR_CLOSED_FRAMES:
windowState = windowState.set('closedFrames', new Immutable.List())
if (!action.location) {
windowState = windowState.set('closedFrames', new Immutable.List())
} else {
windowState = windowState.set('closedFrames',
windowState.get('closedFrames').filterNot((frame) => frame.get('location') === action.location))
}
break
case windowConstants.WINDOW_SET_PREVIEW_FRAME:
windowState = frameStateUtil.setPreviewFrameKey(windowState, action.frameKey, true)
Expand Down
3 changes: 1 addition & 2 deletions test/unit/app/browser/reducers/sitesReducerTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,7 @@ describe('sitesReducerTest', function () {
let newState = sitesReducer(state, action)
action.actionType = appConstants.APP_REMOVE_SITE
newState = sitesReducer(newState, action).toJS()
assert.equal(Object.keys(newState.sites).length, 1)
assert.equal(Object.keys(newState.sites)[0].lastAccessedTime, undefined)
assert.equal(Object.keys(newState.sites).length, 0)
})
})
describe('APP_MOVE_SITE', function () {
Expand Down
8 changes: 1 addition & 7 deletions test/unit/state/siteUtilTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -702,13 +702,7 @@ describe('siteUtil', function () {
location: testUrl1,
lastAccessedTime: 123
}
const expectedSites = {
'https://brave.com/|0|0': {
tags: [],
location: testUrl1,
lastAccessedTime: undefined
}
}
const expectedSites = {}
const siteKey = siteUtil.getSiteKey(Immutable.fromJS(siteDetail))
let sites = {}
sites[siteKey] = siteDetail
Expand Down

0 comments on commit a732ed5

Please sign in to comment.