browsh
browsh copied to clipboard
browsh-org
Vim mode
Relates to #31
This still needs a lot of work, but I want to start a PR anyway for somewhere to have the discussion.
@tobimensch I've had a play with this and it's AMAZING! Hats off to you, this can't have been easy - you've made changes right across the spectrum from the CLI to the webextension code.
The main thing that needs to be done is fixing the tests that this breaks and adding new tests. I've already added a new test file specifically for Vim tests. Following links by link hints doesn't seem to be consistently passing its test - should the first link always be hinted with a? Also gt doesn't work for me. There may be other things that don't work, I didn't comprehensively go through everything. Which reminds me, we can start a new docs page for this, just have it hidden on the main site for now.
I'll leave my other feedback as comments on the PR's diffs.
@tombh Thanks.
About the failing tests. Of the 4 tests that tend to fail because of my changes 3 are failing because of the change I made to frame_builder.go. This is a change of behavior, as the input box positioning has slightly changed, which explains the failing tests, that continue to expect old positions.
The other failing test might actually be due to a preexisting bug in our tests. Ctrl+L gets executed one too many times and because it's a toggle the input focus is taken away from the url bar and therefore some key presses are forwarded to vim handling, that shouldn't be, resulting in the failing test. If you execute this test by itself it passes (hinting at the bug possibly not being on the program side), if you execute it together with the other tests, it fails. (Because of the ctrl+l toggling twice and the focus being taken way from the url bar)
So, I essentially know what the issues are, it seems not to be anything major.
About gt not working. Well, that keybinding is simply K (vimium has gt and K per default, we only have one binding, but it's easy to add a second one in config.go). You can see the set of keybindings in config.go. With the exception of gi, all of them have an implementation. Scrolling is known not to work for the find feature.
@tombh The link hint keys are intended to be user configurable. There's a default setting, and as of now it's not configurable. When there are very few links on a site, we don't need to generate two letter hints, therefore a link hint like a, e, d is enough in that case. On most sites two letter link hints will be generated, because there are a lot more links than link hint keys. Vimium does the same thing. So if you want to test the link hinting you have to test against a static site (same number of links and link positions) with the same/default link hint keys. It should be repeatable.
Thanks for the comments. I've fixed the tests now (at least locally). The only things that need to be done are adding a few comments and fleshing out the tests.
The more I go through this the more I realise how epic this PR is and how great this feature is going to be! Thanks so much @tobimensch
@tombh Thank you.
Btw. the init function you removed had a purpose. Functions named init get called automatically at startup in golang. This helps to prepare link hint texts. To generate them every time a user requests link hints would be a waste of resources as they stay the same (unless the linkHintKeys variable is changed, then it should be regenerated, this variable isn't configurable yet and typically wouldn't be changed on the fly) almost all the time.
Oh I didn't know that about init()! I put it in TTYStart() instead then, just so all the init is in one place.
Oh I didn't know that about
init()! I put it inTTYStart()instead then, just so all the init is in one place.
I just tested your changes. Putting setupLinkHints into TTYStart doesn't work here, I get an index out of range error when I hit f or F. Putting it into init() again fixes that. I'm not sure what's the reason for that, but assuming we're both using very similar Linux systems and go versions I wonder why this is working for you and not for me.
I found some bugs that I'm having trouble wrapping my head around. When I open a new tab and enter brow.sh the page never loads correctly and stays completely white. As start page brow.sh works, but not in a new tab, not sure if this has anything to do with vim-mode-experimental.
@tombh Pushed a commit for a pretty annoying bug where key combinations did only work consistently after a certain number of them were entered.
Hmm, that's an interesting bug. I don't see it on my machine. Which makes me think that the only possible difference is that I've recently run dep ensure and npm update. So maybe your Go and JS deps are out of sync?
Interestingly though, I'm really struggling with Travis at the moment, the tests are just freezing half way through, which could possible be caused by the same thing your seeing.
@tombh Which bug did you refer to? The brow.sh not showing up correctly in a new page one or the index out of range error without the init() function one?
@tombh I did some rebasing/merging and that seems to have changed your two latest commits (not their content, they're now listed as being authored by you and commited by me). Feel free to revert this, if you think it is bad. And I pushed this commit: 68823278a4abbceabf14c7f5529c61c7a76c9ea4 , that allows for using the ESC key for leaving input boxes. Now you can fill out input forms and use login fields without having to use the mouse to switch from one input box to the next.
I'm looking at writing some tests for the link hinting. I'd like to test the multi character link hints, which I think will require a new test web page. Is the preference to add a new folder within the /test/sites folder and add css/html in there, or should I add a new HTML file within the existing /test/sites/smorgasbord folder?
Thanks Jared :) Either is fine. I guess I'd lean towards adding a new folder for those tests.
What would be the best way for me to contribute to this PR? I don't have write access to this branch, but I could submit a separate PR to this branch, which if it was accepted would probably affect this PR. Any advice?
Any plan on integrating this any time soon? Just tried browsh and really like the idea but current keybinds wouldn't even work on tmux on my Mac (cmd+l did nothing) so I'll keep using w3m (which have acceptable vi-bindings) as my in terminal browser for now when doing quick lookups. Another prime example of a great vi-mode browser is qtbrowser.
The biggest thing stopping this being merged is the failing tests. Travis is just hanging so it's hard to debug. I've got some WIP code locally overhauling the test setup code, but I'm just really slow at working on it :(
Link hint generation is broken. e.g. "fa" and "f" will be marked as a flags. Thus trying to type "fa" causes "f" to be selected. Vimium does this by generating link flags on the fly https://github.com/philc/vimium/blob/881a6fdc3644f55fc02ad56454203f654cc76618/content_scripts/link_hints.coffee#L449
While currently in this implementation, the flags just come from a prepopulated list.
I might grab this myself, just documenting somewhere.
Hello lovely people. In my opinion this is the current killer feature for brow.sh. If this PR got merged even if some features are missing I have no doubt it would drive adoption of the app.
Hey Lovely Commenter. I'm so sorry that I haven't had time to look at this. It's been so long now that I've looked at the code, it would take me weeks to get back up to speed. And I just have too much to do at my my day job :/