[vte/wip/egmont/bidi: 2/5] improved ringview update locations

[Egmont Koblinger <egmontkob src gnome org>]

commit 89dcdc90576221ed495a472cf4e73bd4f11b62af
Author: Egmont Koblinger <egmont gmail com>
Date:   Thu Jan 24 22:44:24 2019 +0100

    improved ringview update locations

 src/bidi.cc        |  24 ++++++++++-
 src/bidi.hh        |   5 ++-
 src/vte.cc         | 120 ++++++++++++++++++++---------------------------------
 src/vteinternal.hh |   2 +
 4 files changed, 73 insertions(+), 78 deletions(-)
---
diff --git a/src/bidi.cc b/src/bidi.cc
index 03b17e18..5078824c 100644
--- a/src/bidi.cc
+++ b/src/bidi.cc
@@ -138,6 +138,8 @@ RingView::RingView()
         for (int i = 0; i < m_height_alloc; i++) {
                 m_bidirows[i] = new BidiRow();
         }
+
+        m_invalid = true;
 }
 
 RingView::~RingView()
@@ -150,16 +152,27 @@ RingView::~RingView()
 
 void RingView::set_ring(Ring *ring)
 {
+        if (ring == m_ring)
+                return;
+
         m_ring = ring;
+        m_invalid = true;
 }
 
 void RingView::set_width(vte::grid::column_t width)
 {
+        if (width == m_width)
+                return;
+
         m_width = width;
+        m_invalid = true;
 }
 
 void RingView::set_rows(vte::grid::row_t start, vte::grid::row_t len)
 {
+        if (start == m_start && len == m_len)
+                return;
+
         if (G_UNLIKELY (len > m_height_alloc)) {
                 int i = m_height_alloc;
                 while (len > m_height_alloc) {
@@ -173,10 +186,14 @@ void RingView::set_rows(vte::grid::row_t start, vte::grid::row_t len)
 
         m_start = start;
         m_len = len;
+        m_invalid = true;
 }
 
-void RingView::update()
+void RingView::maybe_update()
 {
+        if (!m_invalid)
+                return;
+
         vte::grid::row_t i = m_start;
         const VteRowData *row_data = m_ring->index_safe(m_start);
 
@@ -189,12 +206,16 @@ void RingView::update()
         while (i < m_start + m_len) {
                 i = paragraph(i);
         }
+
+        m_invalid = false;
 }
 
 BidiRow const* RingView::get_row_map(vte::grid::row_t row) const
 {
         g_assert_cmpint (row, >=, m_start);
         g_assert_cmpint (row, <, m_start + m_len);
+        g_assert_false (m_invalid);
+
         return m_bidirows[row - m_start];
 }
 
@@ -202,6 +223,7 @@ BidiRow* RingView::get_row_map_writable(vte::grid::row_t row) const
 {
         g_assert_cmpint (row, >=, m_start);
         g_assert_cmpint (row, <, m_start + m_len);
+
         return m_bidirows[row - m_start];
 }
 
diff --git a/src/bidi.hh b/src/bidi.hh
index d42ac411..1b75dc98 100644
--- a/src/bidi.hh
+++ b/src/bidi.hh
@@ -85,7 +85,8 @@ public:
         void set_rows(vte::grid::row_t start, vte::grid::row_t len);
         void set_width(vte::grid::column_t width);
 
-        void update();
+        inline void invalidate() { m_invalid = true; }
+        void maybe_update();
 
         BidiRow const* get_row_map(vte::grid::row_t row) const;
 
@@ -100,6 +101,8 @@ private:
 
         vte::grid::row_t m_height_alloc;
 
+        bool m_invalid;
+
         BidiRow* get_row_map_writable(vte::grid::row_t row) const;
 
         void explicit_line(vte::grid::row_t row, bool rtl);
diff --git a/src/vte.cc b/src/vte.cc
index 5e555c73..3a06009a 100644
--- a/src/vte.cc
+++ b/src/vte.cc
@@ -1635,15 +1635,7 @@ Terminal::selection_maybe_swap_endpoints(vte::view::coords const& pos)
         if (m_selection_resolved.empty())
                 return;
 
-
-
-        // FIXME find a nicer place for these
-        m_ringview.set_ring (m_screen->row_data);
-        m_ringview.set_rows ((long) m_screen->scroll_delta, m_row_count + 3);
-        m_ringview.set_width (m_column_count);
-        m_ringview.update ();
-
-
+        ringview_maybe_update();
 
         auto current = selection_grid_halfcoords_from_view_coords (pos);
 
@@ -3856,6 +3848,11 @@ Terminal::process_incoming()
                queue_contents_changed();
        }
 
+        /* BiDi properties might have changed, even when !modified.
+         * emit_pending_signals() requires the ringview to be updated. */
+        m_ringview.invalidate();
+        ringview_maybe_update();
+
        emit_pending_signals();
 
        if (invalidated_text) {
@@ -4982,13 +4979,13 @@ Terminal::widget_key_press(GdkEventKey *event)
                              keyval == GDK_KEY_Right ||
                              keyval == GDK_KEY_KP_Left ||
                              keyval == GDK_KEY_KP_Right)) {
-                                /* m_ringview is for the onscreen contents and the cursor
-                                 * may be offscreen, so use a temporary ringview. */
+                                /* m_ringview is for the onscreen contents and the cursor may be
+                                 * offscreen, so use a temporary ringview for the cursor's row. */
                                 vte::base::RingView *ringview = new vte::base::RingView();
                                 ringview->set_ring(m_screen->row_data);
                                 ringview->set_rows(m_screen->cursor.row, 1);
                                 ringview->set_width(m_column_count);
-                                ringview->update();
+                                ringview->maybe_update();
                                 if (ringview->get_row_map(m_screen->cursor.row)->base_is_rtl()) {
                                         switch (keyval) {
                                         case GDK_KEY_Left:
@@ -5534,15 +5531,7 @@ Terminal::modify_selection (vte::view::coords const& pos)
 {
         g_assert (m_selecting);
 
-
-
-        // FIXME find a nicer place for these
-        m_ringview.set_ring (m_screen->row_data);
-        m_ringview.set_rows ((long) m_screen->scroll_delta, m_row_count + 3);
-        m_ringview.set_width (m_column_count);
-        m_ringview.update ();
-
-
+        ringview_maybe_update();
 
         auto current = selection_grid_halfcoords_from_view_coords (pos);
 
@@ -5929,14 +5918,6 @@ Terminal::hyperlink_hilite_update()
         if (!m_allow_hyperlink)
                 return;
 
-
-        // FIXME find a nicer place for these
-        m_ringview.set_ring (m_screen->row_data);
-        m_ringview.set_rows ((long) m_screen->scroll_delta, m_row_count + 3);
-        m_ringview.set_width (m_column_count);
-        m_ringview.update ();
-
-
         _vte_debug_print (VTE_DEBUG_HYPERLINK,
                          "hyperlink_hilite_update\n");
 
@@ -6046,14 +6027,6 @@ Terminal::match_hilite_update()
         glong col = pos.x / m_cell_width;
         glong row = pixel_to_row(pos.y);
 
-
-        // FIXME find a nicer place for these
-        m_ringview.set_ring (m_screen->row_data);
-        m_ringview.set_rows ((long) m_screen->scroll_delta, m_row_count + 3);
-        m_ringview.set_width (m_column_count);
-        m_ringview.update ();
-
-
         /* BiDi: convert to logical column. */
         vte::base::BidiRow const* bidirow = m_ringview.get_row_map(confine_grid_row(row));
         col = bidirow->vis2log(col);
@@ -6280,12 +6253,12 @@ Terminal::get_text(vte::grid::row_t start_row,
                  * m_ringview corresponds to the currently onscreen bits, therefore does not
                  * necessarily include the entire selection.
                  * Modifying m_ringview and then reverting would be a bit cumbersome,
-                 * creating a new one for the selection is cleaner. */
+                 * creating a new one for the selection is simpler. */
                 ringview = new vte::base::RingView();
                 ringview->set_ring(m_screen->row_data);
                 ringview->set_rows(start_row, end_row - start_row + 1);
                 ringview->set_width(m_column_count);
-                ringview->update();
+                ringview->maybe_update();
         }
 
         vte::grid::column_t col = block ? 0 : start_col;
@@ -6831,16 +6804,6 @@ Terminal::start_selection (vte::view::coords const& pos,
        if (m_selection_block_mode)
                type = selection_type_char;
 
-
-
-        // FIXME find a nicer place for these
-        m_ringview.set_ring (m_screen->row_data);
-        m_ringview.set_rows ((long) m_screen->scroll_delta, m_row_count + 3);
-        m_ringview.set_width (m_column_count);
-        m_ringview.update ();
-
-
-
         m_selection_origin = m_selection_last = selection_grid_halfcoords_from_view_coords(pos);
 
        /* Record the selection type. */
@@ -7001,6 +6964,8 @@ Terminal::widget_motion_notify(GdkEventMotion *event)
 {
        bool handled = false;
 
+        ringview_maybe_update();
+
         GdkEvent* base_event = reinterpret_cast<GdkEvent*>(event);
         auto pos = view_coords_from_event(base_event);
         auto rowcol = grid_coords_from_view_coords(pos);
@@ -7009,6 +6974,8 @@ Terminal::widget_motion_notify(GdkEventMotion *event)
                          "Motion notify %s %s\n",
                          pos.to_string(), rowcol.to_string());
 
+        ringview_maybe_update();
+
        read_modifiers(base_event);
 
        switch (event->type) {
@@ -7066,6 +7033,8 @@ Terminal::widget_button_press(GdkEventButton *event)
        bool handled = false;
        gboolean start_selecting = FALSE, extend_selecting = FALSE;
 
+        ringview_maybe_update();
+
         GdkEvent* base_event = reinterpret_cast<GdkEvent*>(event);
         auto pos = view_coords_from_event(base_event);
         auto rowcol = grid_coords_from_view_coords(pos);
@@ -7215,6 +7184,8 @@ Terminal::widget_button_release(GdkEventButton *event)
 {
        bool handled = false;
 
+        ringview_maybe_update();
+
         GdkEvent* base_event = reinterpret_cast<GdkEvent*>(event);
         auto pos = view_coords_from_event(base_event);
         auto rowcol = grid_coords_from_view_coords(pos);
@@ -7332,6 +7303,8 @@ Terminal::widget_focus_out(GdkEventFocus *event)
 void
 Terminal::widget_enter(GdkEventCrossing *event)
 {
+        ringview_maybe_update();
+
         GdkEvent* base_event = reinterpret_cast<GdkEvent*>(event);
         auto pos = view_coords_from_event(base_event);
 
@@ -7349,6 +7322,8 @@ Terminal::widget_enter(GdkEventCrossing *event)
 void
 Terminal::widget_leave(GdkEventCrossing *event)
 {
+        ringview_maybe_update();
+
         GdkEvent* base_event = reinterpret_cast<GdkEvent*>(event);
         auto pos = view_coords_from_event(base_event);
 
@@ -8998,6 +8973,22 @@ Terminal::draw_cells_with_attributes(struct _vte_draw_text_request *items,
 }
 
 
+void
+Terminal::ringview_maybe_update()
+{
+        m_ringview.set_ring (m_screen->row_data);
+        /* Due to possibly unaligned height and per-pixel scrolling, up to 2 more lines than the
+         * logical height can be partially visible.
+         * If a row is just underneath the current viewport, we still need to run BiDi on it
+         * because the top an outlined rectangle cursor shape peeks in into the viewport, and we need
+         * to know where the BiDi algorithm maps that cursor. However, +2 is still enough for this
+         * as long as the outline cursor is 1px thin. If we ever make it wider, we'll need +3.
+         */
+        m_ringview.set_rows ((long) m_screen->scroll_delta, m_row_count + 2);
+        m_ringview.set_width (m_column_count);
+        m_ringview.maybe_update ();
+}
+
 /* XXX tmp hack */
 #define _vte_row_data_get_visual(row_data_p, bidimap, col) \
         row_data_p == nullptr ? nullptr : _vte_row_data_get(row_data_p, bidimap->vis2log(col))
@@ -9033,16 +9024,6 @@ Terminal::draw_rows(VteScreen *screen_,
 
         items = g_newa (struct _vte_draw_text_request, column_count);
 
-
-
-        // FIXME find a nicer place for these
-        m_ringview.set_ring (m_screen->row_data);
-        m_ringview.set_rows ((long) m_screen->scroll_delta, m_row_count + 3);
-        m_ringview.set_width (m_column_count);
-        m_ringview.update ();
-
-
-
         /* Paint the background.
          * Do it first for all the cells we're about to paint, before drawing the glyphs,
          * so that overflowing bits of a glyph (to the right or downwards) won't be
@@ -9334,17 +9315,6 @@ Terminal::paint_cursor()
        if (CLAMP(col, 0, m_column_count - 1) != col)
                return;
 
-
-
-
-        // FIXME find a nicer place for these
-        m_ringview.set_ring (m_screen->row_data);
-        m_ringview.set_rows ((long) m_screen->scroll_delta, m_row_count + 3);
-        m_ringview.set_width (m_column_count);
-        m_ringview.update ();
-
-
-
         /* Find the first cell of the character "under" the cursor.
          * This is for CJK.  For TAB, paint the cursor where it really is. */
         VteRowData const *row_data = find_row_data(drow);
@@ -9591,6 +9561,8 @@ Terminal::widget_draw(cairo_t *cr)
         if (region == NULL)
                 return;
 
+        ringview_maybe_update();
+
         allocated_width = get_allocated_width();
         allocated_height = get_allocated_height();
 
@@ -9750,6 +9722,8 @@ Terminal::widget_scroll(GdkEventScroll *event)
 
         GdkEvent *base_event = reinterpret_cast<GdkEvent*>(event);
 
+        ringview_maybe_update();
+
        read_modifiers(base_event);
 
        switch (event->direction) {
@@ -9783,12 +9757,6 @@ Terminal::widget_scroll(GdkEventScroll *event)
                                "Scroll application by %d lines, smooth scroll delta set back to %f\n",
                                cnt, m_mouse_smooth_scroll_delta);
 
-                // FIXME find a nicer place for these – rowcol below needs an updated ringview to do BiDi
-                m_ringview.set_ring (m_screen->row_data);
-                m_ringview.set_rows ((long) m_screen->scroll_delta, m_row_count + 3);
-                m_ringview.set_width (m_column_count);
-                m_ringview.update ();
-
                 auto rowcol = confined_grid_coords_from_event(base_event);
 
                button = cnt > 0 ? 5 : 4;
diff --git a/src/vteinternal.hh b/src/vteinternal.hh
index 73211e11..e3e3c485 100644
--- a/src/vteinternal.hh
+++ b/src/vteinternal.hh
@@ -1311,6 +1311,8 @@ public:
                                    vte::parser::StringTokeniser::const_iterator& token,
                                    vte::parser::StringTokeniser::const_iterator const& endtoken) noexcept;
 
+        void ringview_maybe_update();
+
         /* Sequence handlers */
         bool m_line_wrapped; // signals line wrapped from character insertion
         // Note: inlining the handlers seems to worsen the performance, so we don't do that