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

Shell openExternal can take optional callback (macOS) #7612

Closed
wants to merge 0 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@gabriel
Contributor

gabriel commented Oct 13, 2016

As discussed in #6889

Added an optional callback argument to openExternal which will perform it asynchronously.

electron.shell.openExternal('http://electron.atom.io', {}, function(err) {
  if (err) console.log('Callback error:', err)
  else console.log('Callback opened')
})

I only implemented the asynchronous behavior on macOS.. I would need some help on the other platforms?

@enlight

If you're not familiar with the other platforms don't worry about it, just get this working on macOS for now :)

Show outdated Hide outdated atom/common/platform_util_linux.cc
Show outdated Hide outdated atom/common/platform_util_win.cc
Show outdated Hide outdated docs/api/shell.md
Show outdated Hide outdated docs/api/shell.md
@gabriel

This comment has been minimized.

Show comment
Hide comment
@gabriel

gabriel Oct 14, 2016

Contributor

To help review, this is what the docs now look like:

2016-10-14 at 12 43 pm

Contributor

gabriel commented Oct 14, 2016

To help review, this is what the docs now look like:

2016-10-14 at 12 43 pm

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Oct 17, 2016

Contributor

IMHO, it should return a promise instead. All new async browser API's use promises.

Contributor

sindresorhus commented Oct 17, 2016

IMHO, it should return a promise instead. All new async browser API's use promises.

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Oct 17, 2016

Member

@sindresorhus I think for consistency our API's should stick with callbacks.

If at some point we want to use promises instead (I agree it's a much nicer / newer pattern) then we should move all API's across at once (possibly as part of 2.0 deprecation). Having half and half would be quite confusing.

Member

MarshallOfSound commented Oct 17, 2016

@sindresorhus I think for consistency our API's should stick with callbacks.

If at some point we want to use promises instead (I agree it's a much nicer / newer pattern) then we should move all API's across at once (possibly as part of 2.0 deprecation). Having half and half would be quite confusing.

@paulcbetts

This comment has been minimized.

Show comment
Hide comment
@paulcbetts

paulcbetts Oct 17, 2016

Contributor

Why is this useful to be asynchronous? Any OS where this call blocks for any amount of time is pretty broken imho

Contributor

paulcbetts commented Oct 17, 2016

Why is this useful to be asynchronous? Any OS where this call blocks for any amount of time is pretty broken imho

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Oct 17, 2016

Contributor

@MarshallOfSound That means that for each new async API added until 2.0, there will be that many more breaking API's for users if 2.0 switches to promises. Consistency is nice, but nothing docs cannot solve, and future-proofing is more important.

Contributor

sindresorhus commented Oct 17, 2016

@MarshallOfSound That means that for each new async API added until 2.0, there will be that many more breaking API's for users if 2.0 switches to promises. Consistency is nice, but nothing docs cannot solve, and future-proofing is more important.

@MarshallOfSound

This comment has been minimized.

Show comment
Hide comment
@MarshallOfSound

MarshallOfSound Oct 17, 2016

Member

For those not interested in the promise discussion, you can skip over this

That means that for each new async API added until 2.0, there will be that many more breaking API's for users if 2.0 switches to promises

We do not add new API's very regularly and I haven't seen an async one come through in a long time. IMO that isn't too big of an issue. Also I don't know if we could / should / will move to promises as it is one hell of a breaking change it was more of a "if we do it definitely shouldn't happen till then".

Consistency is nice, but nothing docs cannot solve, and future-proofing is more important.

This is just IMO but I would normally take consistency over partially adopting new technologies. Using a library that has a mixture of promises and callbacks requires constant doc lookups and adds extra hassle / time to what should be a simply experience. Again, IMO, but I'd much rather know that either Electron is promise driven or callback driven. Given that lots of Chromium's internals still use callbacks and wrapping those in promises would be time consuming and "temporary" code, I think we would be jumping on the Promise train is a bit too soon.

But this is a discussion for lots of people to have another time in another place if we want to discuss adopting promise logic.

Argh I typed too much

Member

MarshallOfSound commented Oct 17, 2016

For those not interested in the promise discussion, you can skip over this

That means that for each new async API added until 2.0, there will be that many more breaking API's for users if 2.0 switches to promises

We do not add new API's very regularly and I haven't seen an async one come through in a long time. IMO that isn't too big of an issue. Also I don't know if we could / should / will move to promises as it is one hell of a breaking change it was more of a "if we do it definitely shouldn't happen till then".

Consistency is nice, but nothing docs cannot solve, and future-proofing is more important.

This is just IMO but I would normally take consistency over partially adopting new technologies. Using a library that has a mixture of promises and callbacks requires constant doc lookups and adds extra hassle / time to what should be a simply experience. Again, IMO, but I'd much rather know that either Electron is promise driven or callback driven. Given that lots of Chromium's internals still use callbacks and wrapping those in promises would be time consuming and "temporary" code, I think we would be jumping on the Promise train is a bit too soon.

But this is a discussion for lots of people to have another time in another place if we want to discuss adopting promise logic.

Argh I typed too much

@paulcbetts

This comment has been minimized.

Show comment
Hide comment
@paulcbetts

paulcbetts Oct 17, 2016

Contributor

@sindresorhus I'm agreeing with you In Principle, but I think before we can add promises in, we need a generalized way to marshal C++ code to return a Promise (i.e. right now I don't even know how I'd return a resolved promise via V8). Then we can retrofit all of these APIs to make the callback optional, similar to how many node libs are doing it so that both patterns are supported

Contributor

paulcbetts commented Oct 17, 2016

@sindresorhus I'm agreeing with you In Principle, but I think before we can add promises in, we need a generalized way to marshal C++ code to return a Promise (i.e. right now I don't even know how I'd return a resolved promise via V8). Then we can retrofit all of these APIs to make the callback optional, similar to how many node libs are doing it so that both patterns are supported

@gabriel

This comment has been minimized.

Show comment
Hide comment
@gabriel

gabriel Oct 20, 2016

Contributor

@paulcbetts It's nice to have an async call, because opening an email client or remote mount or document or whatever can take a long time (especially if accessing something remote)... having the ability to open and then get a callback with the status of the open is useful in these cases. And it's nice to show progress or report on the failure to open, asynchronously.

The callback is optional and follows the same API as the file dialog APIs and is fully backward compatible.

Contributor

gabriel commented Oct 20, 2016

@paulcbetts It's nice to have an async call, because opening an email client or remote mount or document or whatever can take a long time (especially if accessing something remote)... having the ability to open and then get a callback with the status of the open is useful in these cases. And it's nice to show progress or report on the failure to open, asynchronously.

The callback is optional and follows the same API as the file dialog APIs and is fully backward compatible.

@gabriel

This comment has been minimized.

Show comment
Hide comment
@gabriel

gabriel Oct 21, 2016

Contributor

@enlight I think I covered all the requested changes, lmk if anything else needs tweaking, thx!

Contributor

gabriel commented Oct 21, 2016

@enlight I think I covered all the requested changes, lmk if anything else needs tweaking, thx!

Show outdated Hide outdated atom/common/platform_util_linux.cc
Show outdated Hide outdated atom/common/platform_util_win.cc
Show outdated Hide outdated atom/common/platform_util_win.cc

@zcbenz zcbenz self-assigned this Oct 25, 2016

@gabriel

This comment has been minimized.

Show comment
Hide comment
@gabriel

gabriel Oct 26, 2016

Contributor

I think I covered all the requested changes, lmk if anything else needs tweaking, thx!

Contributor

gabriel commented Oct 26, 2016

I think I covered all the requested changes, lmk if anything else needs tweaking, thx!

@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov Nov 1, 2016

Contributor

Would it be possible to keep node-style error-first callback signature to keep promisification simpler?
Error might signal that item is not found or something like that,

Contributor

YurySolovyov commented Nov 1, 2016

Would it be possible to keep node-style error-first callback signature to keep promisification simpler?
Error might signal that item is not found or something like that,

@gabriel

This comment has been minimized.

Show comment
Hide comment
@gabriel

gabriel Nov 2, 2016

Contributor

Yeah, we probably should follow that pattern. I couldn't find any of the current callbacks in the API passing an Error object as the first arg though, so not sure exactly how to do that, but I'll look into it.

Contributor

gabriel commented Nov 2, 2016

Yeah, we probably should follow that pattern. I couldn't find any of the current callbacks in the API passing an Error object as the first arg though, so not sure exactly how to do that, but I'll look into it.

@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@gabriel

This comment has been minimized.

Show comment
Hide comment
@gabriel

gabriel Nov 2, 2016

Contributor

I updated to use an error for the callback. I was also able to simplify the code a little bit since LSGetApplicationForURL is also thread safe.

I'm able to determine the underlying error message in some cases from the OSStatus returned from LSGetApplicationForURL.

So for example, running these examples would return the following error messages:

  electron.shell.openExternal('ooops', {}, function(err) {
    console.log('Should error with bad URL:', err)
  })

  electron.shell.openExternal('noappforscheme://oops', {}, function(err) {
    console.log('Should error with no application for URL:', err)
  })

  electron.shell.openExternal('file:///notfound', {}, function(err) {
    console.log('Should error with fail to open:', err)
  })
Should error with bad URL: Error: Invalid URL
    at Error (native)
    at App.<anonymous> (/Users/gabe/Projects/Electron/electron-quick-start/main.js:42:18)
    at emitTwo (events.js:111:20)
    at App.emit (events.js:191:7)

Should error with no application for URL: Error: No application in the Launch Services database matches the input criteria.
    at Error (native)

Should error with fail to open: Error: Failed to open (-43)
    at Error (native)

There is no error stack if the error happened asynchronously so I'm probably doing something wrong there?

This isn't very relevant but that last example, LSGetApplicationForURL returns -43 error code if file (via file://) isn't found, but I couldn't find it anywhere in CoreFoundation or LaunchServices source anywhere, so might be a Finder error code. (Not part of standard error codes via NSError or LaunchServices.)

Contributor

gabriel commented Nov 2, 2016

I updated to use an error for the callback. I was also able to simplify the code a little bit since LSGetApplicationForURL is also thread safe.

I'm able to determine the underlying error message in some cases from the OSStatus returned from LSGetApplicationForURL.

So for example, running these examples would return the following error messages:

  electron.shell.openExternal('ooops', {}, function(err) {
    console.log('Should error with bad URL:', err)
  })

  electron.shell.openExternal('noappforscheme://oops', {}, function(err) {
    console.log('Should error with no application for URL:', err)
  })

  electron.shell.openExternal('file:///notfound', {}, function(err) {
    console.log('Should error with fail to open:', err)
  })
Should error with bad URL: Error: Invalid URL
    at Error (native)
    at App.<anonymous> (/Users/gabe/Projects/Electron/electron-quick-start/main.js:42:18)
    at emitTwo (events.js:111:20)
    at App.emit (events.js:191:7)

Should error with no application for URL: Error: No application in the Launch Services database matches the input criteria.
    at Error (native)

Should error with fail to open: Error: Failed to open (-43)
    at Error (native)

There is no error stack if the error happened asynchronously so I'm probably doing something wrong there?

This isn't very relevant but that last example, LSGetApplicationForURL returns -43 error code if file (via file://) isn't found, but I couldn't find it anywhere in CoreFoundation or LaunchServices source anywhere, so might be a Finder error code. (Not part of standard error codes via NSError or LaunchServices.)

@gabriel

This comment has been minimized.

Show comment
Hide comment
@gabriel

gabriel Nov 7, 2016

Contributor

This might be ready to review again :)

Contributor

gabriel commented Nov 7, 2016

This might be ready to review again :)

@gabriel

This comment has been minimized.

Show comment
Hide comment
@gabriel

gabriel Nov 11, 2016

Contributor

Let me know if there is anything I can do to help move this along.

This PR should be relatively safe since it's added behavior and code for the callback scenario and doesn't alter anything with the existing non-callback behavior.

Contributor

gabriel commented Nov 11, 2016

Let me know if there is anything I can do to help move this along.

This PR should be relatively safe since it's added behavior and code for the callback scenario and doesn't alter anything with the existing non-callback behavior.

@zcbenz

This comment has been minimized.

Show comment
Hide comment
@zcbenz

zcbenz Nov 17, 2016

Contributor

👍

Contributor

zcbenz commented Nov 17, 2016

👍

@zcbenz zcbenz closed this Nov 17, 2016

@zcbenz

This comment has been minimized.

Show comment
Hide comment
@zcbenz

zcbenz Nov 17, 2016

Contributor

Sorry I messed up when trying to rebase this branch, it has been pushed to master even though this PR shows as closed.

Contributor

zcbenz commented Nov 17, 2016

Sorry I messed up when trying to rebase this branch, it has been pushed to master even though this PR shows as closed.

@vivekdaramwalwy

This comment has been minimized.

Show comment
Hide comment
@vivekdaramwalwy

vivekdaramwalwy May 3, 2017

I need this asynchronous behavior for Windows as well. When I use openExternal() or openItem(), app hangs on Windows. Any solution?

vivekdaramwalwy commented May 3, 2017

I need this asynchronous behavior for Windows as well. When I use openExternal() or openItem(), app hangs on Windows. Any solution?

@YurySolovyov

This comment has been minimized.

Show comment
Hide comment
@YurySolovyov

YurySolovyov May 3, 2017

Contributor

Someone will probably need to implement one

Contributor

YurySolovyov commented May 3, 2017

Someone will probably need to implement one

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