Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Removes non-primitive types from NavigationBar #9792

Closed
wants to merge 1 commit into from
Closed

Removes non-primitive types from NavigationBar #9792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 1 addition & 22 deletions app/renderer/components/navigation/navigationBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ const windowActions = require('../../../../js/actions/windowActions')
const appActions = require('../../../../js/actions/appActions')

// Constants
const siteTags = require('../../../../js/constants/siteTags')
const messages = require('../../../../js/constants/messages')
const settings = require('../../../../js/constants/settings')

Expand All @@ -28,15 +27,10 @@ const tabState = require('../../../common/state/tabState')
const publisherState = require('../../../common/lib/publisherUtil')
const frameStateUtil = require('../../../../js/state/frameStateUtil')

// Store
const windowStore = require('../../../../js/stores/windowStore')

// Utils
const cx = require('../../../../js/lib/classSet')
const {getBaseUrl} = require('../../../../js/lib/appUrlUtil')
const siteUtil = require('../../../../js/state/siteUtil')
const eventUtil = require('../../../../js/lib/eventUtil')
const UrlUtil = require('../../../../js/lib/urlutil')
const {getSetting} = require('../../../../js/settings')
const contextMenus = require('../../../../js/contextMenus')

Expand All @@ -51,22 +45,8 @@ class NavigationBar extends React.Component {
this.onReloadLongPress = this.onReloadLongPress.bind(this)
}

get activeFrame () {
return windowStore.getFrame(this.props.activeFrameKey)
}

onToggleBookmark () {
const editing = this.props.isBookmarked
// show the AddEditBookmarkHanger control; saving/deleting takes place there
let siteDetail = siteUtil.getDetailFromFrame(this.activeFrame, siteTags.BOOKMARK)
const key = siteUtil.getSiteKey(siteDetail)

if (key !== null) {
siteDetail = siteDetail.set('parentFolderId', this.props.sites.getIn([key, 'parentFolderId']))
siteDetail = siteDetail.set('customTitle', this.props.sites.getIn([key, 'customTitle']))
}
siteDetail = siteDetail.set('location', UrlUtil.getLocationIfPDF(siteDetail.get('location')))
windowActions.setBookmarkDetail(siteDetail, siteDetail, null, editing, true)
windowActions.onToggleBookmark(this.props.isBookmarked)
}

onReload (e) {
Expand Down Expand Up @@ -145,7 +125,6 @@ class NavigationBar extends React.Component {

// used in other functions
props.navbar = navbar // TODO(nejc) remove, primitives only
props.sites = state.get('sites') // TODO(nejc) remove, primitives only
props.activeTabId = activeTabId
props.showHomeButton = !props.titleMode && getSetting(settings.SHOW_HOME_BUTTON)

Expand Down
7 changes: 7 additions & 0 deletions js/actions/windowActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1142,6 +1142,13 @@ const windowActions = {
url,
error
})
},

onToggleBookmark: function (isBookmarked) {
dispatch({
actionType: windowConstants.WINDOW_ON_TOGGLE_BOOKMARK,
isBookmarked
})
}
}

Expand Down
3 changes: 2 additions & 1 deletion js/constants/windowConstants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ const windowConstants = {
WINDOW_ON_GO_BACK_LONG: _,
WINDOW_ON_GO_FORWARD_LONG: _,
WINDOW_CLOSE_OTHER_FRAMES: _,
WINDOW_ON_CERT_ERROR: _
WINDOW_ON_CERT_ERROR: _,
WINDOW_ON_TOGGLE_BOOKMARK: _
}

module.exports = mapValuesByKeys(windowConstants)
20 changes: 19 additions & 1 deletion js/stores/windowStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const frameStateUtil = require('../state/frameStateUtil')
const ipc = require('electron').ipcRenderer
const messages = require('../constants/messages')
const debounce = require('../lib/debounce')
const getSetting = require('../settings').getSetting
const {getSetting} = require('../settings')
const UrlUtil = require('../lib/urlutil')
const {l10nErrorText} = require('../../app/common/lib/httpUtil')
const { makeImmutable } = require('../../app/common/state/immutableUtil')
Expand All @@ -23,6 +23,8 @@ const assert = require('assert')
const contextMenuState = require('../../app/common/state/contextMenuState')
const appStoreRenderer = require('./appStoreRenderer')
const windowActions = require('../actions/windowActions')
const siteUtil = require('../state/siteUtil')
const siteTags = require('../constants/siteTags')

let windowState = Immutable.fromJS({
activeFrameKey: null,
Expand Down Expand Up @@ -750,6 +752,22 @@ const doAction = (action) => {
}
break
}
case windowConstants.WINDOW_ON_TOGGLE_BOOKMARK:
{
// show the AddEditBookmarkHanger control; saving/deleting takes place there
const activeFrame = frameStateUtil.getActiveFrame(windowState)
let siteDetail = siteUtil.getDetailFromFrame(activeFrame, siteTags.BOOKMARK)
const key = siteUtil.getSiteKey(siteDetail)

if (key !== null) {
Copy link
Member

Choose a reason for hiding this comment

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

this logic here would be better if it was contained inside siteUtil.getDetailFromFrame method, IMO (especially since it can be tested). There are other places that call this method and they too should be having the parentFolderId and customTitle set (in fact, if the other calls are not setting this, it may be causing a bug / bugs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why this logic should be in siteUtil.getDetailFromFrame. In this if statement we are getting data from appStore and adding them to the siteDetail. Not sure where else would we be using this? We use this function in 13 different places. We only want to access appStore in reducers and not in components.

Copy link
Member

@bsclifton bsclifton Jul 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

the data it gets from appStore is the site which matches... getDetailFromFrame should be returning the entry from the sites map (which is what siteUtil does; it's the wrapper around the sites map which is stored in appStore)

In this case, getDetailFromFrame is just taking data from the frame and returning it (without transforming it)... rather than doing a lookup and fetching the already existing one. This code here then copies the data from the existing one (which is a mistake IMO, since it could have just used the existing reference)

Copy link
Member

@bsclifton bsclifton Jul 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Here's a quick explanation of why this is a problem:
if you remove this line:
siteDetail = siteDetail.set('parentFolderId', site.get('parentFolderId'))

The code would then create a NEW sites entry (which is wrong). The reason why it creates a new one is because the key has the folderId in it. The windowStore shouldn't care about this logic- it should just use the object it gets back. Instead, we are having business logic in the windowStore about a siteDetails entry to avoid a bug (instead of returning an existing reference- the one which the key matches)

Copy link
Member

Choose a reason for hiding this comment

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

this would fix the problem, although it's a little more sloppy:

siteDetail = appStoreRenderer.state.getIn(['sites', key], Immutable.Map())
// siteDetail = siteDetail.set('parentFolderId', site.get('parentFolderId'))
// siteDetail = siteDetail.set('customTitle', site.get('customTitle'))

Copy link
Contributor Author

@NejcZdovc NejcZdovc Jul 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure that we can to that, because now in your example you overrode sites from window state, which means that bookmark will not have favicon, tags, themeColor etc. It will only have parentFolderId, title and customTitle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are only missing parentFolderId in this example, I just checked and everything else is there. But I think this whole PR, will be redundant when I do #9710 😃

Copy link
Member

@bsclifton bsclifton Jul 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

that's true- maybe it's OK as-is (for this PR) and that would be the place to tackle it 😄

The example above should have everything (including parentFolderId). In the current code, whatever is found is copied from appState. So instead of copying it, just use the entire object (which like you said, has favicon, tags, themeColor, etc). This is great because if we needed to expose those we could. By not using the real object, we're opening ourselves up to bugs. For example, what if parentFolderId is changed to parentDirectoryId. If we properly handled it, we'd only need to update in siteUtil. But because this code is copying the object, we'd have to update in 2 (or more) places

const site = appStoreRenderer.state.getIn(['sites', key], Immutable.Map())
siteDetail = siteDetail.set('parentFolderId', site.get('parentFolderId'))
siteDetail = siteDetail.set('customTitle', site.get('customTitle'))
}
siteDetail = siteDetail.set('location', UrlUtil.getLocationIfPDF(siteDetail.get('location')))
windowActions.setBookmarkDetail(siteDetail, siteDetail, null, action.isBookmarked, true)
Copy link
Member

Choose a reason for hiding this comment

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

This is definitely something we want to avoid (calling an action in the handler for an action). If it can't be avoided, I think it would be preferable to call doAction like so:

doAction({
  actionType: windowConstants.WINDOW_SET_BOOKMARK_DETAIL,
  currentDetail: siteDetail,
  originalDetail: siteDetail,
  destinationDetail: null,
  shouldShowLocation: action.isBookmarked,
  isBookmarkHanger: true
})

cc: @bridiver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we want to remove this, but sadly we need to keep it for now. You will see this pattern in other PR's as well. For this particular case we will remove it in #9710, because we will remove setBookmarkDetail as per our discussion.

break
}
default:
break
}
Expand Down