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

Support shortened URLs and straight to setting #77

Closed lucasnz closed 3 years ago

lucasnz commented 3 years ago

Three changes:

zumoshi commented 3 years ago

Hello, Thank you for your contribution. I'm not so sure about that last one though. Can you explain the rationale for not allowing access to the application's main window?

As for expanding links and resolving shortened URLs, I like what you're doing but I have some concerns.

lucasnz commented 3 years ago
  1. All redirects infinite loop. Is this a likely scenario? I guess it would be easy to add a maximum or even a user configurable option (instead of the "all redirects" or "first redirect" option).
  2. Are you suggesting a timeout and/or a "loading" screen? Or do you think we simply shouldn't be following redirects?
  3. This is essentially the same as point one. The reason I've allowed the first redirects to follow a 307 is I found if you give the http version of a url shortner, often the first then it does is a 307 redirect to the https site. The next redirect is usually the useful redirect. Having said this, some url shortners appear to have multiple redirects before hitting the site they are targeting.
  4. Yeah, I hadn't considered this angle, probably is best to leave certificate validation in place. In my testing I found some sites were untrusted which resulted in an app crash, probably better to handle the untrusted site (and not follow the link).
  5. I was concerned the redirect might go somewhere different. But I'm somewhat on the fence about this one. It could be a maintenance issue.
  6. Going direct settings. It just seemed unnecessary and untidy to launch the app and hard code google.com. When launching without any parameters, I thought the user would want to go straight to settings (that's what I've found myself doing). Just a suggestion - if you don't like it, I can pull that commit.

Before I do some updates, it would be good to clarify what you think the best behaviour and options should be - e.g. "loading" screen, configurable redirect etc. Let me know :)

zumoshi commented 3 years ago
  1. Likely? No, Since it's a bug on the website's end. But it's an edge case that chrome handles (by showing too many redirects error). We should handle it too. Chrome allows a maximum of 5 before giving up.
  2. I would prefer the application to open instantly (before resolving the URL) while attempting to resolve it in the background. If it succeeded it should change the URL in the title bar. You may display a spinner or just prepend [Resolving...] in the title with an increasing number of dots per second. This way instead of an explicit timeout the user can choose to wait or give up and open the unresolved URL immediately.
  3. My main problem with this was the loop despite choosing the first redirect option.
  4. A crash shouldn't happen. preferably just fail to resolve and show unresolved URL in that case. But do validate the certificate.
  5. Hmm. needs more testing, I would've put BrowserSelect/1.4.1 as User-agent but you have a point about sites redirecting to 403 or some other page suggesting the user should download chrome. Up to you if you want to keep spoofing it or add an option.
  6. In the current version that may make sense. but I had plans to put a Customize key that allows users to drag-and-drop sort browsers.

https://user-images.githubusercontent.com/4184939/108031845-c5979780-7046-11eb-9740-fad727b04b5d.mp4

Edit: Regarding point 2, also consider local links when internet access is not available. Not sure how timeout works in that case but the speed of seeing the app from click is a priority. Browser select should not get in the way/add delays. Although now that I think about it for URLs that match a rule to automatically open showing the default app may become problematic. Maybe you can add an extra button in case the rule matches an existing rule before resolving so it would follow that? Also not sure about how good the UI of browser select opening just to close immediately after resolved URL matched a rule is. I'm open to better ideas.

Also regarding the gif, If you have extra time to put into browserselect I can give you access to my half-done v2 branch attempts.

lucasnz commented 3 years ago

I like the gif! I'll drop the last commit to ensure that is supported. I am interested in helping. But lets bottom this one out first...

WRT loading BrowserSelect and displaying the "first" URL while following redirects, as you allude to, this doesn't make sense if it's hitting a rule. Perhaps the only way around this is to maintain a list of url shorteners (user customizable) and then only follow those redirects. I agree, we need a short timeout, and to display something useful to the user while we follow the redirects...

andrew-hill commented 3 years ago

Although now that I think about it for URLs that match a rule to automatically open showing the default app may become problematic. Maybe you can add an extra button in case the rule matches an existing rule before resolving so it would follow that? Also not sure about how good the UI of browser select opening just to close immediately after resolved URL matched a rule is. I'm open to better ideas.

I'm not 100% sure if the data on the following page is the situation being discussed in your redirections, but this page suggests most redirections should complete by ~260ms.

https://www.littledata.io/average/server-redirection-time

This might be awkward to add to the code, but it makes me think the right approach would be to hide the UI while assessing the redirection for up to say 150-250ms. It's not terribly long for a user (especially if you send the redirected URL to the browser so the user doesn't wait twice), and will hide the UI for a lot of redirects. For multi-redirects, you're going to hit this even for fast sites (4x redirects at 70ms each is 280ms). Personally I'd want the UI to show up after about 150-250ms so I at least know the URL has been clicked and is processing, even if it's about to auto-match, so if you're allowing redirects to be processed, well this is kind of on the user to deal with (i.e. they've chosen to enable this feature).

Another danger is the UI shows up and vanishes just as you're about to interact with it of course... so you end up clicking on something else unintentionally and frustratingly.

I think a configured list of URL shorteners might be very useful to keep some of this under control. For me, I'm going to send most standard redirecting URLs to my main browser anyway (no redirection testing required), but there's particular domains that can need to go a few different ways (mimecast protection, as per #85). If I had a lot of e.g. tinyurls and really wanted to control where they end up, I'm probably OK to wait the 250ms or so to handle redirection when I knowingly click one.

lucasnz commented 3 years ago

My intention has been to add a whitelist (ideally user configurable) of url shortners. But I haven't got around to it. I've been using it as is for some time now...

lucasnz commented 3 years ago

Good feedback. I've made the following updates:

lucasnz commented 3 years ago

I've added a UI for users to configure url shortners now. This PR is ready for review and possibly inclusion.

lucasnz commented 3 years ago

I'm closing this PR and opening a new one against a separate branch, otherwise all my changes get pulled into this PR