From 21ab16239d87fe8fb4d7651b4bccbe1f611df620 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= Date: Mon, 22 Aug 2022 20:14:06 +0200 Subject: [PATCH] selection: once again highlight non-trailing empty cells MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Foot 1.13.0 introduced a regression where non-trailing empty cells were highlighted inconsistently (cells that shouldn’t be highlighted, were, seemingly at random). 86663522d5915c0fdb2b8b132bacc247fcfd1fb8 “fixed” this by never highlighting *any* empty cells. This meant the behavior, compared to foot 1.12 and earlier, changed. In foot 1.12 and older versions, non-trailing empty cells were highlighted, as long as the selection covered at least one of the trailing non-empty cells. This patch restores that behavior. To understand how this works, lets first take a look at how selection works: When a selection is made, and updated (i.e. the mouse is dragged, or the selection is extended through RMB etc), we need to (un)tag and dirty the cells that are a) newly selected, or b) newly deselected. That is, we look at the diff between the “old” and the “new” selection, and only update those cells. This is for performance reasons: iterating the entire selection is not feasible with large selections. However, it also means we cannot reason about empty cells; we simply don’t know if an empty cells is a trailing empty cell, or a non-trailing one. Then, when we render a frame, we iterate all the *visible* and *selected* cells, once again tagging them as selected (this is needed since a selected cell might have lost its selected tag if the cell was written to, by the client application, after the selection was made). At this point, we *can* reason about empty cells. So, to restore the highlighting behavior to that of foot 1.12, we do this: When working with the selection diffs when a selection is updated, we don’t special case empty cells at all. Thus, all empty cells covered by the selection is highlighted, and dirtied. But, when rendering the frame, we _do_ special case them. The only difference (compared to foot 1.12) is that we *must* explicitly *clear* the selection tag, and dirty the empty cells. This is to ensure the empty cells that were incorrectly highlighted by the selection update algorithm, isn’t rendered as that. This does have a slight performance impact, as empty cells are now always re-rendered. The impact should however be small. --- CHANGELOG.md | 2 -- selection.c | 44 ++++++++++++++++++++++++++++++++++++++------ 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 656c5a46..2eddc3bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,8 +51,6 @@ ### Changed * Window is now dimmed while in Unicode input mode. -* Selected empty cells are **never** highlighted as being - selected. They used to be, when followed by non-empty cells. * Default color theme from a variant of the Zenburn theme, to a variant of the Solarized dark theme. diff --git a/selection.c b/selection.c index 2fd62f8a..5b65310c 100644 --- a/selection.c +++ b/selection.c @@ -713,7 +713,8 @@ pixman_region_for_coords(const struct terminal *term, static void mark_selected_region(struct terminal *term, pixman_box32_t *boxes, - size_t count, bool selected, bool dirty_cells) + size_t count, bool selected, bool dirty_cells, + bool highlight_empty) { for (size_t i = 0; i < count; i++) { const pixman_box32_t *box = &boxes[i]; @@ -737,7 +738,9 @@ mark_selected_region(struct terminal *term, pixman_box32_t *boxes, row->dirty = true; for (int c = box->x1, empty_count = 0; c < box->x2; c++) { - if (row->cells[c].wc == 0) { + struct cell *cell = &row->cells[c]; + + if (cell->wc == 0 && !highlight_empty) { /* * We used to highlight empty cells *if* they were * followed by non-empty cell(s), since this @@ -757,7 +760,36 @@ mark_selected_region(struct terminal *term, pixman_box32_t *boxes, * cells (they still get converted to spaces when * copied, if followed by non-empty cells). */ - /* empty_count++; */ + empty_count++; + + /* + * When the selection is *modified*, empty cells + * are treated just like non-empty cells; they are + * marked as selected, and dirtied. + * + * This is due to how the algorithm for updating + * the selection works; it uses regions to + * calculate the difference between the “old” and + * the “new” selection. This makes it impossible + * to tell if an empty cell is a *trailing* empty + * cell (that should not be highlighted), or an + * empty cells between non-empty cells (that + * *should* be highlighted). + * + * Then, when a frame is rendered, we loop the + * *visibible* cells that belong to the + * selection. At this point, we *can* tell if an + * empty cell is trailing or not. + * + * So, what we need to do is check if a + * ‘selected’, and empty cell has been marked as + * selected, temporarily unmark (forcing it dirty, + * to ensure it gets re-rendered). If it is *not* + * a trailing empty cell, it will get re-tagged as + * selected in the for-loop below. + */ + cell->attrs.clean = false; + cell->attrs.selected = false; continue; } @@ -810,10 +842,10 @@ selection_modify(struct terminal *term, struct coord start, struct coord end) pixman_box32_t *boxes = NULL; boxes = pixman_region32_rectangles(&no_longer_selected, &n_rects); - mark_selected_region(term, boxes, n_rects, false, true); + mark_selected_region(term, boxes, n_rects, false, true, true); boxes = pixman_region32_rectangles(&newly_selected, &n_rects); - mark_selected_region(term, boxes, n_rects, true, true); + mark_selected_region(term, boxes, n_rects, true, true, true); pixman_region32_fini(&newly_selected); pixman_region32_fini(&no_longer_selected); @@ -1078,7 +1110,7 @@ selection_dirty_cells(struct terminal *term) int n_rects = -1; pixman_box32_t *boxes = pixman_region32_rectangles(&visible_and_selected, &n_rects); - mark_selected_region(term, boxes, n_rects, true, false); + mark_selected_region(term, boxes, n_rects, true, false, false); pixman_region32_fini(&visible_and_selected); pixman_region32_fini(&view);