-
-
Notifications
You must be signed in to change notification settings - Fork 419
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
Vim mode #236
base: master
Are you sure you want to change the base?
Vim mode #236
Conversation
@tombh 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 |
@@ -139,34 +147,66 @@ func isMultiLineEnter(ev *tcell.EventKey) bool { | |||
return activeInputBox.isMultiLine() && ev.Key() == 13 && ev.Modifiers() != 4 | |||
} | |||
|
|||
func handleScrolling(ev *tcell.EventKey) { | |||
func generateLeftClickYHack(x, y int, yHack bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just remind me of this hack again? Is it to do with doubling the y-coord? We should either fix this properly now, or have a good reason not to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically the issue is that browsh partitions the screen into a number of columns and rows (grid) and that these rows times columns areas are the only places that can be clicked with the mouse per default. Sometimes user elements (like an input field, clickable image or the like) in the browser backend aren't placed exactly in the middle of a row/cell pair, but instead in the middle of two neighbouring cells. This hack allows to generate a click in between them and therefore increases the click resolution so to speak, so maybe this shouldn't even be called hack and another name instead. But looking at the code it definitely looks hackish, so the name came to me easily. The reason I created this was that I had input fields that weren't properly aligned in the grid and therefore it wasn't straightforward to translate clicks from the link hint data (generated by the backend and sent to the tty for processing) that has a higher resolution (pixels) so to speak into clicks on the tty (with the lower tty resolution). I think this will prove useful again, even though it looks unpleasant.
Looking at the code I realize that this functionality actually is in use for exactly the use case I describe above. It makes misclicking less likely in some situations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right I understand now, so the hack just allows you to click in the middle of a TTY cell, rather than at the top right? So is there a reason to not actually just make this the default? Considering the code I've already written I think defaulting to clicking in the middle will be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tombh
It allows to generate a click, that's between two cells (the cell the click is generated for and the cell directly below(y) it), and that's what I referred to as the middle. From the top of my head I can't tell you why this shouldn't be the default, but I feel it'd be good to have more control over where generated clicks in the tty go when they're received in the backend. The terminal mouse has a low resolution anyway, so picking the middle of the cell seems reasonable for this translation.
Defaulting to the middle might indeed be fine, but I needed this YHack only to fix a corner case, so hopefully we'll not run into new corner cases where we need the old version of the code again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you remember what the corner case is then we can write a test and then hopefully any problems arising for switching the default to this new behaviour will show up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh and do you remember why 2 mouse events with a pause in between are sent for each click?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tombh
One is for pressing the button, and one is for releasing it. Generating a single click. The pause is there to make it appear human-like. I tested different setups, and this one seemed to work by far the best, while it allowed me to reuse existing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes of course!
@@ -0,0 +1,1249 @@ | |||
// The code in this file was copied and adapted from vimium's codebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you get the raw code here? Was there a build process?
As I mentioned before, ideally I don't want to be committing this to the main code base. If there's a way to independently build it and include as a dependency, that would be ideal. Perhaps there's even a PR we could send to Vimium to help us?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was all done manually by copying the code from the webinspector while vimium was running in Chrome, until I could do something useful with the code.
There are three main issues:
- vimium's code is originally written in coffeescript. Assuming we don't want to switch over to using coffeescript and assuming we don't want a codebase that mixes javascript and coffeescript I copied the compiled code.
- there doesn't exist a single file containing all relevant code (that we as browsh need) in vimium like the vimium.js file I created. The code is spread over multiple modules and not structured in a way that makes it easy to load in a 3rd party application. I think it's good to fetch only the relevant bits from vimium and not the 3/4ths we don't have much use for.
- I made some (mostly minor) modifications to the code I documented at the top of vimium.js
I agree it's not the ideal solution, maybe we can create a issue on the vimium github site asking for the best way to share this particular code among the code bases. Unfortunately nobody answered my last question/issue I created there, so I went ahead anyway with the intention of making it work.
It should be possible to write a script(bash/python or the like) to recreate vimium.js. A lot of special case handling would be required (I took some liberty in splicing everything together) and we would have to maintain a patch set.
Depending on how much more functionality we want to use from vimium we could need to import additional code. To make the gi(focus first input) feature work there are some vimium bits we're still missing. Sometimes we can implement stuff our own way, I focused on importing vimium code for features that are most difficult to get right(i.e. link hinting).
Finally I ran the code through prettier in order to make Travis happy. Unfortunately this makes the code even less closely resemble the compiled coffeescript code. I think it also makes the line numbers I mention in the top of vimium.js somewhat wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you copied it from the inspector! Ha, ok nice. Well then what about just renaming vimium.js
to link_hinting.js
? It doesn't seem so necessary to have a formal dependency mechanism if we're just pulling out a quarter of the code. We should still of course reference the source of the code. And we can also mention at the top of the file that we would still ideally like to formalise the mechanism by which the code is included. Like you say, that would just be some scripts that convert the Coffee Script to JS at build time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tombh
I disagree about renaming it to link_hinting.js. It contains more than just the bits for link hinting, and I think vimium.js will grow a bit more and contain even more bits of vimium code, for example for input box selection. Link hinting is just an example of something not worth recreating the wheel for (lot's of work!). Adding more comments at the top is always a good idea.
Last but not least I strongly feel we are showing the vimium project our thankfulness by naming the file this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, no problem. Ok, I'll have a quick go at importing with some build scripts. First thing though is to flesh out the tests so that any changes we make aren't going to break a bunch of the Vim features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tombh
It will be especially difficult to import the MiscVimium() function, because I created it. The code in there is for initializing stuff that other vimium code expects. It was put together from different vimium parts and also contains some modifications. This is the only part that you won't see in similar form in vimium.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think MiscVimium
would be a good candidate for just including in the main Browsh code though. It's not very big. BTW I don't understand what any of that function does. It'd be good to have a few comments. That's another good reason for depending on the original Vimium Coffee Script, it already comes with some comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tombh
The Non-MiscVimium-Vimium code was complaining about missing global objects and the like, so I had to figure out what those were and initialized them in MiscVimium (with code I took from vimium). I would advise against putting it into main Browsh code, as it has no relation to any core Browsh logic, and would look very out of place. I should've put more comments into MiscVimium though, this is a obscure function to the uninitiated! Good point about the comments not being included, maybe there's an option to compile CoffeeScript with intact comments. The missing comments didn't bother me much. Using coffee script for compiling our own vimium.js makes sense, but I do think the vimium.js should be precompiled in the repository and coffeescript shouldn't be required part of the toolchain for every Browsh developer or packager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created an issue at the github site of the vimium project about using their code in another application:
philc/vimium#3133
So far there is no response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this already merged pull request correctly, this makes it possible to keep all types of comments in the transpiled coffescript code.
jashkenas/coffeescript#4572
"github.com/gdamore/tcell" | ||
) | ||
|
||
// TODO: A little description as to the respective responsibilties of this code versus the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tobimensch could you answer the TODO briefly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mode based keyboard navigation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was somewhat answered by the previous comment. This file is only the link hinting code right? So the JS just just generates the position and dimensions of the link hints and then the Go code actually places the link hints on the screen. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree about the word "only". Getting the link hint positions is the hardest part and therefore we're lucky we can use vimium code for it and don't have to reinvent the wheel. And I disagree about it for a second reason, because vimium.js contains a lot of useful code besides link hinting, that I strongly believe we're going to use. For example I'm using vimium.js also for going to the next/previous page (this isn't the same as history, try [[ and ]] to see how it works) on websites like Google, which requires finding the correct links in the page, but is separate from the link hinting code. So vimium.js is an integral part of the whole keyboard navigation and not only link hinting.
You're right that we receive the link positions in the go code and display it directly in the terminal, contrary to how it's done in the original vimium. I think this gives us the best of both worlds.
The management of modes and disposing of actions based on the current mode is all done in go code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've got a better idea of what's going on now. It's definitely more than just link hinting.
interfacer/src/browsh/vim_mode.go
Outdated
} | ||
} | ||
|
||
func searchScreenForText(text string) []Coordinate { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also scroll to the currently found text hint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was my first implementation of link clicking by typing in and searching for text on the screen. It works, too. But it's not enabled by default, so my plan was to allow for this with some alternative key combination or making it a configurable link selection mode replacing the normal mode. It's possible to click all text with this, not just links, which might enable a few additional use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this shouldn't scroll. This is intended to search for text in the currently visible screen only, hence the name searchScreenForText. This makes it very fast/efficient as no communication with the backend is required in this process. However in the future this could be combined automatically/optionally with the find (/) feature. I.e. when the typed text isn't found on the screen a search for it could be initiated over the whole page (using the same code as the find (/) feature), it would scroll there if scrolling was fixed for the find feature (we talked about this before), and finally searchScreenForText could be called again in order to click on the text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, no problem. I just added the word visible
to the function name.
So anyway, my point was, that when I use this feature the screen doesn't scroll when focussing matches outside the screen. Is that intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tombh
I'm not sure we're talking about the same thing here. The function you reference wasn't enabled in the current code and got replaced by the vimium link hinting based navigation. Unless you changed some code to make this functionality active again, you can't possibly have tried it. But you definitely should give it a go(pun intended), since I believe this way of navigation/clicking might even be preferred by some people, or work better in some situations.
The find feature (activated by the / key like in vim) is something completely different and is implemented in the webextension code. searchScreenForText and / have nothing to do with each other at the moment, but they could be combined in the future (what I was talking about in the last comment).
The find feature (/ key) can't scroll at the moment because of the scrolling not synchronizing issue I wrote you about. The window.find method is used to implement it in the webextension and this method handles the scrolling, too. The problem is that browsh ignores/reverts the scrolling that is being done by window.find.
To be honest I dislike your choice of adding visible to the name of searchScreenForText, because I can't imagine a logical scenario where someone would search an invisible screen. You can only search what's on the screen, and that's visible, or otherwise it wouldn't be on the screen. The word screen refers to the terminal here, not the screen of the browser backend.
I now realize I might have created the idea of using the term "visible screen" myself by using it in my last comment. It nevertheless makes little sense to me. The screen is the active view.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, understood.
So what about providing a setting for changing the navigation behaviour? So others can try it out, I mean give it a go :D Otherwise how do you feel about removing the unused code? We could leave a comment referencing the commit hash so future devs can find it again?
Regarding visible
; is not the default behaviour of search in a browser to search the whole document, including the invisible areas?
And where shall we make a note about the broken scrolling? Is that something I could fix now for this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tombh
We should provide a setting for it! It could also be used in parallel to link hinting. Ie. by simply binding it to a different key. I never intended for the code to be obsoleted, leave it up to the users to decide if it's useful.
The searchScreenForText function is used by findAndHighlightTextOnScreen, which was written for the purpose of link navigation by text based clicking. This isn't intended as the find text feature, which indeed should scroll and search the whole page. The find text feature (like vim / and also bound to /) is implemented correctly and the window.find method used in the webextension does scrolling by itself. Unfortunately browsh somehow overwrites that scrolling, therefore it doesn't really scroll (only for a few milliseconds in the webextension, then it is being reverted/overwritten). This could and should be fixed. You wrote to me about this, saying that we also have to solve this for pages that want to scroll to a #hash position, pages that scroll to comments and the like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this is bringing back memories. I can fix the scrolling. The only difficulty I think will be the potential for positive feedback loops. I'll need to place a scroll listener that will trigger for both internally automated scrolling (eg; anchor tags) and Browsh initiated scrolling. Browsh scrolls its own copy of the page and then tells the main web page to scroll, the main web page then receives a scroll trigger and tells Browsh of the new scroll position, which Browsh should honour. So either there needs to be a way to distinguish Browsh-intiated scrolling from internal scrolling, or the scrolling values have to be precisely identical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tombh
Makes sense. Fixing this will be a huge step forward. Thank you for looking into it.
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 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 |
switch currentVimMode { | ||
case linkMode: | ||
if (*r).Height == 2 { | ||
generateLeftClickYHack((*r).Left, (*r).Top, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the corner case for the yhack?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. A height of 2 actually implies a height of 1. This is a bit confusing I have to admit, look into parseJSONLinkHints to get some of the logic behind that. Basically we're getting rectangles for the total size of links and we're moving them into a vertical middle position if they're higher than one row, and we're also moving them a bit to the right if there's enough space in that direction. The only positions we need for displaying link hints are r.Top and r.Left, so those are the ones that are being adapted, while r.Height is unchanged.
When the height is 1 we want to ensure we really hit the input box and not the corner of it, because when the border of an input box is clicked, it doesn't actually get focused.
This is not only needed to make input boxes work though, I remember that I tested some websites where I hit on corner cases that were solved by this type of positioning in combination with the "YHack".
I'm glad you're creating test cases, because this will make things much easier going forward. I tested with links, navigation links and input boxes on multiple sites, among them google, osnews, lwn.net, bing. Maybe we can capture static versions of these complex sites to make our testcases with. Is this legal to do?
d745f21
to
134aa4b
Compare
a51db9c
to
15f541c
Compare
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 |
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 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 |
d034497
to
2bf920b
Compare
@tombh |
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working on adding some tests for link hinting.
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? |
@j-rewerts |
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 :( |
The keybindings are hardcoded for now, but this is going to change.
Added moveTabLeft and moveTabRight functions, which take a tab ID and try to move the tab as far right or left in the tabOrder as possible. Added previouslyVisitedTab function that switches to the previously selected tab.
with activating input boxes using link hinting.
Vim mode still needs a lot more tests
…to key combinations only working after a certain number of key strokes.
This is because Travis' logs had 2 problems. 1. it doesn't capture the entire log output 2. it doesn't show logs when there's a timeout
… event could get interpreted repeatedly. Also some rewrites/improvements of the code. Key mappings can now contain control characters and meta keys with a vim-like notation. There's a hard insert mode, which disables all of browsh's shortcuts and requires 4 hits on ESC to leave. There's a new multiple link opening feature analogous to vimium, that's still incomplete.
0c0b907
to
b2ade39
Compare
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 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 :/ |
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
? Alsogt
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.