search

zumoshi / BrowserSelect

Browser Select is a utility to dynamically select the browser you want instead of just having one default for all links.
GNU General Public License v2.0
289 stars 39 forks source link

matching on urls #68

Closed JaspalX closed 4 years ago

JaspalX commented 4 years ago

Have a look and let me know what you think. This meets my needs but ymmv of course.

This simply adds the url to the list of options on the 'always' button and allows you to add the full url as a rule. I wasn't quite sure what was going on with the various ambiguous rule bits of code so I just stuck the url onto the list for the context menu.

Caveats: I'm not sure how the button click works - once you've clicked 'always' you can't get open the page without adding a rule, which I find a bit of a pain.

JaspalX commented 4 years ago

Hi, all good points you made.  As it is, it 'scratches the itch' I had so tbh I'll probably carry on using as is. I was really sending the pull request as a prompt for discussion.

Couple of comments below...

On 04/07/2020 17:53:45, Borhan Hafez notifications@github.com wrote: @zumoshi commented on this pull request. Hi, First, thanks for your work. what was going on with the various ambiguous rule bits Basically tries to be smart and not annoy the user with a context menu for most cases. but asks for clarity in cases like these: something.com.au => .com.au or .something.com.au something.fun.ir => .fun.ir or .something.fun.ir (first is a tld, so *.com.au is an invalid and useless rule, second case .ir is a tld, making first one valid) once you've clicked 'always' you can't get open the page without adding a rule That's because open_url is called inside the add_rule [https://github.com/zumoshi/BrowserSelect/blob/405648a2989fa477dd1a107cfafac2a628ade8bb/BrowserSelect/Form1.cs#L116] method. There is nothing stopping you from calling it outside, but since the whole point of Always button is to add a rule I don't see why you would. add the full url as a rule Personally, I wouldn't do this, users usually expect a rule added without a layer of the URL to have an implicit wildcard for lower layers, but your implementation doesn't do this. e.g. a rule for https://forums.spacebattles.com/threads/whatever.853195/ should match https://forums.spacebattles.com/threads/whatever.853195/#post-67461289 Jaspal Sahota: Actually, what it is doing now is pretty much how I want it to work! Horses for courses I guess.

Also, many places don't put the protocol (i.e. http://) in the URL, (e.g. in telegram you can just write google.com and it would ba link), in that case, many programs (e.g. telegram again) default to opening the URL with http instead of https. So I don't see the benefit of requiring the users to either always put a wildcard in front of the URL or add two rules one for each protocol. And lastly, as I've also mentioned in the comments to the code itself, your DoesAutoRuleMatch method isn't backward compatible which would break the existing rules, this is the biggest dealbreaker for merging this commit as-is. The rest of my complaints are mostly good to have. But an update that forces users to edit every single rule they have isn't ideal. In BrowserSelect/Browser.cs [https://github.com/zumoshi/BrowserSelect/pull/68#discussion_r449784860]:

  • foreach (string Profile in ChromeProfiles) - { - browsers.Add(new Browser() - { - name = "Chrome (" + GetChromeProfileName(ChromeUserDataDir + "\" + Profile) + ")", - exec = BrowserChrome.exec, - icon = icon2String( IconExtractor.fromFile(ChromeUserDataDir + "\" + Profile + "\Google Profile.ico") ), - additionalArgs = String.Format("--profile-directory={0}", Profile) - }); - } - browsers.Remove(BrowserChrome); - browsers = browsers.OrderBy(x => x.name).ToList(); - } - } + //check for edge chromium profiles + AddChromeProfiles(browsers, "Microsoft Edge", @"Microsoft\Edge\User Data", "Edge Profile.ico"); These seem to be duplicated changes from your previous pull request. Did you perhaps forget to sync with the master branch before creating this pull request? (On your forked repo it says This branch is 2 commits ahead, 1 commit behind zumoshi:master., it shouldn't be behind when you create a pull request) Jaspal Sahota: Yeah, that's my git-noobness coming through

In BrowserSelect/Form1.cs [https://github.com/zumoshi/BrowserSelect/pull/68#discussion_r449785121]:

@@ -104,9 +105,14 @@ public void add_rule(Browser b) } else { - // in case pattern was not ambiguous, just set the pattern as rule and open the url - var pat = (_alwaysRule.mode == 1) ? _alwaysRule.tld_rule : _alwaysRule.second_rule; - add_rule(b, pat); + + _alwaysAsk = new ContextMenu((new[] { _alwaysRule.tld_rule, _alwaysRule.second_rule, _alwaysRule.url_rule }) + .Select(x => new MenuItem(x, (s, e) => add_rule(b, x))).ToArray()); + // display the context menu at mouse position + _alwaysAsk.Show(this, PointToClient(Cursor.Position)); + // // in case pattern was not ambiguous, just set the pattern as rule and open the url The point of ambiguity detection was to hide the context menu (prevent the need for clicking multiple times) if not needed. The context menu was added for issue #13 [https://github.com/zumoshi/BrowserSelect/issues/13]. Maybe make it show the context menu on right-click so the normal path of just adding the domain won't get longer requiring more clicks? In BrowserSelect/Form1.cs [https://github.com/zumoshi/BrowserSelect/pull/68#discussion_r449785276]: @@ -104,9 +105,14 @@ public void add_rule(Browser b) } else { - // in case pattern was not ambiguous, just set the pattern as rule and open the url - var pat = (_alwaysRule.mode == 1) ? _alwaysRule.tld_rule : _alwaysRule.second_rule; - add_rule(b, pat); + + _alwaysAsk = new ContextMenu((new[] { _alwaysRule.tld_rule, _alwaysRule.second_rule, _alwaysRule.url_rule }) Wouldn't this show invalid rules (e.g. *.com) or duplicated options if there was no ambiguity? Maybe keep the check and hide tld_rule or second_rule only showing the other one? [image] [https://user-images.githubusercontent.com/4184939/86516823-f836ac00-be38-11ea-9004-a9461052d98e.png] Jaspal Sahota: Yep, this is part of the 'I couldn't work out how the ambiguous rule bit works'  - it's a bit rubbish as is.

[https://user-images.githubusercontent.com/4184939/86516823-f836ac00-be38-11ea-9004-a9461052d98e.png]

[https://user-images.githubusercontent.com/4184939/86516823-f836ac00-be38-11ea-9004-a9461052d98e.png] In BrowserSelect/Program.cs [https://github.com/zumoshi/BrowserSelect/pull/68#discussion_r449785559]:

  • { + return true; + } + } catch (UriFormatException e) + { + // will probably get this with wildcard strings, ignore + } + // for the regex, replace every '' with '.' so the regex can understand it + // of course this means that urls with in them won't match very well, but + // assume that's not very common kind of url + + // tidy up the pattern for regex + string newpattern = Regex.Escape(pattern); + // unfortunately, this means that the wildcard that we wanted in the pattern + // has now been escaped to *, so let's reverse that + newpattern = newpattern.Replace(@"*", "."); What if the URL itself included ? For example http://google.com/search?q= which is a google search for , would match a google search for anything. There should be a way to differentiate from a literal (maybe from Always button) and a wildcard (entered by the user). Jaspal Sahota: On balance, I think it is worth suffering the risk of a url with a in it - I don't come across many urls with in them and having the wildcard (for me) is way more useful than catering for the -in-url scenario. As for ? I thought I'd excluded ? from the regex but may  not have done it and only thought about doing it ! - query strings certainly come up a lot but again, on balance using the regex search in a url is way more useful to me  than not.

In BrowserSelect/Program.cs [https://github.com/zumoshi/BrowserSelect/pull/68#discussion_r449786218]:

@@ -62,7 +63,7 @@ static void Main(string[] args) var browser = sr[1]; // matching the domain to pattern - if (DoesDomainMatchPattern(uri.Host, pattern)) If you don't use it here anymore, DoesDomainMatchPattern becomes dead code. Please remove any methods that aren't used anywhere in the codebase. In BrowserSelect/Program.cs [https://github.com/zumoshi/BrowserSelect/pull/68#discussion_r449786944]: @@ -146,7 +147,43 @@ public static string ProgramFilesx86() return Environment.GetEnvironmentVariable("ProgramFiles"); } + public static bool DoesAutoRuleMatch(Uri uri, string pattern) Your method is not backward compatible and would break rules that work with the current version. Either add an indicator that would separate host-only rules from protocol+host+path rules and use different methods, or make sure it works the same way for host-only rules as the latest released version. Example: [image] [https://user-images.githubusercontent.com/4184939/86516942-249ef800-be3a-11ea-9293-f96d0ed88c28.png] — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub [https://github.com/zumoshi/BrowserSelect/pull/68#pullrequestreview-442617939], or unsubscribe [https://github.com/notifications/unsubscribe-auth/AANQOL57UIXDI55V765SCBLRZ5NBRANCNFSM4OQOBZLA].

zumoshi commented 4 years ago

As it is, it 'scratches the itch' I had so tbh I'll probably carry on using as is.

That's fine, thanks for your contributions. The spirit of opensource is pretty much that, it's not a job to deliver fully polished code, just sharing what you implemented for personal use so others can use it is good enough.

Someone else may work off of what you've shared.