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

prefs that require browser restart should show prompt for browser restart #2546

Closed
wants to merge 1 commit into from

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Jul 17, 2016

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Ran git rebase -i to squash commits if needed.

fix #2543

@@ -381,6 +383,26 @@ app.on('ready', () => {
app.quit()
})

ipcMain.on(messages.PREFS_RESTART, () => {
var message = 'Do you want to restart now ?'
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be translatable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 69bd164, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, and will you please include ? too?

@darkdh
Copy link
Member Author

darkdh commented Jul 17, 2016

Sure. Will do.

var message = locale.translation('prefsRestart')

appActions.showMessageBox({
buttons: ['Yes', 'No'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also be translatable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and also update button of password prompt in 94d28c7

@bbondy
Copy link
Member

bbondy commented Jul 18, 2016

I think it would be nice to remove the Requires a restart text from the affected preferences as well because the text is too long and repetitive that way. Maybe also say this:

This change requires a restart to come into effect. Would you want to restart now?

@bbondy
Copy link
Member

bbondy commented Jul 18, 2016

cc @bradleyrichter

@darkdh
Copy link
Member Author

darkdh commented Jul 18, 2016

I came up with one scenario, what if the user made change to the pref required restart and ignore the notification bar and keep changing other prefs. Then this user may not know which pref required restart specifically.

@bbondy
Copy link
Member

bbondy commented Jul 18, 2016

oh ok I see, we could do an electron MessageBox which is modal instead but we try to avoid those because it locks up everything completely. Will defer to Brad.

@@ -591,6 +592,10 @@ class AboutPreferences extends React.Component {
settings: this.state.settings.set(key, value)
})
aboutActions.changeSetting(key, value)
if (key === settings.DO_NOT_TRACK || key === settings.HARDWARE_ACCELERATION_ENABLED ||
key === settings.PDFJS_ENABLED) {
Copy link
Member

Choose a reason for hiding this comment

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

@bbondy was there a reason you added the restart requirement for PDFJS in e962ef9? it seems to work fine when i turn it on/off and don't restart

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how to reproduce but sometimes even having the option on gives a save prompt. It's something related to flipping pref though. It works reliably with a restart without the bug.

@diracdeltas
Copy link
Member

cherry-picked onto master, thanks

@luixxiul luixxiul modified the milestones: 0.11.2dev, 0.11.3dev Jul 21, 2016
message
})
prefsRestartCallbacks[message] = (buttonIndex, persist) => {
if (buttonIndex === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is sometimes being called before the pref is actually saved. if i have flash enabled and disable in prefs then click 'yes' to restart, it is still enabled when Brave relaunches.

@luixxiul luixxiul modified the milestones: 0.11.2dev, 0.11.3dev Jul 27, 2016
@darkdh darkdh deleted the 2543 branch August 4, 2016 15:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

prefs that require browser restart should show prompt for browser restart
4 participants