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

electron-browser/openerService.ts isn't needed #79453

Closed
jrieken opened this issue Aug 19, 2019 · 4 comments
Closed

electron-browser/openerService.ts isn't needed #79453

jrieken opened this issue Aug 19, 2019 · 4 comments
Assignees
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Aug 19, 2019

Commit ba69d8d adds another opener service so that it can take a dependency onto the IWindowService. That's not needed as the base opener service allows to register handler, see:

registerOpener(opener: IOpener): IDisposable;

Yet another opener service adds unnecessary code/bloat

@jrieken jrieken added this to the August 2019 milestone Aug 19, 2019
@jrieken jrieken closed this as completed Aug 19, 2019
@jrieken
Copy link
Member Author

jrieken commented Aug 19, 2019

Oh, open vs openExternal not nice but understood. I think it should still use the opener-logic tho

@jrieken jrieken reopened this Aug 19, 2019
@jrieken jrieken removed this from the August 2019 milestone Aug 19, 2019
@bpasero
Copy link
Member

bpasero commented Aug 19, 2019

@jrieken to be clear: instead of having a separate service, we allow to register an opener that takes care of the openExternal thingy for electron only from VSCode desktop?

@bpasero bpasero added this to the August 2019 milestone Aug 19, 2019
@jrieken
Copy link
Member Author

jrieken commented Aug 19, 2019

Yeah, well actually two issues

  • Generally, I think one opener service is better than two, esp since the former allows contributions.
  • Tho I think it's weird that IOpenerService has open and openExternal esp. since the implementation of open calls openExternal. As a consumer, how would I know what to call? Should consumers look at schemes first? Should openExternal maybe be internal and be used as fallback in case we cannot open something with the registered openers?

@bpasero
Copy link
Member

bpasero commented Aug 19, 2019

@jrieken yeah I can see to get the count to 2.

We can think about merging open and openExternal but for now I would prefer to preserve our existing behaviour as much as possible. After all, open is doing things different from openExternal (e.g. sometimes it runs a command or opens a file), so we cannot just merge them both into one unless we would introduce an option (which is similar to having 2 methods).

@bpasero bpasero closed this as completed Aug 20, 2019
bpasero added a commit that referenced this issue Aug 20, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants