Skip to content

Commit

Permalink
[WIP] Making the Clear Browsing Data dialog stateful
Browse files Browse the repository at this point in the history
  • Loading branch information
@maazadeeb
maazadeeb committed Nov 6, 2016
1 parent 951a127 commit 97537e3
Show file tree
Hide file tree
Showing 5 changed files with 49 additions and 12 deletions.
7 changes: 7 additions & 0 deletions js/actions/appActions.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,13 @@ const appActions = {
AppDispatcher.dispatch({
actionType: AppConstants.APP_DEFAULT_BROWSER_CHECK_COMPLETE
})
},

setClearBrowserDataDefaults: function (defaultValues) {

This comment has been minimized.

Sign in to view

Copy link
@bsclifton

bsclifton Nov 6, 2016

Instead of having a new action, I'd like to propose using the existing action (clearAppData). And we can rename it to have a more event based name, such as onClearBrowserData(). Then defaultValues can be passed as an optional 2nd parameter 😄
AppDispatcher.dispatch({
actionType: AppConstants.APP_UPDATE_CLEAR_BROWSER_DATA_DEFAULTS,
defaultValues
})
}
}

Expand Down
45 changes: 35 additions & 10 deletions js/components/clearBrowsingDataPanel.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* You can obtain one at http://mozilla.org/MPL/2.0/. */

const React = require('react')
const Immutable = require('immutable')
const ImmutableComponent = require('./immutableComponent')
const Dialog = require('./dialog')
const Button = require('./button')
Expand All @@ -24,14 +25,38 @@ class ClearBrowsingDataPanel extends ImmutableComponent {
this.onToggleAutofillData = this.onToggleSetting.bind(this, 'autofillData')
this.onToggleSavedSiteSettings = this.onToggleSetting.bind(this, 'savedSiteSettings')
this.onClear = this.onClear.bind(this)
this.getIsChecked = this.getIsChecked.bind(this)
this.getNewDefaultValues = this.getNewDefaultValues.bind(this)
}
getIsChecked (setting) {
const currentValue = this.props.clearBrowsingDataDetail.get(setting)
if (this.props.defaultValues && currentValue === undefined) {
return this.props.defaultValues.get(setting)
} else {
return currentValue
}
}
getNewDefaultValues () {
let newDefaultValues = new Immutable.Map()
let settings = ['browserHistory', 'downloadHistory', 'cachedImagesAndFiles', 'savedPasswords', 'allSiteCookies', 'autocompleteData', 'autofillData', 'savedSiteSettings']
// TODO: Optimize this to send back only defined settings
settings.forEach(function (setting) {
const currentValue = this.props.clearBrowsingDataDetail.get(setting)
newDefaultValues = newDefaultValues.set(setting, currentValue !== undefined ? currentValue : this.props.defaultValues.get(setting))
}.bind(this))
return newDefaultValues
}
onToggleSetting (setting, e) {
windowActions.setClearBrowsingDataDetail(this.props.clearBrowsingDataDetail.set(setting, e.target.value))

This comment has been minimized.

Sign in to view

Copy link
@bsclifton

bsclifton Nov 6, 2016 •
edited
Loading

It looks like the values are already being saved... but instead of being saved into the AppStore (application wide settings), it's being saved for the window (lost when the window is closed). If you hit cancel, we currently reset the settings (unneeded):
https://github.com/brave/browser-laptop/blob/master/js/components/main.js#L609

This is the conflict you are seeing, where the values are being considered twice (props.defaultValue is yours vs props.clearBrowsingDataDetail which is the one being saved to window state).

You should be able to update the code to call your action instead (the proposed onClearBrowserData ), then you can delete the code for windowActions.setClearBrowsingDataDetail (and the handler in windowStore, WINDOW_SET_CLEAR_BROWSING_DATA_DETAIL) and also delete the onHideClearBrowsingDataPanel method in main.js. When the control is bound in main.js, you can then omit these properties (no longer needed):

clearBrowsingDataDetail={this.props.windowState.get('clearBrowsingDataDetail')}
onHide={this.onHideClearBrowsingDataPanel}
}
onClear () {
appActions.clearAppData(this.props.clearBrowsingDataDetail)
this.props.onHide()
let detail = this.props.clearBrowsingDataDetail
if (this.props.defaultValues) {
detail = this.getNewDefaultValues()
}
appActions.setClearBrowserDataDefaults(detail)
appActions.clearAppData(detail)
this.props.onHide()
if (detail.get('allSiteCookies') && detail.get('browserHistory') &&
detail.get('cachedImagesAndFiles')) {
ipc.send(messages.PREFS_RESTART)
Expand All @@ -42,14 +67,14 @@ class ClearBrowsingDataPanel extends ImmutableComponent {
<div className='clearBrowsingData' onClick={(e) => e.stopPropagation()}>
<div className='formSection clearBrowsingDataTitle' data-l10n-id='clearBrowsingData' />
<div className='formSection clearBrowsingDataOptions'>
<SwitchControl className='browserHistorySwitch' rightl10nId='browserHistory' checkedOn={this.props.clearBrowsingDataDetail.get('browserHistory')} onClick={this.onToggleBrowserHistory} />
<SwitchControl rightl10nId='downloadHistory' checkedOn={this.props.clearBrowsingDataDetail.get('downloadHistory')} onClick={this.onToggleDownloadHistory} />
<SwitchControl rightl10nId='cachedImagesAndFiles' checkedOn={this.props.clearBrowsingDataDetail.get('cachedImagesAndFiles')} onClick={this.onToggleCachedImagesAndFiles} />
<SwitchControl rightl10nId='savedPasswords' checkedOn={this.props.clearBrowsingDataDetail.get('savedPasswords')} onClick={this.onToggleSavedPasswords} />
<SwitchControl rightl10nId='allSiteCookies' checkedOn={this.props.clearBrowsingDataDetail.get('allSiteCookies')} onClick={this.onToggleAllSiteCookies} />
<SwitchControl className='autocompleteDataSwitch' rightl10nId='autocompleteData' checkedOn={this.props.clearBrowsingDataDetail.get('autocompleteData')} onClick={this.onToggleAutocompleteData} />
<SwitchControl className='autofillDataSwitch' rightl10nId='autofillData' checkedOn={this.props.clearBrowsingDataDetail.get('autofillData')} onClick={this.onToggleAutofillData} />
<SwitchControl className='siteSettingsSwitch' rightl10nId='savedSiteSettings' checkedOn={this.props.clearBrowsingDataDetail.get('savedSiteSettings')} onClick={this.onToggleSavedSiteSettings} />
<SwitchControl className='browserHistorySwitch' rightl10nId='browserHistory' checkedOn={this.getIsChecked('browserHistory')} onClick={this.onToggleBrowserHistory} />
<SwitchControl rightl10nId='downloadHistory' checkedOn={this.getIsChecked('downloadHistory')} onClick={this.onToggleDownloadHistory} />
<SwitchControl rightl10nId='cachedImagesAndFiles' checkedOn={this.getIsChecked('cachedImagesAndFiles')} onClick={this.onToggleCachedImagesAndFiles} />
<SwitchControl rightl10nId='savedPasswords' checkedOn={this.getIsChecked('savedPasswords')} onClick={this.onToggleSavedPasswords} />
<SwitchControl rightl10nId='allSiteCookies' checkedOn={this.getIsChecked('allSiteCookies')} onClick={this.onToggleAllSiteCookies} />
<SwitchControl className='autocompleteDataSwitch' rightl10nId='autocompleteData' checkedOn={this.getIsChecked('autocompleteData')} onClick={this.onToggleAutocompleteData} />
<SwitchControl className='autofillDataSwitch' rightl10nId='autofillData' checkedOn={this.getIsChecked('autofillData')} onClick={this.onToggleAutofillData} />
<SwitchControl className='siteSettingsSwitch' rightl10nId='savedSiteSettings' checkedOn={this.getIsChecked('savedSiteSettings')} onClick={this.onToggleSavedSiteSettings} />
</div>
<div className='formSection clearBrowsingDataButtons'>
<Button l10nId='cancel' className='secondaryAltButton' onClick={this.props.onHide} />
Expand Down
3 changes: 2 additions & 1 deletion js/components/main.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -995,7 +995,8 @@ class Main extends ImmutableComponent {
clearBrowsingDataPanelIsVisible
? <ClearBrowsingDataPanel
clearBrowsingDataDetail={this.props.windowState.get('clearBrowsingDataDetail')}
onHide={this.onHideClearBrowsingDataPanel} />
onHide={this.onHideClearBrowsingDataPanel}
defaultValues={this.props.appState.get('clearBrowsingDataDefaults')} />
: null
}
{
Expand Down
3 changes: 2 additions & 1 deletion js/constants/appConstants.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ const AppConstants = {
APP_UPDATE_ADBLOCK_CUSTOM_RULES: _,
APP_SUBMIT_FEEDBACK: _,
APP_DEFAULT_BROWSER_UPDATED: _,
APP_DEFAULT_BROWSER_CHECK_COMPLETE: _
APP_DEFAULT_BROWSER_CHECK_COMPLETE: _,
APP_UPDATE_CLEAR_BROWSER_DATA_DEFAULTS: _
}

module.exports = mapValuesByKeys(AppConstants)
3 changes: 3 additions & 0 deletions js/stores/appStore.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -773,6 +773,9 @@ const handleAppAction = (action) => {
case AppConstants.APP_DEFAULT_BROWSER_CHECK_COMPLETE:
appState = appState.set('defaultBrowserCheckComplete', {})
break
case AppConstants.APP_UPDATE_CLEAR_BROWSER_DATA_DEFAULTS:

This comment has been minimized.

Sign in to view

Copy link
@bsclifton

bsclifton Nov 6, 2016 •
edited
Loading

Like the above comment about reusing the existing clearAppData action, this logic can be moved under the existing case AppConstants.APP_CLEAR_DATA: (which maybe we can rename as APP_ON_CLEAR_BROWSER_DATA and rename the action as onClearBrowserData)
appState = appState.set('clearBrowsingDataDefaults', action.defaultValues)
break
case WindowConstants.WINDOW_SET_FAVICON:
appState = appState.set('sites', siteUtil.updateSiteFavicon(appState.get('sites'), action.frameProps.get('location'), action.favicon))
appState = aboutNewTabState.updateSiteFavicon(appState, action)
Expand Down

0 comments on commit 97537e3

Please sign in to comment.