Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

Support for Microsoft Edge. #4

Merged
merged 2 commits into from
Oct 16, 2015
Merged

Support for Microsoft Edge. #4

merged 2 commits into from
Oct 16, 2015

Conversation

masta
Copy link
Contributor

@masta masta commented Oct 15, 2015

BrowserSelector made my life a lot easier but I needed it to support Microsoft Edge so here's a pull request in case you're interested in this change.

@DanTup
Copy link
Owner

DanTup commented Oct 15, 2015

Thanks for this; I haven't updated my Desktop yet, so hadn't been able to test it.

Is the reason for these changes simply to allow using edge instead of microsoft-edge in the config file, or is there more to it? The [browsers] section already allows you to assign short names to browsers, so I don't think there's huge value in being able to use edge instead of microsoft-edge (in fact, I think it's slightly confusing having this "magic", because what's in the browser section doesn't match the command a user could use to launch the browser)?

Eg., can't you just do this:

[browsers]
edge = microsoft-edge

and not need any code changes?

@masta
Copy link
Contributor Author

masta commented Oct 15, 2015

I made a few tests before starting Visual Studio/forking.
The problem is the statement starting the process: Process.Start(preference.Browser.Location, url);
I never got it running with using microsoft-edge: as location. It unfortunately just opens an empty tab in Edge.
Maybe indeed a simpler change would also be possible to support Edge.
Do you have any idea?

@DanTup
Copy link
Owner

DanTup commented Oct 15, 2015

Sorry, I didn't see the colon in your command, I thought you were just launching microsoft-edge (url). Didn't realise Edge was weird like that :(

I guess it does need special handling; but maybe it can be done more generically, like a dictionary for aliases and a format string for the command?

var specialCommands = new Dictionary<string, string> {
    { "edge", "microsoft-edge:{0}" }
}

I don't know if there would be any other browsers that need special handling like this, I wonder if this is some new plan from MS to use protocol handlers for things (this might allow hyperlinks in one browser to target another, like the handling of mailto:!?)

@masta
Copy link
Contributor Author

masta commented Oct 15, 2015

No problem. Indeed I could have started my message with stating that Edge is 'weird' :-)
This is just because Edge is one of these modern apps (Windows 10 UWP app).
I don't know whether such a dictionary is required, but I agree that the configuration edge = edge is ugly. Maybe just edge could be given?
Or how would you set up the configuration if such a dictionary is used?

@DanTup
Copy link
Owner

DanTup commented Oct 15, 2015

Something like:

if (specialCommands.ContainsKey(preference.Browser.Name))
    Process.Start(string.Format(specialCommands[preference.Browser.Name], url));
else
    Process.Start(preference.Browser.Location, url)

?

@masta
Copy link
Contributor Author

masta commented Oct 15, 2015

I was asking about the configuration in the INI file. Would this be fine for you?

[browsers]
edge
chrome = C:\Program Files (x86)\Google\Chrome\Application\chrome.exe
ff = C:\Program Files (x86)\Mozilla Firefox\firefox.exe
ie = iexplore.exe

The code you suggest seems fine and like that it should work.

@DanTup
Copy link
Owner

DanTup commented Oct 15, 2015

Sorry, I understand what you mean now.

I think that would be fine, or even edge= (this seems to be how Mercurial handles built-in extensions, so possibly it's an existing convention).

@masta
Copy link
Contributor Author

masta commented Oct 16, 2015

I've updated the pull request with this change. Hope it's alright now.
An alternative would have been to handle it all in the INI file, like so:

[browsers]
edge = micrsoft-edge:{0}
chrome = C:\Program Files (x86)\Google\Chrome\Application\chrome.exe {0}
ff = C:\Program Files (x86)\Mozilla Firefox\firefox.exe {0}
ie = iexplore.exe {0}

But this would break existing configurations.

@DanTup
Copy link
Owner

DanTup commented Oct 16, 2015

This all looks fine to me. I like your last idea too (we could assume " {0}" on the end if the user didn't provide it in the console), but it would slightly complicate things because of spaces needing quoting.

I'll build and upload a new binary with these changes when I get a moment on the desktop.

Thanks!

DanTup added a commit that referenced this pull request Oct 16, 2015
Support for Microsoft Edge.
@DanTup DanTup merged commit d547f4a into DanTup:master Oct 16, 2015
@masta
Copy link
Contributor Author

masta commented Oct 16, 2015

Well thank you Danny for this great idea. Your application fixes a very long time problem for me and certainly a lot of others too.

@DanTup DanTup mentioned this pull request Mar 2, 2016
mrubli pushed a commit to mrubli/BrowserSelector that referenced this pull request May 5, 2023
ConfigService: Fix missing default case for [urls] section
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants