Skip to content

Conversation

@artur-shaik
Copy link
Contributor

@artur-shaik artur-shaik commented May 5, 2015

Added option to webview for selection enabled caret mode.
In status bar checking value of this option to identificate about it.

Added bindings: for toggle selection mode, <Ctrl+Space> drop
selection and keep selection mode enabled.

In webview added javascript snippet to position caret at top of the
viewport after caret enabling. This code mostly was taken from cVim sources. (Maybe should be moved to somewhere better place?)


This change is Reviewable

Allow using '0' for move caret to beginnig of the line.
Added option to webview for selection enabled caret mode.
In status bar checking value of this option to identificate about it.

Added bindings: <Space> for toggle selection mode, <Ctrl+Space> drop
selection and keep selection mode enabled.

In webview added javascript snippet to position caret at top of the
viewport after caret enabling. This code mostly was taken from cVim sources.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should probably be removed now, or replaced by a debug log command 😉

Added option to webview for selection enabled caret mode.
In status bar checking value of this option to identificate about it.

Added bindings: <Space> for toggle selection mode, <Ctrl+Space> drop
selection and keep selection mode enabled.

In webview added javascript snippet to position caret at top of the
viewport after caret enabling. This code mostly was taken from cVim sources.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nitpick: That should be self.caret_selection_active in the comment.

@The-Compiler
Copy link
Member

The javascript snippet is a good workaround! 😄

I suggest moving it to its own file (e.g. at qutebrowser/javascript/visual_position_caret.js or so), and then you can do evaluateJavaScript(utils.read_file('javascript/visual_position_caret.js')). Please also add a license header (from cVim I guess) to avoid issues when packaging (e.g. the debian packaging guidelines want a license header in every file).

@artur-shaik
Copy link
Contributor Author

Oops... I was a bit rushed.

About counts I think yes, it can be usefull, but have you any ideas how to use '0' for jumping to start of line?

@The-Compiler
Copy link
Member

I just looked at the cVim sources, and I think there's enough changed so we don't have to explicitely put their (MIT) license in. I suggest adding a comment with the usual GPL header (like in the other files), and then a comment mentioning some code was taken from https://github.com/1995eaton/chromium-vim/blob/master/content_scripts/visual.js which also makes it easier to find in the future.

Regarding binding of 0, that's currently not possible as it's handled as count, but I think it shouldn't be too hard to edit some stuff in qutebrowser/keyinput to allow it to be bound - the first place you'd have to look at is probably qutebrowser/keyinput/basekeyparser.py:_split_count() - maybe checking if count is 0, and if so, adding it to cmd_input instead of count will already be enough.

@artur-shaik
Copy link
Contributor Author

Ok. Thanks for info, everything noticed, will look at it... 😄

@artur-shaik
Copy link
Contributor Author

Ok, made some changes. I hope nothing was forgotten. 😄

@The-Compiler
Copy link
Member

Thanks and sorry for the delay! Merging this into the visual branch now, then I'll do a final code review and test and merge that to master.

One small change: I'll change statusbar.bg.caret_selection to statusbar.bg.caret-selection as qutebrowser uses - instead of _ in all other settings too.

The-Compiler added a commit that referenced this pull request May 11, 2015
@The-Compiler The-Compiler merged commit 012e124 into qutebrowser:visual May 11, 2015
@artur-shaik
Copy link
Contributor Author

artur-shaik commented May 12, 2015 via email

@The-Compiler
Copy link
Member

I noticed in d594798 you removed the v keybinding - was this intentional?

As people in the IRC channel seemed to like it, I think v should be readded for toggle-selection. What do you think?

I also did various cleanups to the sources, added eslint as a linter and wrote some tests (#668).

@artur-shaik
Copy link
Contributor Author

v key was deleted because of your suggestion. And you know, I found that I always try to enable selection mode with v key. So, may be people are right, it better to make v key for toggle-selection.

@The-Compiler
Copy link
Member

v key was deleted because of your suggestion. And you know, I found that I always try to enable selection mode with v key. So, may be people are right, it better to make v key for toggle-selection.

Okay, I'll re-add it then. Sorry for the confusion!

What about handling enter similiar to what tmux does? I'd like this workflow:

  • Press v to go to caret mode.
  • Press Space to start a selection.
  • Select stuff.
  • Press Enter to go to back to normal mode and yank the selected text.

I can add that if you agree.

Also, thanks for your patience! As you can see, I'm a perfectionist, so I'm sorry this takes some time until it's merged. After those changes, I have one or two places in the code I want to take a closer look at, and after that's done I'll finally merge. 😄

The-Compiler added a commit that referenced this pull request May 13, 2015
The-Compiler added a commit that referenced this pull request May 13, 2015
@artur-shaik
Copy link
Contributor Author

He he, this is nice to be perfectionist. And don't worry about that, I'm fine with it.

For me better workflow is:

  • Press c to go to caret;
  • Press v to start selection (As I said, I always instinctively try to toggle selection with v);
  • Select stuff;
  • Press y (Enter for me too far away 😄 , and not common) to yank and go to normal mode (Now it keeps selection after yank, but I found that I don't need to keep selection in most time, so it better to go to normal, and drop selection).

The-Compiler added a commit that referenced this pull request May 13, 2015
When trying to add a new binding with multiple values, the bindings were added
immediately and the next _is_new() check returned False because the command was
already bound.

With this change, the new bindings first get added to a temporary dict so
_is_new() returns the correct result.

See #653.
The-Compiler added a commit that referenced this pull request May 13, 2015
When trying to add a new binding with multiple values, the bindings were added
immediately and the next _is_new() check returned False because the command was
already bound.

With this change, the new bindings first get added to a temporary dict so
_is_new() returns the correct result.

See #653.
The-Compiler added a commit that referenced this pull request May 13, 2015
When trying to add a new binding with multiple values, the bindings were added
immediately and the next _is_new() check returned False because the command was
already bound.

With this change, the new bindings first get added to a temporary dict so
_is_new() returns the correct result.

See #653.
The-Compiler added a commit that referenced this pull request May 13, 2015
@The-Compiler
Copy link
Member

Okay! I changed :yank-selected to leave the mode by default, unless --stay is given, so now y/Y and Enter leave the mode.

I also fixed a bug which caused <Space> not to be bound correctly (only v) and cherry-picked it to the visual branch.

Can you please delete the [caret] section from your keys.conf, restart and confirm the workflow feels right to you? 😄

@artur-shaik
Copy link
Contributor Author

artur-shaik commented May 13, 2015 via email

@The-Compiler
Copy link
Member

It's now all merged, thank you again! 👍

@artur-shaik
Copy link
Contributor Author

artur-shaik commented May 19, 2015 via email

@The-Compiler
Copy link
Member

Have you seen #675? I wonder if that happened for you during testing as well...

And if you feel like and have some time, I'd still be glad about a spatial navigation mode 😄

@artur-shaik
Copy link
Contributor Author

artur-shaik commented May 19, 2015 via email

@demure
Copy link

demure commented Feb 12, 2017

So, I recently found this mode, but find it a very un-vim like. To select text in Caret Selection Mode, I must hold down shift. Might we consider adding the three vim visual block modes? These could 'lock' you into selecting without holding shift, and line/block modes are nice.

I also notice I can't use "/", "f", "F", "t", "T", or other vim fast navigation tricks in this mode. I think adding such bindings would make our caret mode quiet epic.

@The-Compiler
Copy link
Member

@demure Is this on QtWebEngine, where caret mode isn't implemented at all yet? 😉

@demure
Copy link

demure commented Feb 12, 2017

Ah, sorry. When I saw that #626 linked here, I assumed this was the current ticket to comment on.

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.

4 participants