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

Off-by-one error when selecting text #1616

Closed
hakonrk opened this issue May 16, 2019 · 15 comments
Closed

Off-by-one error when selecting text #1616

hakonrk opened this issue May 16, 2019 · 15 comments

Comments

@hakonrk
Copy link

hakonrk commented May 16, 2019

After double-clicking a word to start a selection, and growing the selection by moving the mouse cursor beyond the end of the initially selected word, the selection grows to one character beyond the mouse cursor. Example:

foo[bar]=1

Let's say you want to select only foo[bar]. You start by double-clicking on foo, thereby highlighting only that word (using the default select_by_word_characters setting). Then you drag the mouse to the closing bracket to also include [bar] in the selection. The result is that the selection also includes the following = character. I don't see any way to select only foo[bar].

@hakonrk
Copy link
Author

hakonrk commented May 16, 2019

Just noticed that this could be a duplicate of #945.

@hakonrk
Copy link
Author

hakonrk commented May 16, 2019

Tried cherry-picking the four patches in martinetd's selection branch (#1007) onto the master branch. They had no effect on the issue I describe in this ticket.

@martinetd
Copy link
Contributor

What's your select_by_word_characters ? By default it includes =, so the equal sign is just part of the word for kitty.

@hakonrk
Copy link
Author

hakonrk commented May 16, 2019

It makes no difference. I tried removing the = from select_by_word_characters and the same thing happens. There are many ways to reproduce this bug. The easiest is using spaces:

foo   bar

Double-click foo, then drag the selection towards `bar. Observe that, while moving over the spaces, the selection is always one character too wide when you hover the mouse over the spaces (or any other non-word character for that matter).

@trygveaa
Copy link
Contributor

I'm also seeing this. I don't think it's a duplicate of #945 because it's not just that it selects the next word too early, but you can also get a selection which doesn't include whole words.

E.g. with the text some words, if you double-click on some and drag the mouse to the right, the selections will be some, then some w and at last some words. The selection some w should never have been active, it should go directly from some to some words.

@hakonrk
Copy link
Author

hakonrk commented May 16, 2019

Just wanted to clarify my answer to martinetd's question, quoted below:

What's your select_by_word_characters ? By default it includes =, so the equal sign is just part of the word for kitty.

When I said it did not matter what I put in select_by_word_characters, I was talking about how kitty behaved on the master branch. With the four patches on your selection branch cherry-picked to master, removing the = character from select_by_word_characters makes it possible to select foo[bar] without including the = character immediately following the closing bracket.

@hakonrk
Copy link
Author

hakonrk commented May 16, 2019

kovidgoyal, thanks for the patch. It changes the way selection works for this issue, but sadly it still doesn't make it possible to include an arbitrary number of non-word characters in the selection. Here's another example:

matrix[x][y]

Double-click on matrix, then try to grow the selection to matrix[x]. With patch 5f33d90, non-word characters never affect the selection, it does not grow until the mouse cursor reaches the next word character.

@kovidgoyal
Copy link
Owner

Well, yeah. Why would word selection allow you to select non-word
characters? If you want to select them, either use non-word selection
mode or add the characters you are interested in to the list of
characters consisting of word characters.

@hakonrk
Copy link
Author

hakonrk commented May 16, 2019

That's the way it works in xterm, gnome-terminal and konsole (maybe more, didn't bother to check others). It makes sense in my opinion, because when I need to copy a mix of word and non-word characters, I usually want whole words + one or more non-word characters following the word, as shown in my matrix example. Word selection doesn't mean the selection must start and end with word-characters. Also, why change the behavior compared to all(?) other terminal emulators?

@trygveaa
Copy link
Contributor

trygveaa commented May 16, 2019

Thanks for the patch. After thinking about it, I agree with @hakonrk though. It would be more practical to be able to control the inclusion of non-word characters, even though I said in my last comment that the selection should go directly from some to some words, having the selection some (with the space included) between would be nice.

@kovidgoyal
Copy link
Owner

OK, I dont see an issue with doing that.

@hakonrk
Copy link
Author

hakonrk commented May 16, 2019

I have played around with it a little bit, and here's a patch that I've applied on top of 5f33d90. It seems to make kitty do word selection the same as other terminal emulators, but it's barely been tested, and I don't claim to fully understand the code I'm modifying, so it might have some unintended side effects.

diff --git a/kitty/screen.c b/kitty/screen.c
index f5f68570..970c61f0 100644
--- a/kitty/screen.c
+++ b/kitty/screen.c
@@ -2013,7 +2013,10 @@ screen_selection_range_for_word(Screen *self, index_type x, index_type *y1, inde
     Line *line = visual_line_(self, *y1);
     *y2 = *y1;
 #define is_ok(x) (is_word_char((line->cpu_cells[x].ch)) || is_opt_word_char(line->cpu_cells[x].ch))
-    if (!is_ok(x)) return false;
+    if (!is_ok(x)) {
+        *e = x;
+        return true;
+    }
     start = x; end = x;
     while(true) {
         while(start > 0 && is_ok(start - 1)) start--;

@hakonrk
Copy link
Author

hakonrk commented May 16, 2019

Should probably also set the start value, *s = x, before returning.

@kovidgoyal
Copy link
Owner

Umm this is already implemented in master. 9849a69

@hakonrk
Copy link
Author

hakonrk commented May 17, 2019

Yes, I noticed that after I had posted it. :) Thanks again for fixing this! 👍

kovidgoyal added a commit that referenced this issue Feb 26, 2020
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

No branches or pull requests

4 participants