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

Commit

Permalink
Avoid serializing Function in array template
Browse files Browse the repository at this point in the history
Auditors: @bridiver

Test Plan:
1. Set `sessionSaveInterval` to `1` in js/constants/appConfig.js
2. Playing around with bookmark toolbar folder
3. Playing around with auotfill menu(fill with entry and clear)
4. Playing around with hamburger menu
5. Playing around with back/forward long pressed menu
6. There shouldn't be any exceptions about "function can be cloned"
  • Loading branch information
darkdh committed Mar 13, 2018
1 parent 9a6ca8b commit e2322c8
Show file tree
Hide file tree
Showing 7 changed files with 49 additions and 45 deletions.
24 changes: 14 additions & 10 deletions app/common/state/contextMenuState.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,38 +5,42 @@
const Immutable = require('immutable')
const assert = require('assert')
const { makeImmutable, isMap } = require('./immutableUtil')
const uuid = require('uuid')

const validateState = function (state) {
state = makeImmutable(state)
assert.ok(isMap(state), 'state must be an Immutable.Map')
return state
}

let contextMenuDetail = Immutable.Map()
let hamburgerMenuWasOpen = false

const api = {
setContextMenu: (windowState, detail) => {
detail = makeImmutable(detail)
windowState = validateState(windowState)

if (!detail) {
if (windowState.getIn(['contextMenuDetail', 'type']) === 'hamburgerMenu') {
windowState = windowState.set('hamburgerMenuWasOpen', true)
if (contextMenuDetail.get('type') === 'hamburgerMenu') {
hamburgerMenuWasOpen = true
} else {
windowState = windowState.set('hamburgerMenuWasOpen', false)
hamburgerMenuWasOpen = false
}
contextMenuDetail = Immutable.Map()
windowState = windowState.delete('contextMenuDetail')
} else {
if (!(detail.get('type') === 'hamburgerMenu' && windowState.get('hamburgerMenuWasOpen'))) {
windowState = windowState.set('contextMenuDetail', detail)
if (!(detail.get('type') === 'hamburgerMenu' && hamburgerMenuWasOpen)) {
contextMenuDetail = detail
windowState = windowState.set('contextMenuDetail', uuid())
}
windowState = windowState.set('hamburgerMenuWasOpen', false)
hamburgerMenuWasOpen = false
}

return windowState
},

getContextMenu: (windowState) => {
windowState = validateState(windowState)
return windowState.get('contextMenuDetail', Immutable.Map())
getContextMenu: () => {
return contextMenuDetail
},

selectedIndex: (windowState) => {
Expand Down
5 changes: 4 additions & 1 deletion app/renderer/components/common/contextMenu/contextMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ const windowActions = require('../../../../../js/actions/windowActions')
// Constants
const keyCodes = require('../../../../common/constants/keyCodes')

// State
const contextMenuState = require('../../../../common/state/contextMenuState')

// Utils
const cx = require('../../../../../js/lib/classSet')
const frameStateUtil = require('../../../../../js/state/frameStateUtil')
Expand Down Expand Up @@ -215,7 +218,7 @@ class ContextMenu extends React.Component {
const currentWindow = state.get('currentWindow')
const activeFrame = frameStateUtil.getActiveFrame(currentWindow) || Immutable.Map()
const selectedIndex = currentWindow.getIn(['ui', 'contextMenu', 'selectedIndex'], null)
const contextMenuDetail = currentWindow.get('contextMenuDetail', Immutable.Map())
const contextMenuDetail = contextMenuState.getContextMenu()

const props = {}
props.lastZoomPercentage = activeFrame.get('lastZoomPercentage')
Expand Down
3 changes: 2 additions & 1 deletion app/renderer/components/frame/frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ const windowStore = require('../../../../js/stores/windowStore')
const appStoreRenderer = require('../../../../js/stores/appStoreRenderer')

// State
const contextMenuState = require('../../../common/state/contextMenuState')
const siteSettings = require('../../../../js/state/siteSettings')
const siteSettingsState = require('../../../common/state/siteSettingsState')
const tabState = require('../../../common/state/tabState')
Expand Down Expand Up @@ -856,7 +857,7 @@ class Frame extends React.Component {
const allSiteSettings = siteSettingsState.getAllSiteSettings(state, isPrivate)
const frameSiteSettings = siteSettings.getSiteSettingsForURL(allSiteSettings, location) || Immutable.Map()

const contextMenu = currentWindow.get('contextMenuDetail')
const contextMenu = contextMenuState.getContextMenu()
const tab = tabId && tabId > -1 && tabState.getByTabId(state, tabId)

const props = {}
Expand Down
2 changes: 1 addition & 1 deletion app/renderer/reducers/contextMenuReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,7 @@ const onShowBookmarkFolderMenu = (state, action) => {

const menuTemplate = showBookmarkFolderInit(state, action.get('bookmarkKey'))
if (action.get('submenuIndex') != null) {
let contextMenu = contextMenuState.getContextMenu(state)
let contextMenu = contextMenuState.getContextMenu()
let openedSubmenuDetails = contextMenu.get('openedSubmenuDetails', Immutable.List())

openedSubmenuDetails = openedSubmenuDetails
Expand Down
18 changes: 1 addition & 17 deletions docs/state.md
Original file line number Diff line number Diff line change
Expand Up @@ -674,23 +674,7 @@ WindowStore
},
cleanedOnShutdown: boolean, // whether app data was successfully cleared on shutdown
closedFrames: [], // holds the same type of frame objects as frames
contextMenuDetail: {
bottom: number, // the bottom position of the context menu
left: number, // the left position of the context menu
maxHeight: number, // the maximum height of the context menu
openedSubmenuDetails: [{
template: [], // same as template in contextMenuDetail
y: number // the relative y position
}],
right: number, // the right position of the context menu
template: [{
click: function, // callback for the context menu to call when clicked
dragOver: function, // callback for when something is dragged over this item
drop: function, // callback for when something is dropped on this item
label: string // label of context menu item
}],
top: number // the top position of the context menu
},
contextMenuDetail: string, // uuid for triggering state update
createdFaviconDirectory: boolean, // whether the ledger-favicons directory has been created already in the appData directory
frames: [{
aboutDetails: object, // details for about pages
Expand Down
13 changes: 9 additions & 4 deletions js/actions/windowActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -531,14 +531,19 @@ const windowActions = {
},

/**
* Dispatches a message to set context menu detail.
* If set, also indicates that the context menu is shown.
* @param {Object} detail - The context menu detail
* Dispatches a message to set context menu uuid and also set detail
* in contextMenuState.
* If uuid set, also indicates that the context menu is shown.
* @param {Object} detail - uuid
*/
setContextMenuDetail: function (detail) {
const Immutable = require('immutable')
const contextMenuState = require('../../app/common/state/contextMenuState')
let state = contextMenuState.setContextMenu(Immutable.Map(), detail)
const contextMenuDetail = state.get('contextMenuDetail')
dispatch({
actionType: windowConstants.WINDOW_SET_CONTEXT_MENU_DETAIL,
detail
contextMenuDetail
})
},

Expand Down
29 changes: 18 additions & 11 deletions js/stores/windowStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -465,21 +465,28 @@ const doAction = (action) => {
windowState = windowState.delete('bookmarkFolderDetail')
break
case windowConstants.WINDOW_AUTOFILL_SELECTION_CLICKED:
windowState = contextMenuState.setContextMenu(windowState)
ipc.send('autofill-selection-clicked', action.tabId, action.value, action.frontEndId, action.index)
windowState = windowState.delete('contextMenuDetail')
break
case windowConstants.WINDOW_AUTOFILL_POPUP_HIDDEN:
if (!action.detail &&
windowState.getIn(['contextMenuDetail', 'type']) === 'autofill' &&
windowState.getIn(['contextMenuDetail', 'tabId']) === action.tabId) {
windowState = windowState.delete('contextMenuDetail')
if (action.notify) {
ipc.send('autofill-popup-hidden', action.tabId)
{
const contextMenuDetail = contextMenuState.getContextMenu()
if (!action.detail &&
contextMenuDetail.get('type') === 'autofill' &&
contextMenuDetail.get('tabId') === action.tabId) {
windowState = contextMenuState.setContextMenu(windowState)
if (action.notify) {
ipc.send('autofill-popup-hidden', action.tabId)
}
}
break
}
break
case windowConstants.WINDOW_SET_CONTEXT_MENU_DETAIL:
windowState = contextMenuState.setContextMenu(windowState, action.detail)
if (action.contextMenuDetail) {
windowState = windowState.set('contextMenuDetail', action.contextMenuDetail)
} else {
windowState = windowState.delete('contextMenuDetail')
}
break
case windowConstants.WINDOW_SET_POPUP_WINDOW_DETAIL:
if (!action.detail) {
Expand Down Expand Up @@ -641,7 +648,7 @@ const doAction = (action) => {
break
case windowConstants.WINDOW_TOGGLE_MENUBAR_VISIBLE:
if (getSetting(settings.AUTO_HIDE_MENU)) {
doAction({actionType: windowConstants.WINDOW_SET_CONTEXT_MENU_DETAIL})
windowState = contextMenuState.setContextMenu(windowState)
// Use value if provided; if not, toggle to opposite.
const newVisibleStatus = typeof action.isVisible === 'boolean'
? action.isVisible
Expand All @@ -665,7 +672,7 @@ const doAction = (action) => {
if (getSetting(settings.AUTO_HIDE_MENU)) {
doAction({actionType: windowConstants.WINDOW_TOGGLE_MENUBAR_VISIBLE, isVisible: false})
} else {
doAction({actionType: windowConstants.WINDOW_SET_CONTEXT_MENU_DETAIL})
windowState = contextMenuState.setContextMenu(windowState)
}
doAction({actionType: windowConstants.WINDOW_SET_MENUBAR_SELECTED_INDEX})
doAction({actionType: windowConstants.WINDOW_SET_CONTEXT_MENU_SELECTED_INDEX})
Expand Down

0 comments on commit e2322c8

Please sign in to comment.