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

Resolves: non-option argument instead of --startup-url #227

Merged
merged 6 commits into from
Oct 8, 2018

Conversation

farhadmak
Copy link
Contributor

Resolves #205

@farhadmak
Copy link
Contributor Author

One issue I ran into was that tabs did not open up in order of command. They get out of sync from the browsh terminal and firefox

@j-rewerts
Copy link
Member

@farhadmak I think that's pre-existing. If you run FF and close one of the tabs, for example, the TTY side will be out of sync.

@j-rewerts
Copy link
Member

Looks like a binary file snuck in. Can you remove interfacer/src/debug?

@j-rewerts
Copy link
Member

Looks good. Thanks @farhadmak!

@tobimensch
Copy link
Collaborator

I'm going to test this later. Is it necessary to check for URL validity? What about special pages such as about:config, which admittedly Browsh doesn't support currently anyhow, or simple file paths such as /home/user/index.html, which currently also doesn't work, but is a requested feature. Or what about going to a bookmark/often visited site by searching for a part of the URL. I.e. I might want to start with:
browsh slash
and expect the history to get searched for the latest top level domain containing the string slash. This would happen to be www.slashdot.org in my case.

Thinking about it the URL validation will still make sense once we implement the features mentioned above, we'd need a way to distinguish valid URLs and other types of page requests.

I'm likely to merge this later. Thanks @farhadmak for implementing and @j-rewerts for commenting.

The issue of the tab order being wrong could perhaps be solved by adding a new command new_tabs instead of new_tab in the webextension or by letting the new_tab command accept multiple arguments.

@farhadmak
Copy link
Contributor Author

This PR will only work with links as the URL validity will search for “http://“ or related and web extension would open each link. This being said, I believe it would be very easy to implement once the special pages start to work.

Would you like me to take a look at adding a letting new_tab accept multiple commands in this pr, or make it a separate issue

@tobimensch
Copy link
Collaborator

@farhadmak
I thought of another use case. Just like you can use the URL bar as search bar already, this would be useful from the command line, too.

browsh "bash documentation" "top10 books of 2018"
This would open two tabs in google (or other preferred search engine) with the upper search strings.

But this doesn't have to be implemented right now, I'd accept your pull request with the current features. It'd be nice if you kept adding more features to Browsh later, though. :-)

If letting new_tab accept multiple arguments fixes the resulting tab order, this would be great. It's not required for me to accept your patch though.

@tobimensch
Copy link
Collaborator

@farhadmak
I just tested your changes and they're working.

I believe the URL/URI validation is too aggressive. It accepts http://www.google.com, yet not www.google.com and in my opinion it should also accept google.com.

I think you should simply extract the top level domain part and test if it's reachable (read: if a ping gets returned).

Let's say I start browsh with lwn.net/some/path/to/an/article, then your code should extract lwn.net as the top-level domain and ping it. If that succeeds, you've a URI/URL that can be opened and it should be.

You could keep the current validation as first level of testing, because it might be quicker in a lot of circumstances, and the pinging should be the second level of testing for strings that were not recognized as URLs in the first level of testing.

We're sometimes using the term URI and at other times URL. Maybe you should change validURL to validURI, since it's a derivative of the ParseRequestURI call's results.

I hope you're willing to refine this further. I'd be thrilled to see more pull requests from you.

I'm merging this, welcome to the Browsh development community.

@tobimensch tobimensch merged commit 6c9e4be into browsh-org:master Oct 8, 2018
@farhadmak
Copy link
Contributor Author

Sounds good, I'll push an update this coming week.

@farhadmak
Copy link
Contributor Author

@tobimensch
I've looked into this some more... turns out if I completely remove the URL validation, it gives us exactly what we want, if not more.

with browsh facebook.com it will open facebook.
with browsh "bash documentation" it will open a google search of the string.

So this means, with any non-flag arguments, it will open a new tab with the link, or google search if it is not a valid link. Is this what we want?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants