commit 8d8988de475bf2055f253823e53fd5627be5de78
Author: Jean-Marc Lasgouttes <lasgout...@lyx.org>
Date:   Wed Oct 11 18:00:48 2017 +0200

    Allow multiple calls to processUpdateFlags before redraw
    
    The goal of this commit is to ensure that a processUpdateFlags call
    that requires no redraw will not override a previous one that did
    require a redraw.
    
    To this end, the semantics of the flag argument is now different: its
    value is now OR'ed with a private update_flags_ variable. This
    variable is only reset after the buffer view has actually been
    redrawn.
    
    A new Update::ForceRedraw flag has been added. It requires a full
    redraw but no metrics computation. It is not used in the main code
    (yet), but avoids to compute metrics repeatedly in consecutive
    processUpdateFlags calls.
    
    The process is now as follows:
    - if flags is just None, return immediately, there is nothing to do.
    - the Force flag is honored (full metrics computation) and replaced
      with ForceDraw.
    - the FitCursor flag is honored and removed from the flags.
    - the SinglePar update is added if ForceDraw is not in flags and only
      the current par has been modified.
    
    The remaining flags are only then added to the BufferView update
    flags, and the update strategy is computed for the next paint event.
    
    Finally the dubious call to updateMacros in updateMetrics has been
    removed for performance reasons.
---
 development/PAINTING_ANALYSIS |   21 +++--
 src/BufferView.cpp            |  176 ++++++++++++++++++++++-------------------
 src/BufferView.h              |    9 ++-
 src/TextMetrics.cpp           |    7 +-
 src/update_flags.h            |   14 +++-
 5 files changed, 127 insertions(+), 100 deletions(-)

diff --git a/development/PAINTING_ANALYSIS b/development/PAINTING_ANALYSIS
index f734edb..ec3566e 100644
--- a/development/PAINTING_ANALYSIS
+++ b/development/PAINTING_ANALYSIS
@@ -60,12 +60,6 @@ cursor.
 
 * Clean-up of drawing code
 
-The goal is to make painting with drawing disable fast enough that it
-can be used after every metrics computation. Then we can separate real
-drawing from metrics.
-
-Other changes are only clean-ups.
-
 ** When a paragraph ends with a newline, compute correctly the height of the 
extra row.
 ** Merging bv::updateMetrics and tm::metrics
 
@@ -76,7 +70,7 @@ insets. We should re-use the bv::updateMetrics logic:
  + transfer all the logic of bv::updateMetrics to tm.
  + Main InsetText should not be special.
 
-The difficuly for a tall table cell for example, is that it may be
+The difficulty for a tall table cell for example, is that it may be
 necessary to break the whole contents to know the width of the cell.
 
 
@@ -113,11 +107,19 @@ DecorationUpdate). It triggers a recomputation of the 
metrics when either:
    existing metrics. Note that the Update::SinglePar flag is *never*
    taken into account.
 
+If a computation of metrics has taken place, Force is removed from the
+flags and ForceDraw is added instead.
+
+It is OK to call processUptateFlags several times before an update. In
+this case, the effects are cumulative.processUpdateFlags execute the
+metrics-related actions, but defers the actual drawing to the next
+paint event.
+
 The screen is drawn (with appropriate update strategy), except when
 update flag is Update::None.
 
 
-** Metrics computation
+** Metrics computation (and nodraw drawing phase)
 
 This is triggered by bv::updateMetrics, which calls tm::redoParagraph for
 all visible paragraphs. Some Paragraphs above or below the screen (needed
@@ -127,6 +129,9 @@ tm::redoParagraph will call Inset::metrics for each inset. 
In the case
 of text insets, this will invoke recursively tm::metrics, which redoes
 all the paragraphs of the inset.
 
+At the end of the function, bv::updatePosCache is called. It triggers
+a repaint of the document with a NullPainter (a painter that does
+nothing). This has the effect of caching all insets positions.
 
 ** Drawing the work area.
 
diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 53d374f..4e2428d 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -228,7 +228,8 @@ enum ScreenUpdateStrategy {
 
 struct BufferView::Private
 {
-       Private(BufferView & bv) : update_strategy_(NoScreenUpdate),
+       Private(BufferView & bv) : update_strategy_(FullScreenUpdate),
+               update_flags_(Update::Force),
                wh_(0), cursor_(bv),
                anchor_pit_(0), anchor_ypos_(0),
                inlineCompletionUniqueChars_(0),
@@ -245,6 +246,8 @@ struct BufferView::Private
        ///
        ScreenUpdateStrategy update_strategy_;
        ///
+       Update::flags update_flags_;
+       ///
        CoordCache coord_cache_;
 
        /// Estimated average par height for scrollbar.
@@ -445,79 +448,85 @@ bool BufferView::needsFitCursor() const
 }
 
 
-void BufferView::processUpdateFlags(Update::flags flags)
+namespace {
+
+// this is for debugging only.
+string flagsAsString(Update::flags flags)
 {
-       // This is close to a hot-path.
-       LYXERR(Debug::PAINTING, "BufferView::processUpdateFlags()"
-               << "[fitcursor = " << (flags & Update::FitCursor)
-               << ", forceupdate = " << (flags & Update::Force)
-               << ", singlepar = " << (flags & Update::SinglePar)
-               << "]  buffer: " << &buffer_);
+       if (flags == Update::None)
+               return "None ";
+       return string((flags & Update::FitCursor) ? "FitCursor " : "")
+               + ((flags & Update::Force) ? "Force " : "")
+               + ((flags & Update::ForceDraw) ? "ForceDraw " : "")
+               + ((flags & Update::SinglePar) ? "SinglePar " : "");
+}
 
-       // FIXME Does this really need doing here? It's done in updateBuffer, 
and
-       // if the Buffer doesn't need updating, then do the macros?
-       buffer_.updateMacros();
+}
 
-       // Now do the first drawing step if needed. This consists on updating
-       // the CoordCache in updateMetrics().
-       // The second drawing step is done in WorkArea::redraw() if needed.
-       // FIXME: is this still true now that Buffer::changed() is used all 
over?
+void BufferView::processUpdateFlags(Update::flags flags)
+{
+       LYXERR(Debug::PAINTING, "BufferView::processUpdateFlags( "
+                  << flagsAsString(flags) << ")  buffer: " << &buffer_);
 
        // Case when no explicit update is requested.
-       if (!flags) {
-               // no need to redraw anything.
-               d->update_strategy_ = NoScreenUpdate;
+       if (flags == Update::None)
                return;
-       }
 
-       if (flags == Update::Decoration) {
-               d->update_strategy_ = DecorationUpdate;
-               buffer_.changed(false);
-               return;
+       // SinglePar is ignored for now (this should probably change). We
+       // set it ourselves below, at the price of always rebreaking the
+       // paragraph at cursor. This can be expensive for large tables.
+       flags = flags & ~Update::SinglePar;
+
+       // First check whether the metrics and inset positions should be updated
+       if (flags & Update::Force) {
+               // This will update the CoordCache items and replace Force
+               // with ForceDraw in flags.
+               updateMetrics(flags);
        }
 
-       if (flags == Update::FitCursor
-               || flags == (Update::Decoration | Update::FitCursor)) {
-               // tell the frontend to update the screen if needed.
+       // Then make sure that the screen contains the cursor if needed
+       if (flags & Update::FitCursor) {
                if (needsFitCursor()) {
-                       showCursor();
-                       return;
+                       scrollToCursor(d->cursor_, false);
+                       // Metrics have to be recomputed (maybe again)
+                       updateMetrics(flags);
                }
-               if (flags & Update::Decoration) {
-                       d->update_strategy_ = DecorationUpdate;
-                       buffer_.changed(false);
-                       return;
-               }
-               // no screen update is needed in principle, but this
-               // could change if cursor row needs horizontal scrolling.
-               d->update_strategy_ = NoScreenUpdate;
-               buffer_.changed(false);
-               return;
+               flags = flags & ~Update::FitCursor;
        }
 
-       bool const full_metrics = flags & Update::Force || !singleParUpdate();
-
-       if (full_metrics)
-               // We have to update the full screen metrics.
-               updateMetrics();
-
-       if (!(flags & Update::FitCursor)) {
-               // Nothing to do anymore. Trigger a redraw and return
-               buffer_.changed(false);
-               return;
+       // Finally detect whether we can only repaint a single paragraph
+       if (!(flags & Update::ForceDraw)) {
+               if (singleParUpdate())
+                       flags = flags | Update::SinglePar;
+               else
+                       updateMetrics(flags);
        }
 
-       // updateMetrics() does not update paragraph position
-       // This is done at draw() time. So we need a redraw!
-       buffer_.changed(false);
+       // Add flags to the the update flags. These will be reset to None
+       // after the redraw is actually done
+       d->update_flags_ = d->update_flags_ | flags;
+       LYXERR(Debug::PAINTING, "Cumulative flags: " << flagsAsString(flags));
 
-       if (needsFitCursor()) {
-               // The cursor is off screen so ensure it is visible.
-               // refresh it:
-               showCursor();
+       // Now compute the update strategy
+       // Possibly values in flag are None, Decoration, ForceDraw
+       LATTEST((d->update_flags_ & ~(Update::None | Update::SinglePar
+                                     | Update::Decoration | 
Update::ForceDraw)) == 0);
+
+       if (d->update_flags_ & Update::ForceDraw)
+               d->update_strategy_ = FullScreenUpdate;
+       else if (d->update_flags_ & Update::Decoration)
+               d->update_strategy_ = DecorationUpdate;
+       else if (d->update_flags_ & Update::SinglePar)
+               d->update_strategy_ = SingleParUpdate;
+       else {
+               // no need to redraw anything.
+               d->update_strategy_ = NoScreenUpdate;
        }
 
        updateHoveredInset();
+
+       // Trigger a redraw.
+       buffer_.changed(false);
 }
 
 
@@ -632,8 +641,7 @@ void BufferView::scrollDocView(int const value, bool update)
        // If the offset is less than 2 screen height, prefer to scroll instead.
        if (abs(value) <= 2 * height_) {
                d->anchor_ypos_ -= value;
-               buffer_.changed(true);
-               updateHoveredInset();
+               processUpdateFlags(Update::Force);
                return;
        }
 
@@ -830,12 +838,7 @@ bool BufferView::moveToPosition(pit_type bottom_pit, 
pos_type bottom_pos,
                d->cursor_.setCurrentFont();
                // Do not forget to reset the anchor (see #9912)
                d->cursor_.resetAnchor();
-               // To center the screen on this new position we need the
-               // paragraph position which is computed at draw() time.
-               // So we need a redraw!
-               buffer_.changed(false);
-               if (needsFitCursor())
-                       showCursor();
+               processUpdateFlags(Update::FitCursor);
        }
 
        return success;
@@ -877,19 +880,15 @@ void BufferView::showCursor()
 void BufferView::showCursor(DocIterator const & dit,
        bool recenter, bool update)
 {
-       if (scrollToCursor(dit, recenter) && update) {
-               buffer_.changed(true);
-               updateHoveredInset();
-       }
+       if (scrollToCursor(dit, recenter) && update)
+               processUpdateFlags(Update::Force);
 }
 
 
 void BufferView::scrollToCursor()
 {
-       if (scrollToCursor(d->cursor_, false)) {
-               buffer_.changed(true);
-               updateHoveredInset();
-       }
+       if (scrollToCursor(d->cursor_, false))
+               processUpdateFlags(Update::Force);
 }
 
 
@@ -1715,8 +1714,8 @@ void BufferView::dispatch(FuncRequest const & cmd, 
DispatchResult & dr)
                bool const in_texted = cur.inTexted();
                cur.setCursor(doc_iterator_begin(cur.buffer()));
                cur.selHandle(false);
-               buffer_.changed(true);
-               updateHoveredInset();
+               // Force an immediate computation of metrics because we need it 
below
+               processUpdateFlags(Update::Force);
 
                d->text_metrics_[&buffer_.text()].editXY(cur, p.x_, p.y_,
                        true, act == LFUN_SCREEN_UP);
@@ -1750,8 +1749,7 @@ void BufferView::dispatch(FuncRequest const & cmd, 
DispatchResult & dr)
                        if (scroll_value)
                                scroll(scroll_step * scroll_value);
                }
-               buffer_.changed(true);
-               updateHoveredInset();
+               dr.screenUpdate(Update::ForceDraw);
                dr.forceBufferUpdate();
                break;
        }
@@ -2646,7 +2644,6 @@ bool BufferView::singleParUpdate()
                return false;
 
        tm.updatePosCache(bottom_pit);
-       d->update_strategy_ = SingleParUpdate;
 
        LYXERR(Debug::PAINTING, "\ny1: " << pm.position() - pm.ascent()
                << " y2: " << pm.position() + pm.descent()
@@ -2658,6 +2655,13 @@ bool BufferView::singleParUpdate()
 
 void BufferView::updateMetrics()
 {
+       updateMetrics(d->update_flags_);
+       d->update_strategy_ = FullScreenUpdate;
+}
+
+
+void BufferView::updateMetrics(Update::flags & update_flags)
+{
        if (height_ == 0 || width_ == 0)
                return;
 
@@ -2741,7 +2745,8 @@ void BufferView::updateMetrics()
                << " pit1 = " << pit1
                << " pit2 = " << pit2);
 
-       d->update_strategy_ = FullScreenUpdate;
+       // metrics is done, full drawing is necessary now
+       update_flags = (update_flags & ~Update::Force) | Update::ForceDraw;
 
        // Now update the positions of insets in the cache.
        updatePosCache();
@@ -3050,8 +3055,8 @@ void BufferView::draw(frontend::Painter & pain, bool 
paint_caret)
 {
        if (height_ == 0 || width_ == 0)
                return;
-       LYXERR(Debug::PAINTING, "\t\t*** START DRAWING ***");
-
+       LYXERR(Debug::PAINTING, (pain.isNull() ? "\t\t--- START NODRAW ---"
+                                : "\t\t*** START DRAWING ***"));
        Text & text = buffer_.text();
        TextMetrics const & tm = d->text_metrics_[&text];
        int const y = tm.first().second->position();
@@ -3130,7 +3135,8 @@ void BufferView::draw(frontend::Painter & pain, bool 
paint_caret)
                }
                break;
        }
-       LYXERR(Debug::PAINTING, "\n\t\t*** END DRAWING  ***");
+       LYXERR(Debug::PAINTING, (pain.isNull() ? "\t\t --- END NODRAW ---"
+                               : "\t\t *** END DRAWING ***"));
 
        // The scrollbar needs an update.
        updateScrollbar();
@@ -3141,13 +3147,19 @@ void BufferView::draw(frontend::Painter & pain, bool 
paint_caret)
        for (pit_type pit = firstpm.first; pit <= lastpm.first; ++pit) {
                ParagraphMetrics const & pm = tm.parMetrics(pit);
                if (pm.position() + pm.descent() > 0) {
+                       if (d->anchor_pit_ != pit
+                           || d->anchor_ypos_ != pm.position())
+                               LYXERR(Debug::PAINTING, "Found new anchor pit = 
" << d->anchor_pit_
+                                      << "  anchor ypos = " << 
d->anchor_ypos_);
                        d->anchor_pit_ = pit;
                        d->anchor_ypos_ = pm.position();
                        break;
                }
        }
-       LYXERR(Debug::PAINTING, "Found new anchor pit = " << d->anchor_pit_
-               << "  anchor ypos = " << d->anchor_ypos_);
+       if (!pain.isNull()) {
+               // reset the update flags, everything has been done
+               d->update_flags_ = Update::None;
+       }
 
        // Remember what has just been done for the next draw() step
        if (paint_caret)
diff --git a/src/BufferView.h b/src/BufferView.h
index a522259..3cbf22a 100644
--- a/src/BufferView.h
+++ b/src/BufferView.h
@@ -120,11 +120,12 @@ public:
        /// \return true if the BufferView is at the bottom of the document.
        bool isBottomScreen() const;
 
-       /// perform pending metrics updates.
-       /** \c Update::FitCursor means first to do a FitCursor, and to
+       /// Add \p flags to current update flags and trigger an update.
+       /* If this method is invoked several times before the update
+        * actually takes place, the effect is cumulative.
+        * \c Update::FitCursor means first to do a FitCursor, and to
         * force an update if screen position changes.
         * \c Update::Force means to force an update in any case.
-        * \retval true if a screen redraw is needed
         */
        void processUpdateFlags(Update::flags flags);
 
@@ -367,6 +368,8 @@ private:
        /// Update current paragraph metrics.
        /// \return true if no further update is needed.
        bool singleParUpdate();
+       /// do the work for the public updateMetrics()
+       void updateMetrics(Update::flags & update_flags);
 
        // Set the row on which the cursor lives.
        void setCurrentRowSlice(CursorSlice const & rowSlice);
diff --git a/src/TextMetrics.cpp b/src/TextMetrics.cpp
index c4ed9ca..0c5d8f4 100644
--- a/src/TextMetrics.cpp
+++ b/src/TextMetrics.cpp
@@ -1932,10 +1932,9 @@ void TextMetrics::drawParagraph(PainterInfo & pi, 
pit_type const pit, int const
                        string const foreword = text_->isMainText() ? "main 
text redraw "
                                : "inset text redraw: ";
                        LYXERR0(foreword << "pit=" << pit << " row=" << i
-                               << " row_selection="    << row.selection()
-                               << " full_repaint="     << pi.full_repaint
-                               << " row_has_changed="  << row_has_changed
-                               << " null painter=" << pi.pain.isNull());
+                               << (row.selection() ? " row_selection": "")
+                               << (pi.full_repaint ? " full_repaint" : "")
+                               << (row_has_changed ? " row_has_changed" : ""));
                }
 
                // Backup full_repaint status and force full repaint
diff --git a/src/update_flags.h b/src/update_flags.h
index 3e877c1..a40e88c 100644
--- a/src/update_flags.h
+++ b/src/update_flags.h
@@ -21,13 +21,16 @@ namespace Update {
                /// Recenter the screen around the cursor if is found outside 
the
                /// visible area.
                FitCursor = 1,
-               /// Force a full screen metrics update.
+               /// Force a full screen metrics update and a full draw.
                Force = 2,
+               /// Force a full redraw (but no metrics computations)
+               ForceDraw = 4,
                /// Try to rebreak only the current paragraph metrics.
-               SinglePar = 4,
+               /// (currently ignored!)
+               SinglePar = 8,
                /// Only the inset decorations need to be redrawn, no text 
metrics
                /// update is needed.
-               Decoration = 8
+               Decoration = 16
        };
 
 inline flags operator|(flags const f, flags const g)
@@ -40,6 +43,11 @@ inline flags operator&(flags const f, flags const g)
        return static_cast<flags>(int(f) & int(g));
 }
 
+inline flags operator~(flags const f)
+{
+       return static_cast<flags>(~int(f));
+}
+
 } // namespace Update
 
 } // namespace lyx

Reply via email to