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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow settings of launch args when using defaultProtocol #6858

Merged
merged 8 commits into from Aug 22, 2016

Conversation

Projects
None yet
4 participants
@MarshallOfSound
Member

MarshallOfSound commented Aug 16, 2016

Currently this is windows only as I don't know how to do the equivalent on macOS (I don't think it is actually needed).

Basically most people use Squirrel.Windows to install Electron applications on Windows. When a user updates the app the stored default path is now wrong and instead of having to update the path during the update process it would be great if we could use the Squirrel --processStart syntax as our launcher.

If someone can point me at the direction to do this on macOS would be much appreciated 馃憤

MarshallOfSound added some commits Aug 16, 2016

Allow client to specify EXE file and args to set as default handler
* Optional path param to setAsDefaultProtocolClient
* Optional args param to setAsDefaultProtocolClient
@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Aug 16, 2016

it would be great if we could use the Squirrel --processStart syntax as our launcher.

I think this issue might also exist for SetLoginItemSettings since that uses the exe path as well.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Aug 17, 2016

@kevinsawicki Quite possibly, I haven't ported over to that API. We still use node-auto-launch which I recently PR'ed into to allow setting a custom EXE path.

I'll take a look at SetLoginItemSettings and see if I can raise a similar PR to do the same thing (custom exe and args)

bool Browser::RemoveAsDefaultProtocolClient(const std::string& protocol) {
if (protocol.empty())
return false;
std::wstring protocolLaunchPath(std::string protocol, mate::Arguments* args) {

This comment has been minimized.

@zcbenz

zcbenz Aug 18, 2016

Contributor

Our convention for a function that may fail is returning a bool, like this:

bool GetProtocolLaunchPath(std::string protocol, mate::Arguments* args, std::wstring* exe)
std::wstring protocolLaunchPath(std::string protocol, mate::Arguments* args) {
// Read in optional exe path arg
std::wstring exePath;
std::string rawExePath;

This comment has been minimized.

@zcbenz

zcbenz Aug 18, 2016

Contributor

Should store the path in wstring or FilePath, otherwise unicode characters may fail to convert.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Aug 18, 2016

Member

Current native_mate doesn't have a converter for std::wstring. Do you know of a way that I don't to bring in a std::wstring from the arguments?

This comment has been minimized.

@enlight

enlight Aug 18, 2016

Contributor

Use base::FilePath instead of std::wstring when dealing with paths, and base::string16 for other stuff you may want to use a std::wstring for (at least for passing read-only strings to Win32 APIs).

}
// Read in optional args arg
std::vector<std::string> launchArgs;

This comment has been minimized.

@zcbenz

zcbenz Aug 18, 2016

Contributor

Should use wstring to avoid conversions.

// Parse launch args into a string of space spearated args
if (launchArgs.size() != 0) {
std::string launchArgString = "";
for (std::string launchArg : launchArgs) {

This comment has been minimized.

@zcbenz

zcbenz Aug 18, 2016

Contributor

There is base::JoinString to do the work.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Aug 18, 2016

Member

馃憤 I was looking for this but couldn't find it... 馃槩

}
std::wstring wLaunchArgString;
wLaunchArgString.assign(launchArgString.begin(), launchArgString.end());
exe = exe + L"\"" + wLaunchArgString + L"\"";

This comment has been minimized.

@zcbenz

zcbenz Aug 18, 2016

Contributor

Using base::StringPrintf would make the code more clear.

This method sets the current executable as the default handler for a protocol
(aka URI scheme). It allows you to integrate your app deeper into the operating
system. Once registered, all links with `your-protocol://` will be opened with
the current executable. The whole link, including protocol, will be passed to
your application as a parameter.
On windows you can provide optional parameters path, the path to your executable,

This comment has been minimized.

@zcbenz

zcbenz Aug 18, 2016

Contributor

windows => Windows.

@@ -448,18 +448,23 @@ bar, and on macOS you can visit it from dock menu.
Clears the recent documents list.
### `app.setAsDefaultProtocolClient(protocol)` _macOS_ _Windows_
### `app.setAsDefaultProtocolClient(protocol[, path, args])` _macOS_ _Windows_

This comment has been minimized.

@zcbenz

zcbenz Aug 18, 2016

Contributor

The docs of is and remove APIs should also be updated.

* `protocol` String - The name of your protocol, without `://`. If you want your
app to handle `electron://` links, call this method with `electron` as the
parameter.
* `path` String (optional) _Windows_ - Defaults to `process.execPath`
* `args` Array (options) _Windows_ - Defaults to an empty array

This comment has been minimized.

@zcbenz

zcbenz Aug 18, 2016

Contributor

options => optional.

MarshallOfSound added some commits Aug 18, 2016

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Aug 22, 2016

馃憤

@zcbenz zcbenz merged commit 8b9fd8a into master Aug 22, 2016

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-mas-x64 Build #2232 succeeded in 7 min 15 sec
Details
electron-osx-x64 Build #2237 succeeded in 7 min 17 sec
Details

@zcbenz zcbenz deleted the default-protocol-launch-args branch Aug 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment