From 18e755f503265e89863e21dbe3f6799c4d845621 Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Wed, 27 Sep 2017 12:45:30 -0400 Subject: [PATCH] Respect bookmark position when adding bookmarks Fix #11178 Auditors: @bsclifton --- app/browser/reducers/bookmarksReducer.js | 5 +- app/common/state/bookmarksState.js | 4 +- .../components/bookmarks/bookmarksToolbar.js | 27 ++-- js/actions/appActions.js | 5 +- .../browser/reducers/bookmarksReducerTest.js | 116 ++++++++++++++++++ 5 files changed, 143 insertions(+), 14 deletions(-) diff --git a/app/browser/reducers/bookmarksReducer.js b/app/browser/reducers/bookmarksReducer.js index bf9df963289..bd0f7325507 100644 --- a/app/browser/reducers/bookmarksReducer.js +++ b/app/browser/reducers/bookmarksReducer.js @@ -30,6 +30,7 @@ const bookmarksReducer = (state, action, immutableAction) => { { const closestKey = action.get('closestKey') const bookmark = action.get('siteDetail') + const isLeftSide = action.get('isLeftSide') if (bookmark == null) { break @@ -39,14 +40,14 @@ const bookmarksReducer = (state, action, immutableAction) => { let bookmarkList = Immutable.List() action.get('siteDetail', Immutable.List()).forEach((bookmark) => { const bookmarkDetail = bookmarkUtil.buildBookmark(state, bookmark) - state = bookmarksState.addBookmark(state, bookmarkDetail, closestKey) + state = bookmarksState.addBookmark(state, bookmarkDetail, closestKey, !isLeftSide) state = syncUtil.updateObjectCache(state, bookmarkDetail, STATE_SITES.BOOKMARKS) bookmarkList = bookmarkList.push(bookmarkDetail) }) textCalc.calcTextList(bookmarkList) } else { const bookmarkDetail = bookmarkUtil.buildBookmark(state, bookmark) - state = bookmarksState.addBookmark(state, bookmarkDetail, closestKey) + state = bookmarksState.addBookmark(state, bookmarkDetail, closestKey, !isLeftSide) state = syncUtil.updateObjectCache(state, bookmarkDetail, STATE_SITES.BOOKMARKS) textCalc.calcText(bookmarkDetail, siteTags.BOOKMARK) } diff --git a/app/common/state/bookmarksState.js b/app/common/state/bookmarksState.js index 58c308ae732..7cee5561f9f 100644 --- a/app/common/state/bookmarksState.js +++ b/app/common/state/bookmarksState.js @@ -77,7 +77,7 @@ const bookmarksState = { return bookmarks }, - addBookmark: (state, bookmarkDetail, destinationKey) => { + addBookmark: (state, bookmarkDetail, destinationKey, isLeftSide) => { state = validateState(state) const key = bookmarkDetail.get('key') if (key === null) { @@ -86,7 +86,7 @@ const bookmarksState = { if (!state.hasIn([STATE_SITES.BOOKMARKS, key])) { state = bookmarkLocationCache.addCacheKey(state, bookmarkDetail.get('location'), key) - state = bookmarkOrderCache.addBookmarkToCache(state, bookmarkDetail.get('parentFolderId'), key, destinationKey) + state = bookmarkOrderCache.addBookmarkToCache(state, bookmarkDetail.get('parentFolderId'), key, destinationKey, isLeftSide) } state = state.setIn([STATE_SITES.BOOKMARKS, key], bookmarkDetail) diff --git a/app/renderer/components/bookmarks/bookmarksToolbar.js b/app/renderer/components/bookmarks/bookmarksToolbar.js index c9b66ddd92e..ada8e933170 100644 --- a/app/renderer/components/bookmarks/bookmarksToolbar.js +++ b/app/renderer/components/bookmarks/bookmarksToolbar.js @@ -51,15 +51,18 @@ class BookmarksToolbar extends React.Component { onDrop (e) { e.preventDefault() - const bookmark = dnd.prepareBookmarkDataFromCompatible(e.dataTransfer) - if (bookmark) { - // Figure out the droppedOn element filtering out the source drag item - let droppedOn = dnd.closestFromXOffset(this.bookmarkRefs.filter((bookmarkRef) => { + const getClosestFromPos = (clientX, sourceKey) => + dnd.closestFromXOffset(this.bookmarkRefs.filter((bookmarkRef) => { if (!bookmarkRef) { return false } - return bookmarkRef.props.bookmarkKey !== bookmark.get('key') + return bookmarkRef.props.bookmarkKey !== sourceKey }), e.clientX) + const bookmark = dnd.prepareBookmarkDataFromCompatible(e.dataTransfer) + if (bookmark) { + // Figure out the droppedOn element filtering out the source drag item + const bookmarkKey = bookmark.get('key') + const droppedOn = getClosestFromPos(e.clientX, bookmarkKey) if (droppedOn.selectedRef) { const isRightSide = !dnd.isLeftSide(ReactDOM.findDOMNode(droppedOn.selectedRef), e.clientX) const droppedOnKey = droppedOn.selectedRef.props.bookmarkKey @@ -74,6 +77,14 @@ class BookmarksToolbar extends React.Component { return } + const droppedOn = getClosestFromPos(e.clientX, undefined) + let isLeftSide = false + let closestKey + if (droppedOn.selectedRef) { + closestKey = droppedOn.selectedRef.props.bookmarkKey + isLeftSide = dnd.isLeftSide(ReactDOM.findDOMNode(droppedOn.selectedRef), e.clientX) + } + const droppedHTML = e.dataTransfer.getData('text/html') if (droppedHTML) { const parser = new window.DOMParser() @@ -83,7 +94,7 @@ class BookmarksToolbar extends React.Component { appActions.addBookmark(Immutable.fromJS({ title: a.innerText, location: e.dataTransfer.getData('text/plain') - })) + }), closestKey, isLeftSide) return } } @@ -93,7 +104,7 @@ class BookmarksToolbar extends React.Component { item.getAsString((name) => appActions.addBookmark(Immutable.fromJS({ location: item.type, title: name - }))) + }), closestKey, isLeftSide)) }) return } @@ -103,7 +114,7 @@ class BookmarksToolbar extends React.Component { .map((x) => x.trim()) .filter((x) => !x.startsWith('#') && x.length > 0) .forEach((url) => - appActions.addBookmark(Immutable.fromJS({ location: url }))) + appActions.addBookmark(Immutable.fromJS({ location: url }), closestKey, isLeftSide)) } onDragEnter (e) { diff --git a/js/actions/appActions.js b/js/actions/appActions.js index 93a494b6e42..dc5d2ae4b09 100644 --- a/js/actions/appActions.js +++ b/js/actions/appActions.js @@ -1493,11 +1493,12 @@ const appActions = { * @param siteDetail{Immutable.Map|Immutable.List} - Bookmark details that we want to add, this can be a List as well * @param closestKey{string} - Key of the sibling where we would like to place this new bookmark */ - addBookmark: function (siteDetail, closestKey) { + addBookmark: function (siteDetail, closestKey, isLeftSide = false) { dispatch({ actionType: appConstants.APP_ADD_BOOKMARK, siteDetail, - closestKey + closestKey, + isLeftSide }) }, diff --git a/test/unit/app/browser/reducers/bookmarksReducerTest.js b/test/unit/app/browser/reducers/bookmarksReducerTest.js index 7d7ff821acf..652a5547a2f 100644 --- a/test/unit/app/browser/reducers/bookmarksReducerTest.js +++ b/test/unit/app/browser/reducers/bookmarksReducerTest.js @@ -304,6 +304,122 @@ describe('bookmarksReducer unit test', function () { assert.equal(spyCalc.callCount, 1) assert.deepEqual(newState.toJS(), expectedState.toJS()) }) + + it('add a bookmark with a close bookmark prepending', function () { + const newState = state.set('bookmarks', Immutable.fromJS({ + 'https://www.clifton.io|0|0': { + lastAccessedTime: 0, + objectId: null, + title: 'Brave', + location: 'https://www.brave.com', + parentFolderId: 0 + }, + 'https://www.bbondy.io|0|0': { + lastAccessedTime: 0, + objectId: null, + title: 'Brave', + location: 'https://www.bbondy.io', + parentFolderId: 0 + }, + 'https://www.bridiver.io|0|0': { + lastAccessedTime: 0, + objectId: null, + title: 'Brave', + location: 'https://www.bridiver.io', + parentFolderId: 0 + } + })) + .setIn(['cache', 'bookmarkOrder', '0'], Immutable.fromJS([ + { + key: 'https://clifton.io/|0|0', + order: 0, + type: siteTags.BOOKMARK + }, + { + key: 'https://www.bbondy.io|0|0', + order: 1, + type: siteTags.BOOKMARK + }, + { + key: 'https://www.bridiver.io|0|0', + order: 2, + type: siteTags.BOOKMARK + } + ])) + const action = { + actionType: appConstants.APP_ADD_BOOKMARK, + siteDetail: Immutable.fromJS({ + parentFolderId: 0, + title: 'Brave', + location: 'https://www.brave.com' + }), + tag: siteTags.BOOKMARK, + closestKey: 'https://www.bbondy.io|0|0', + isLeftSide: true + } + + const newBookmarks = { + 'https://www.clifton.io|0|0': { + lastAccessedTime: 0, + objectId: null, + title: 'Brave', + location: 'https://www.brave.com', + parentFolderId: 0 + }, + 'https://www.bbondy.io|0|0': { + lastAccessedTime: 0, + objectId: null, + title: 'Brave', + location: 'https://www.bbondy.io', + parentFolderId: 0 + }, + 'https://www.brave.com|0|0': { + favicon: undefined, + key: 'https://www.brave.com|0|0', + objectId: null, + title: 'Brave', + location: 'https://www.brave.com', + parentFolderId: 0, + partitionNumber: 0, + skipSync: null, + themeColor: undefined, + type: 'bookmark', + width: 0 + }, + 'https://www.bridiver.io|0|0': { + lastAccessedTime: 0, + objectId: null, + title: 'Brave', + location: 'https://www.bridiver.io', + parentFolderId: 0 + } + } + + const newBookmarksOrder = [ + { + key: 'https://clifton.io/|0|0', + order: 0, + type: siteTags.BOOKMARK + }, + { + key: 'https://www.brave.com|0|0', + order: 1, + type: siteTags.BOOKMARK + }, + { + key: 'https://www.bbondy.io|0|0', + order: 2, + type: siteTags.BOOKMARK + }, + { + key: 'https://www.bridiver.io|0|0', + order: 3, + type: siteTags.BOOKMARK + }] + const result = bookmarksReducer(newState, action) + assert.deepEqual(result.get('bookmarks').toJS(), newBookmarks) + assert.deepEqual(result.getIn(['cache', 'bookmarkOrder', '0']).toJS(), newBookmarksOrder) + }) }) describe('APP_EDIT_BOOKMARK', function () {