Hello,
The following patches are a backport of the recent patch series which
added the possibility of knowing which paragraph metrics have a computed
position and then fixed situations where the assertion added in this
first patch was triggered.
Adding a new way to assert does not sound good in a stable series, BUT
1/ the assertion is not active in release code (this is not a real crash)
2/ I propose to add 3 more patches which addressed the known assertions.
Moreover, compared to the patch in master where a bad position will be
returned in release code as -10000, the backported code will return -1
as it used to do.
The 3 additional patches are harmless IMO, since all they do is add
checks, add update operations, or remove a SinglePar optimization hint.
Riki, would that be OK?
JMarc
From 24dbdfa68b64d8b574cd39d459fca4296314fb4a Mon Sep 17 00:00:00 2001
From: Jean-Marc Lasgouttes <lasgout...@lyx.org>
Date: Fri, 17 May 2024 15:42:08 +0200
Subject: [PATCH 1/4] Handle metrics of not visible paragraphs
The code is not ready for situations where some paragraphs that are
not visible have metrics available.
In PararagraphMetrics, some methods are added to be able to handle the
fact that paragraphs have or do not have a position.
In TextMetrics, a new method returns the first visible paragraph.
Finally, in BufferView::updateMetrics, the paragraphs' positions are
reset (in the case where everything is not cleared) and some care is
taken to skip the ones that are not relevant.
The assumption outside of this method is that all the paragraphs that
are in the TextMetrics are visible (we are talking about top-level
TextMetrics here). This could be changed (in order to avoid
recomputing paragraph metrics), but the cost is high in terms of
complexity and it is not clear that the gain in terms of performance
would be important.
NOTE: contrary to the code in master which returns npos = -10000, this
code still returns -1 when the position of a paragraph is unknown.
(cherry picked from commit 145af7c2ac1eb2c5ec5102a7a11cb740be7b3c43)
(cherry picked from commit 05bb851adfb733c942d243800d7151c6b9194645)
(cherry picked from commit 8bc3799b354908f22270f9d96b2cce43ebd96d66)
---
src/BufferView.cpp | 11 +++++++----
src/ParagraphMetrics.cpp | 24 ++++++++++++++++++++++--
src/ParagraphMetrics.h | 6 +++++-
src/TextMetrics.cpp | 6 +++++-
4 files changed, 39 insertions(+), 8 deletions(-)
diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 1b2752e638..ec77a81c46 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -3197,6 +3197,7 @@ void BufferView::updateMetrics(bool force)
d->text_metrics_.clear();
}
+ // This should not be moved earlier
TextMetrics & tm = textMetrics(&buftext);
// make sure inline completion pointer is ok
@@ -3212,14 +3213,16 @@ void BufferView::updateMetrics(bool force)
// Check that the end of the document is not too high
int const min_visible = lyxrc.scroll_below_document ? minVisiblePart() : height_;
- if (tm.last().first == lastpit && tm.last().second->bottom() < min_visible) {
+ if (tm.last().first == lastpit && tm.last().second->hasPosition()
+ && tm.last().second->bottom() < min_visible) {
d->anchor_ypos_ += min_visible - tm.last().second->bottom();
LYXERR(Debug::SCROLLING, "Too high, adjusting anchor ypos to " << d->anchor_ypos_);
tm.updateMetrics(d->anchor_pit_, d->anchor_ypos_, height_);
}
// Check that the start of the document is not too low
- if (tm.first().first == 0 && tm.first().second->top() > 0) {
+ if (tm.first().first == 0 && tm.first().second->hasPosition()
+ && tm.first().second->top() > 0) {
d->anchor_ypos_ -= tm.first().second->top();
LYXERR(Debug::SCROLLING, "Too low, adjusting anchor ypos to " << d->anchor_ypos_);
tm.updateMetrics(d->anchor_pit_, d->anchor_ypos_, height_);
@@ -3232,11 +3235,11 @@ void BufferView::updateMetrics(bool force)
* extra paragraphs are removed
*/
// Remove paragraphs that are outside of screen
- while(tm.first().second->bottom() <= 0) {
+ while(!tm.first().second->hasPosition() || tm.first().second->bottom() <= 0) {
//LYXERR0("Forget pit: " << tm.first().first);
tm.forget(tm.first().first);
}
- while(tm.last().second->top() > height_) {
+ while(!tm.last().second->hasPosition() || tm.last().second->top() > height_) {
//LYXERR0("Forget pit: " << tm.first().first);
tm.forget(tm.last().first);
}
diff --git a/src/ParagraphMetrics.cpp b/src/ParagraphMetrics.cpp
index 31b31a2d01..5bf7cb1d43 100644
--- a/src/ParagraphMetrics.cpp
+++ b/src/ParagraphMetrics.cpp
@@ -40,9 +40,10 @@ using namespace lyx::support;
namespace lyx {
+const int pm_npos = -10000;
ParagraphMetrics::ParagraphMetrics(Paragraph const & par) :
- position_(-1), id_(par.id()), par_(&par)
+ position_(pm_npos), id_(par.id()), par_(&par)
{}
@@ -61,7 +62,14 @@ void ParagraphMetrics::reset(Paragraph const & par)
{
par_ = ∥
dim_ = Dimension();
- //position_ = -1;
+ //position_ = pm_npos;
+}
+
+
+int ParagraphMetrics::position() const
+{
+ LASSERT(hasPosition(), return -1);
+ return position_;
}
@@ -71,6 +79,18 @@ void ParagraphMetrics::setPosition(int position)
}
+void ParagraphMetrics::resetPosition()
+{
+ position_ = pm_npos;
+}
+
+
+bool ParagraphMetrics::hasPosition() const
+{
+ return position_ != pm_npos;
+}
+
+
Row const & ParagraphMetrics::getRow(pos_type pos, bool boundary) const
{
LBUFERR(!rows().empty());
diff --git a/src/ParagraphMetrics.h b/src/ParagraphMetrics.h
index 805d056541..b572f122b5 100644
--- a/src/ParagraphMetrics.h
+++ b/src/ParagraphMetrics.h
@@ -70,8 +70,12 @@ public:
bool hfillExpansion(Row const & row, pos_type pos) const;
/// The vertical position of the baseline of the first line of the paragraph
- int position() const { return position_; }
+ int position() const;
void setPosition(int position);
+ /// Set position to unknown
+ void resetPosition();
+ /// Return true when the position of the paragraph is known
+ bool hasPosition() const;
/// The vertical position of the top of the paragraph
int top() const { return position_ - dim_.ascent(); }
/// The vertical position of the bottom of the paragraph
diff --git a/src/TextMetrics.cpp b/src/TextMetrics.cpp
index 9979909f28..57e944455f 100644
--- a/src/TextMetrics.cpp
+++ b/src/TextMetrics.cpp
@@ -220,7 +220,10 @@ void TextMetrics::updateMetrics(pit_type const anchor_pit, int const anchor_ypos
int const bv_height)
{
LASSERT(text_->isMainText(), return);
- pit_type const npit = pit_type(text_->paragraphs().size());
+
+ // Forget existing positions
+ for (auto & pm_pair : par_metrics_)
+ pm_pair.second.resetPosition();
if (!contains(anchor_pit))
// Rebreak anchor paragraph.
@@ -246,6 +249,7 @@ void TextMetrics::updateMetrics(pit_type const anchor_pit, int const anchor_ypos
int y2 = anchor_ypos + anchor_pm.descent();
// We are now just below the anchor paragraph.
pit_type pit2 = anchor_pit + 1;
+ pit_type const npit = pit_type(text_->paragraphs().size());
for (; pit2 < npit && y2 < bv_height; ++pit2) {
if (!contains(pit2))
redoParagraph(pit2);
--
2.34.1
From 6f61b0b4318e435838a29ecdd6775d8ffbb9400a Mon Sep 17 00:00:00 2001
From: Jean-Marc Lasgouttes <lasgout...@lyx.org>
Date: Wed, 3 Jul 2024 00:22:55 +0200
Subject: [PATCH 2/4] Make sure paragraph positions are updated when scrolling
Sometimes quick selection-scrolling could cause a crash because the
position of some paragraphs is not computed. To fix that, in
BufferView::showCursor, make sure that the metrics are always kept
clean using updateMetrics(false), which is lighweight.
As a consequence, the 'update' parameter of showCursor and
scrollDocView is not needed anymore. Its removal is mechanical and
accounts for most of this commit.
The only other significant change is that, when creating synthetic
mouse events and relying on scroll() for small moves, the full metrics
recomputation is replaced by the lighter version.
More work is still to come on this code, but this should be going in
the right direction.
(cherry picked from commit 6e0ea4269ae792225bb4e0d0f0ffcb3236c3c5c9)
---
src/BufferView.cpp | 21 ++++++++++-----------
src/BufferView.h | 5 ++---
src/frontends/qt/GuiWorkArea.cpp | 6 +++---
3 files changed, 15 insertions(+), 17 deletions(-)
diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index ec77a81c46..428c03299f 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -755,7 +755,7 @@ Inset const * BufferView::mathContextMenu(InsetMathNest const * inset,
}
-void BufferView::scrollDocView(int const pixels, bool update)
+void BufferView::scrollDocView(int const pixels)
{
// The scrollbar values are relative to the top of the screen, therefore the
// offset is equal to the target value.
@@ -778,7 +778,7 @@ void BufferView::scrollDocView(int const pixels, bool update)
// cut off at the top
if (pixels <= d->scrollbarParameters_.min) {
DocIterator dit = doc_iterator_begin(&buffer_);
- showCursor(dit, SCROLL_VISIBLE, update);
+ showCursor(dit, SCROLL_VISIBLE);
LYXERR(Debug::SCROLLING, "scroll to top");
return;
}
@@ -787,7 +787,7 @@ void BufferView::scrollDocView(int const pixels, bool update)
if (pixels >= d->scrollbarParameters_.max) {
DocIterator dit = doc_iterator_end(&buffer_);
dit.backwardPos();
- showCursor(dit, SCROLL_VISIBLE, update);
+ showCursor(dit, SCROLL_VISIBLE);
LYXERR(Debug::SCROLLING, "scroll to bottom");
return;
}
@@ -806,14 +806,14 @@ void BufferView::scrollDocView(int const pixels, bool update)
// It seems we didn't find the correct pit so stay on the safe side and
// scroll to bottom.
LYXERR0("scrolling position not found!");
- scrollDocView(d->scrollbarParameters_.max, update);
+ scrollDocView(d->scrollbarParameters_.max);
return;
}
DocIterator dit = doc_iterator_begin(&buffer_);
dit.pit() = i;
LYXERR(Debug::SCROLLING, "pixels = " << pixels << " -> scroll to pit " << i);
- showCursor(dit, SCROLL_VISIBLE, update);
+ showCursor(dit, SCROLL_VISIBLE);
}
@@ -999,21 +999,20 @@ int BufferView::workWidth() const
void BufferView::recenter()
{
- showCursor(d->cursor_, SCROLL_CENTER, true);
+ showCursor(d->cursor_, SCROLL_CENTER);
}
void BufferView::showCursor()
{
- showCursor(d->cursor_, SCROLL_VISIBLE, true);
+ showCursor(d->cursor_, SCROLL_VISIBLE);
}
-void BufferView::showCursor(DocIterator const & dit, ScrollType how,
- bool update)
+void BufferView::showCursor(DocIterator const & dit, ScrollType how)
{
- if (scrollToCursor(dit, how) && update)
- processUpdateFlags(Update::Force);
+ if (scrollToCursor(dit, how))
+ processUpdateFlags(Update::ForceDraw);
}
diff --git a/src/BufferView.h b/src/BufferView.h
index b5439b7036..ca2d7bee77 100644
--- a/src/BufferView.h
+++ b/src/BufferView.h
@@ -219,8 +219,7 @@ public:
/// This method will automatically scroll and update the BufferView
/// (metrics+drawing) if needed.
/// \param how Use this scroll strategy
- /// \param force If true, update screen after scrolling
- void showCursor(DocIterator const & dit, ScrollType how, bool update);
+ void showCursor(DocIterator const & dit, ScrollType how);
/// Scroll to the cursor.
/// \param how Use this scroll strategy
/// \return true if screen was scrolled
@@ -232,7 +231,7 @@ public:
/// scroll document by the given number of pixels.
int scroll(int pixels);
/// Scroll the view by a number of pixels.
- void scrollDocView(int pixels, bool update);
+ void scrollDocView(int pixels);
/// Set the cursor position based on the scrollbar one.
void setCursorFromScrollbar();
diff --git a/src/frontends/qt/GuiWorkArea.cpp b/src/frontends/qt/GuiWorkArea.cpp
index 8690b6f4f0..7593d4a0c7 100644
--- a/src/frontends/qt/GuiWorkArea.cpp
+++ b/src/frontends/qt/GuiWorkArea.cpp
@@ -588,7 +588,7 @@ void GuiWorkArea::Private::updateScrollbar()
void GuiWorkArea::scrollTo(int value)
{
stopBlinkingCaret();
- d->buffer_view_->scrollDocView(value, true);
+ d->buffer_view_->scrollDocView(value);
if (lyxrc.cursor_follows_scrollbar) {
d->buffer_view_->setCursorFromScrollbar();
@@ -961,9 +961,9 @@ void GuiWorkArea::generateSyntheticMouseEvent()
// Scroll
if (step <= 2 * wh) {
d->buffer_view_->scroll(up ? -step : step);
- d->buffer_view_->updateMetrics();
+ d->buffer_view_->processUpdateFlags(Update::ForceDraw);
} else {
- d->buffer_view_->scrollDocView(value + (up ? -step : step), false);
+ d->buffer_view_->scrollDocView(value + (up ? -step : step));
}
// In which paragraph do we have to set the cursor ?
--
2.34.1
From 0e0fee9ad1715069d1416407367e9be8894d4728 Mon Sep 17 00:00:00 2001
From: Jean-Marc Lasgouttes <lasgout...@lyx.org>
Date: Wed, 3 Jul 2024 17:31:38 +0200
Subject: [PATCH 3/4] Remove useless SinglePar update when scrolling in text
Using SinglePar does not majke sense here since the paragraph is not
modified and it might even not have a position yet.
This fixes a crash in BufferView::singleParUpdate, which is not
prepared (yet) to such situations.
(cherry picked from commit 2bdd691130e5507fc5cc2ca81e71eef189e90fc5)
---
src/Text.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/Text.cpp b/src/Text.cpp
index 687144ce50..d314c251ae 100644
--- a/src/Text.cpp
+++ b/src/Text.cpp
@@ -5343,7 +5343,7 @@ void Text::dispatch(Cursor & cur, FuncRequest & cmd)
bvcur.setCurrentFont();
if (cur.top() == old) {
// We didn't move one iota, so no need to update the screen.
- cur.screenUpdateFlags(Update::SinglePar | Update::FitCursor);
+ cur.screenUpdateFlags(Update::FitCursor);
//cur.noScreenUpdate();
return;
}
--
2.34.1
From 6cf99a5fa60fc4014837e368e77379f5444d5372 Mon Sep 17 00:00:00 2001
From: Jean-Marc Lasgouttes <lasgout...@lyx.org>
Date: Mon, 1 Jul 2024 23:56:33 +0200
Subject: [PATCH 4/4] Make BufferView::singeParUpdate more robust
In some cases, it might happen that this method is called with no
metrics or position known for the current paragraph.
Take care of these cases to avoid assertions.
Remove setting of inset positions in the method, but make sure that
updateMetrics(false) is always called to get everything right.
In the new code, updateMetrics(bool) is the method that sets
everything right with minimal effort.
(cherry picked from commit 89ab9eb569ec0eea87d9a8c269eb87507934e1c5)
---
src/BufferView.cpp | 25 ++++++++++++++++---------
1 file changed, 16 insertions(+), 9 deletions(-)
diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 428c03299f..1fdfa78e2a 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -564,10 +564,12 @@ void BufferView::processUpdateFlags(Update::flags flags)
// are not already redrawing all).
// We handle this before FitCursor because the later will require
// correct metrics at cursor position.
- if (!(flags & Update::ForceDraw)
- && (flags & Update::SinglePar)
- && !singleParUpdate())
- updateMetrics(true);
+ if (!(flags & Update::ForceDraw) && (flags & Update::SinglePar)) {
+ if (singleParUpdate())
+ updateMetrics(false);
+ else
+ updateMetrics(true);
+ }
// Then make sure that the screen contains the cursor if needed
if (flags & Update::FitCursor) {
@@ -3132,6 +3134,10 @@ bool BufferView::singleParUpdate()
if (d->inlineCompletionPos_.fixIfBroken())
d->inlineCompletionPos_ = DocIterator();
+ if (!tm.contains(pit)) {
+ LYXERR(Debug::PAINTING, "SinglePar optimization failed: no known metrics");
+ return false;
+ }
/* Try to rebreak only the paragraph containing the cursor (if
* this paragraph contains insets etc., rebreaking will
* recursively descend). We need a full redraw if either
@@ -3149,18 +3155,19 @@ bool BufferView::singleParUpdate()
tm.redoParagraph(pit);
ParagraphMetrics & pm = tm.parMetrics(pit);
if (pm.height() != old_dim.height()
- || (pm.width() != old_dim.width() && old_dim.width() == tm.width())) {
+ || (pm.width() != old_dim.width() && old_dim.width() == tm.width())) {
// Paragraph height or width has changed so we cannot proceed
// to the singlePar optimisation.
- LYXERR(Debug::PAINTING, "SinglePar optimization failed.");
+ LYXERR(Debug::PAINTING, "SinglePar optimization failed: paragraph metrics changed");
return false;
}
// Since position() points to the baseline of the first row, we
// may have to update it. See ticket #11601 for an example where
// the height does not change but the ascent does.
- pm.setPosition(pm.position() - old_dim.ascent() + pm.ascent());
-
- tm.updatePosCache(pit);
+ if (pm.hasPosition())
+ pm.setPosition(pm.position() - old_dim.ascent() + pm.ascent());
+ else
+ LYXERR0("SinglePar optimization succeeded, but no position to update");
LYXERR(Debug::PAINTING, "\ny1: " << pm.top() << " y2: " << pm.bottom()
<< " pit: " << pit << " singlepar: 1");
--
2.34.1
--
lyx-devel mailing list
lyx-devel@lists.lyx.org
http://lists.lyx.org/mailman/listinfo/lyx-devel
