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

Mouse selection should be based on horizontal center of characters #945

Closed
jscheid opened this issue Sep 10, 2018 · 20 comments
Closed

Mouse selection should be based on horizontal center of characters #945

jscheid opened this issue Sep 10, 2018 · 20 comments

Comments

@jscheid
Copy link

jscheid commented Sep 10, 2018

I'm using Kitty 0.12.1 on OS X 10.12.6.

When I use the mouse to select text moving from left to right, the right-most character appears to get selected as soon as the mouse X position is beyond the left edge of the character. It would be better if it gets selected only when the mouse is beyond the horizontal center of the character. The same goes for the other end of the selection

For what it's worth, that is how Terminal.app works.

Here is some gratuitous ASCII art:

 __        __
 \ \      / /
  \ \ /\ / / 
   \ V  V /  
    \_/\_/             
 ^    ^
 |    |
 |    +--------- It would be nice if W only got selected when mouse crosses this point
 |
 +------------ W currently gets selected as soon as mouse crosses
               this point (coming from left)

Let me know if this doesn't make sense and I'll make a screencast.

@kovidgoyal
Copy link
Owner

I dont understand how that would work. What if I entered a cell vertically instead of horizontally? Or if the selection is just starting and there is no history of movement to detect whether the selection is going form left to right or right to left.

@jscheid
Copy link
Author

jscheid commented Sep 10, 2018

Good question, I think something like this, in pseudo code:

if startRow < endRow or (startRow == endRow and start.x < end.x)
  startColumn = round(start.x - 0.5)
  endColumn = round(end.x + 0.5)
else
  startColumn = round(start.x + 0.5)
  endColumn = round(end.x - 0.5)
endif

As for "the selection is just starting" -- don't you wait anyway before going into selection mode until the mouse has moved a certain distance, so that you can distinguish clicks?

If you have Terminal.app around I'd suggest to set font size very large (72pt say) and play a little bit with it to see what I mean. If you don't, let me know and I'll figure out how to make a screencast GIF for you.

@jscheid
Copy link
Author

jscheid commented Sep 10, 2018

I think maybe a better way to describe it is that the selection should be based on the gaps/edges between characters rather than the characters themselves. So for example, if you have "ABC" and the mouse is in the right half of A or the left half of B, then the edge between A and B is where the selection should start.

@kovidgoyal
Copy link
Owner

Wouldn't that mean you cannot select the first character in a line when going left to right and the last character in a line going right-to-left? I suppose those could be special cased, but I am not entirely clear as to what the benefit of this mode of selection is? I would find it fairly confusing if I clicked on the right half of A and the next character got selected.

@jscheid
Copy link
Author

jscheid commented Sep 10, 2018

Ok so take for example this line:

ABCDEF

If I start between A and B (i.e. between 0.5 and 1.5 characters) and move to the right, then B should be the first character selected.

If I start between A and B and move to the left, then A should be selected.

If I start in the left half of A (i.e between 0 and 0.5) and move to the right, then the selection should start with A.

I would say that is a much more natural way to select, and I'm pretty sure it's how selection works in the majority of terminals and text editors.

@kovidgoyal
Copy link
Owner

So to summarize, the algorithm seems to be select a character if the mouse is in the latter half of the character (where latter half is defined by the recent horizontal motion of the mouse), is that correct?

@jscheid
Copy link
Author

jscheid commented Sep 10, 2018

Yes pretty much, but it doesn't depend on recent motion, it only depends on start and end position, i.e. on whether you select text forwards or backwards.

Again, I think the best way to conceptualize it is to think of the selection as stretching between the (zero-sized) space between characters, rather than the characters themselves.

@kovidgoyal
Copy link
Owner

OK, personally I dont much see the point, but then I'm not a big mouse user, so I wont claim any special expertise on how mice should behave :)

The selection code is in mouse.c it would require some refactoring to implement this algorithm since the mouse handling code currently tracks only the cell the mouse cursor is on, not its offset within the cell.

SO maybe I'll get to it someday, in the meantime, patches are welcome. If you do want to implement it, fele free to ask if you need help with anything.

@jscheid
Copy link
Author

jscheid commented Sep 10, 2018

Ok, thank you. I might take a look tonight to see how much work it would be.

@jscheid
Copy link
Author

jscheid commented Sep 10, 2018

Is it possible that you can't currently select just a single character, that you have to select at least two?

It seems to be so because of (start_x == end_x && start_y == end_y) in

kitty/kitty/screen.c

Lines 1492 to 1494 in 0195e7f

is_selection_empty(Screen *self, unsigned int start_x, unsigned int start_y, unsigned int end_x, unsigned int end_y) {
return (start_x >= self->columns || start_y >= self->lines || end_x >= self->columns || end_y >= self->lines || (start_x == end_x && start_y == end_y)) ? true : false;
}

@kovidgoyal
Copy link
Owner

Yes, it is the case. If you want to remove it, you can probably do so safely by changing the check to

(start_x == end_x && start_y == end_y && start_x && start_y)

the idea is to exclude the empty selection which is initialized with all parameters zero

@jscheid
Copy link
Author

jscheid commented Sep 10, 2018

Hmm... I've got something working, but it requires a slight change to the semantics of screen_update_selection. Are there many plugins I'd break with it?

@kovidgoyal
Copy link
Owner

No plugins depend on selection semantics, but there are several selection modes, such as right click to exten selection, multiple clicks to select words/lines, etc. https://sw.kovidgoyal.net/kitty/#mouse-features

jscheid added a commit to jscheid/kitty that referenced this issue Sep 10, 2018
Base mouse selection on the edges between cells rather than the cells
themselves.

For finding the start and end line in regular selection, the vertical
cell index is used rather than the edge.  However, the vertical edge
is used in rectangular selection mode.

Now allows selection of a single character.

This changes the semantics of screen_start_selection and
screen_update_selection to accept an edge index instead of cell index
for x, and an edge or cell index for y depending on whether
rectangular selection is enabled.

Closes kovidgoyal#945
jscheid added a commit to jscheid/kitty that referenced this issue Sep 10, 2018
Base mouse selection on the edges between cells rather than the cells
themselves.

For finding the start and end line in regular selection, the vertical
cell index is used rather than the edge.  However, the vertical edge
is used in rectangular selection mode.

Now allows selection of a single character.

This changes the semantics of screen_start_selection and
screen_update_selection to accept an edge index instead of cell index
for x, and an edge or cell index for y depending on whether
rectangular selection is enabled.

Closes kovidgoyal#945
@egmontkob
Copy link

egmontkob commented Sep 20, 2018

+1 for the feature request.

Note that currently I'm rewriting the selection code in VTE, and handling issues like this.

For me, the essential is that for UX consistency it should behave like pretty much all graphical applications do (and unfortunately, terminal emulators often don't).

In pretty much all graphical apps, the purpose of a single click is to position the I-beam cursor, where for intuitive behavior the closer boundary has to be located. And IMO logically and intuitively, they keep this behavior for selecting too.

The two axes aren't handled symmetrically. As for the row, it's simple: what matters is which row you clicked in. As for the column: what should matter is which vertical boundary between characters (even considering CJKs) is closer. If you clicked (or dragged the mouse) over the left half of a character (no matter how many cells that character occupies) then the selection's endpoint should be before this character, if you clicked (or dragged the mouse) over its right half then after this character.

This method, however, cannot be extended to word selection by double clicking. There the only reasonable behavior I could come up with (after examining the behavior of graphical toolkits and finding inconsistencies/bugs in pretty much all of them) is to check over which character the click occurred and make sure that that character (and its entire word) is selected; and the same for the other endpoint: over which cell the mouse was dragged to.

See https://gitlab.gnome.org/GNOME/vte/issues/34, and a long detailed comment inside work-in-progress vte-34-selection-big-rewrite-v1.patch, beginning with "Convert the mouse click or drag location" for further details and crazy corner cases.

I found it the most convenient to store internally the "half cell" (left or right half) over which the click occurred, special values -1 and 2*width denoting padding clicks/drags and off-window drags, and resolve to selection endpoints using different strategies for normal, word and other selection modes. It's not strictly speaking a necessity, I had working versions of the patch where entire cells we denoted (the concept of "half cell" wasn't introduced), however, the semantics was different for regular and word selection (the variable contained boundary in regular mode, and actual cell coordinate in word mode). I just happened to find the overall code design cleaner with the half cell approach in VTE.

@kovidgoyal
Copy link
Owner

@egmontkob thanks I'm sure your experience will come in handy.

martinetd pushed a commit to martinetd/kitty that referenced this issue Nov 30, 2018
Base mouse selection on the edges between cells rather than the cells
themselves.

For finding the start and end line in regular selection, the vertical
cell index is used rather than the edge.  However, the vertical edge
is used in rectangular selection mode.

Now allows selection of a single character.

This changes the semantics of screen_start_selection and
screen_update_selection to accept an edge index instead of cell index
for x, and an edge or cell index for y depending on whether
rectangular selection is enabled.

[asmadeus@codewreck.org: fixed rebase conflicts]
Closes kovidgoyal#945
@fdev31
Copy link
Contributor

fdev31 commented Feb 13, 2019

Hi!
What's up with this feature ?
I thought we could just keep track of the side clicked when starting the selection, then add -1 to the cell or +1 depending on the drag direction... I made a quick prototype which works only for a right drag and introduces a bug on the left drag (only one side is supported).
I don't know the code, it is quite clean and I tough a more minimal patch could do it (just a very imperfect proof of concept to show the idea).

I believe to finish it screen_start_selection could be changed to pass the full mouse context instead of some attributes only (lazy solution) or to preserve functions decoupling a new parameter could be introduced...

diff --git a/kitty/mouse.c b/kitty/mouse.c
index 40581451..fd7561d4 100644
--- a/kitty/mouse.c
+++ b/kitty/mouse.c
@@ -131,7 +131,7 @@ distance_to_window(Window *w, OSWindow *os_window) {
 static bool clamp_to_window = false;
 
 static inline bool
-cell_for_pos(Window *w, unsigned int *x, unsigned int *y, OSWindow *os_window) {
+cell_for_pos(Window *w, unsigned int *x, unsigned int *y, unsigned int *right_side, OSWindow *os_window) {
     WindowGeometry *g = &w->geometry;
     Screen *screen = w->render_data.screen;
     if (!screen) return false;
@@ -151,6 +151,13 @@ cell_for_pos(Window *w, unsigned int *x, unsigned int *y, OSWindow *os_window) {
     else if (mouse_y >= g->top) qy = (unsigned int)((double)(mouse_y - g->top) / os_window->fonts_data->cell_height);
     if (qx < screen->columns && qy < screen->lines) {
         *x = qx; *y = qy;
+        if (right_side != NULL) {
+            if (fmod(mouse_x , os_window->fonts_data->cell_width) > (os_window->fonts_data->cell_width/2.0) && qx+1 < screen->columns) {
+                *right_side = 1;
+            } else {
+                *right_side = 0;
+            }
+        }
         return true;
     }
     return false;
@@ -170,6 +177,7 @@ update_drag(bool from_button, Window *w, bool is_release, int modifiers) {
         }
         else {
             global_state.active_drag_in_window = w->id;
+            w->mouse_pos.drag_from_right = w->mouse_pos.right_side;
             screen_start_selection(screen, w->mouse_pos.cell_x, w->mouse_pos.cell_y, modifiers == (int)OPT(rectangle_select_modifiers) || modifiers == ((int)OPT(rectangle_select_modifiers) | GLFW_MOD_SHIFT), EXTEND_CELL);
         }
     } else if (screen->selection.in_progress) {
@@ -268,23 +276,28 @@ detect_url(Screen *screen, unsigned int x, unsigned int y) {
 
 
 HANDLER(handle_move_event) {
-    unsigned int x = 0, y = 0;
+    unsigned int x = 0, y = 0, rs = 0;
     if (OPT(focus_follows_mouse)) {
         Tab *t = global_state.callback_os_window->tabs + global_state.callback_os_window->active_tab;
         if (window_idx != t->active_window) {
             call_boss(switch_focus_to, "I", window_idx);
         }
     }
-    if (!cell_for_pos(w, &x, &y, global_state.callback_os_window)) return;
+    if (!cell_for_pos(w, &x, &y, &rs, global_state.callback_os_window)) return;
     Screen *screen = w->render_data.screen;
     detect_url(screen, x, y);
     bool mouse_cell_changed = x != w->mouse_pos.cell_x || y != w->mouse_pos.cell_y;
-    w->mouse_pos.cell_x = x; w->mouse_pos.cell_y = y;
     bool handle_in_kitty = (
             (screen->modes.mouse_tracking_mode == ANY_MODE ||
             (screen->modes.mouse_tracking_mode == MOTION_MODE && button >= 0)) &&
             !(global_state.callback_os_window->is_key_pressed[GLFW_KEY_LEFT_SHIFT] || global_state.callback_os_window->is_key_pressed[GLFW_KEY_RIGHT_SHIFT])
     ) ? false : true;
+    if (rs && !screen->selection.in_progress && handle_in_kitty) {
+        w->mouse_pos.cell_x = x + 1;
+    } else {
+        w->mouse_pos.cell_x = x;
+    }
+    w->mouse_pos.cell_y = y;
     if (handle_in_kitty) {
         if (screen->selection.in_progress && button == GLFW_MOUSE_BUTTON_LEFT) {
             double now = monotonic();
diff --git a/kitty/screen.c b/kitty/screen.c
index 3e8bbc35..ecae7746 100644
--- a/kitty/screen.c
+++ b/kitty/screen.c
@@ -2105,6 +2105,9 @@ screen_update_selection(Screen *self, index_type x, index_type y, bool ended) {
             break;
         }
         case EXTEND_CELL:
+            if (! extending_leftwards) {
+
+            }
             break;
     }
     call_boss(set_primary_selection, NULL);
diff --git a/kitty/state.h b/kitty/state.h
index 7c2b5d1d..e2cfaf05 100644
--- a/kitty/state.h
+++ b/kitty/state.h
@@ -71,6 +71,8 @@ typedef struct {
     struct {
         unsigned int cell_x, cell_y;
         double x, y;
+        unsigned char right_side;
+        unsigned char drag_from_right;
     } mouse_pos;
     WindowGeometry geometry;
     ClickQueue click_queue;

@fdev31
Copy link
Contributor

fdev31 commented Feb 13, 2019

Just for clarification, this part:

+    if (rs && !screen->selection.in_progress && handle_in_kitty) {
+        w->mouse_pos.cell_x = x + 1;
+    } else {
+        w->mouse_pos.cell_x = x;
+    }

is a mistake to demonstrate the behaviour (on the right drag only), it should be computed dynamically depending on the current "extending_leftwards" status.

@kovidgoyal
Copy link
Owner

Well @martinetd has something in the works, see #1007

@fdev31
Copy link
Contributor

fdev31 commented Feb 17, 2019

I've been testing this code for some days now (still applies automatically on top of master):

  • it works "great" (selection works as on any other Xorg App)
  • I didn't have any issue

Did I miss some remaining work/regressions ?

@kovidgoyal
Copy link
Owner

See the PR for details.

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

No branches or pull requests

4 participants