Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that default file/protocol handlers are re-installed after updating. #997

Merged
merged 1 commit into from
Sep 23, 2021

Conversation

bnjmnt4n
Copy link
Member

@bnjmnt4n bnjmnt4n commented Oct 2, 2016

Previously, they were only installed when the preference was changed. This caused the handlers to point to non-existing files after updates occurred and older versions were removed by Squirrel.

Closes #791, #911.

@bnjmnt4n
Copy link
Member Author

bnjmnt4n commented Oct 2, 2016

Also, it seems weird that state.js is in the renderer folder, when it is called by both the renderer and main processes. Also, I think migrations.js belongs in the main folder, as it is typically only called to migrate updates by the main process. Maybe we should check which process it is in to decide which code to call?

Copy link
Contributor

@dcposch dcposch left a comment

Choose a reason for hiding this comment

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

state.js should only be used from the renderer process. Migrations also run in the renderer process.

I like the idea of reinstalling handlers on every app update though!

Only caveat is that someone might get frustrated if they remove handlers via Control Panel / System Preferences and they keep coming back

// enabled them.
function installHandlers (saved) {
if (saved.prefs.isFileHandler) {
const handlers = require('../../main/handlers')
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you can do this from the renderer process.

You have to use ipcRenderer to tell the main process to reinstall the handlers.

@bnjmnt4n
Copy link
Member Author

bnjmnt4n commented Oct 3, 2016

@dcposch I believe that currently state.js is run on both renderer and main processes, as it is loaded in the main process in order to restore window bounds.

@feross
Copy link
Member

feross commented Oct 3, 2016

I believe that currently state.js is run on both renderer and main processes, as it is loaded in the main process in order to restore window bounds.

You're right. state.js is required in both processes at the moment. That came in with the bound restoration change. I added a change in #995 that makes migrations only run once in the renderer (for perf, and bug prevention). state.save() should never be called from the main process, or we'll have serious problems. Not sure what the cleanest solution is here.

@bnjmnt4n
Copy link
Member Author

bnjmnt4n commented Oct 3, 2016

@feross I'll try to rebase on master and change the code to account for #773, which I hadn't seen before doing this PR.

@bnjmnt4n
Copy link
Member Author

bnjmnt4n commented Oct 3, 2016

@feross for now I've just set up the IPC renderer to install the handlers after every update. I'll look into #773 in the future.

…ating.

Previously, they were only installed when the preference was changed.
This caused the handlers to point to non-existing files after updates
occurred and older versions were removed by Squirrel.

Closes #791, #911.
@bnjmnt4n
Copy link
Member Author

bnjmnt4n commented Oct 9, 2016

Any updates?

@stale
Copy link

stale bot commented May 10, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label May 10, 2018
@stale stale bot closed this May 17, 2018
@feross feross reopened this May 20, 2018
@stale stale bot removed the stale label May 20, 2018
@DiegoRBaquero DiegoRBaquero merged commit 0d512db into master Sep 23, 2021
@DiegoRBaquero DiegoRBaquero deleted the demoneaux/fix-handlers branch September 23, 2021 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

torrent handler registration broken? (windows)
4 participants