Support custom launch path in app.setLoginItemSettings #8515
Conversation
| } | ||
| } | ||
|
|
||
| return settings; | ||
| } | ||
|
|
||
|
|
docs/api/app.md
Outdated
| @@ -778,7 +786,7 @@ Returns `Object`: | |||
| **Note:** This API has no effect on | |||
| [MAS builds][mas-builds]. | |||
|
|
|||
| ### `app.setLoginItemSettings(settings)` _macOS_ _Windows_ | |||
| ### `app.setLoginItemSettings(settings[, path, args])` _macOS_ _Windows_ | |||
There was a problem hiding this comment.
These new optional arguments feel like they could just be properties of the settings object, any feelings towards including them there vs. separate arguments?
There was a problem hiding this comment.
I initially tried to bolt them on but ran into issues with the Mate converter. Someone better than me at V8 could probably work it out.
There was a problem hiding this comment.
@CharlieHess I pushed up a commit to bolt them on, let me know what you think.
kevinsawicki
force-pushed
the
auto-launch-squirrel-support
branch
from
c96ca16
to
cad37de
Compare
kevinsawicki
force-pushed
the
auto-launch-squirrel-support
branch
from
cad37de
to
67d3f76
Compare
atom/browser/browser_win.cc
Outdated
| if (launch_args.empty()) { | ||
| base::string16 formatString = includeOriginalArg ? | ||
| L"\"%s\" \"%%1\"" : | ||
| L"\"%s\""; |
There was a problem hiding this comment.
@CharlieHess is there a reason the path needs to be quoted here for login item settings path?
I ask because I'm wondering if this breaks backwards compatibility for people that previously used setLoginItemSettings which did not quote the path value in the old implementation. After this change, if they called getLoginItemSettings the paths would not match because it is not expected the registry value to be quoted when in previous releases it did not quote it.
Thoughts?
There was a problem hiding this comment.
@kevinsawicki I think it's still consistent. It was quoted before:
*exe = base::StringPrintf(L"\"%s\" \"%%1\"", exe->c_str());The command line args were never quoted though:
L"\"%s\" %s \"%%1\""
L"\"%s\" %s"
There was a problem hiding this comment.
But before GetLoginItemSettings wasn't calling this though, it was just using PathService::Get(base::FILE_EXE, &path) and comparing the value directly to that.
There was a problem hiding this comment.
I think if you call GetLoginItemSettings with no arguments (as before), ReadAppCommandLine will just dish to PathService and the behavior is the same (otherwise our existing unit tests would be failing, right?). I'll confirm tomorrow.
There was a problem hiding this comment.
Hmm, I guess tests wouldn't cover backwards compatibility, though. 😞 I'll take a look.
There was a problem hiding this comment.
@kevinsawicki yeah after closer inspection it was definitely not backwards compatible. Good catch. I've reworked it to avoid quoting the path in setLoginItemSettings.
There was a problem hiding this comment.
Looks good 👍 Will merge once the CI completes.
Make all these arguments user-provided instead.
It's only necessary for the protocol launch path.
We need to make sure the executable path is not quoted.
kevinsawicki
force-pushed
the
auto-launch-squirrel-support
branch
from
57cd206
to
1b3d3b6
Compare
|
Great work on this @CharlieHess 👍 🚢 |
This PR fixes #6901. It leverages the fine work by @MarshallOfSound in #6858 to add support for custom executables and command-line arguments in
app.setLoginItemSettings. This allows it to properly support Squirrel.Windows, which needs to point to Update.exe instead ofprocess.execPath. I've updated the documentation to include an example.