From 86d42bc5eb5284de4fe97e4a3257031b9d127974 Mon Sep 17 00:00:00 2001 From: Jean-Marc Lasgouttes Date: Fri, 2 Oct 2015 15:30:16 +0200 Subject: [PATCH] Add a file that describe the current state of painting This is a work in progress intended to start collective work towards improving the performance of our painting process. The intent is to make it a living document that is updated as code evolves. --- development/PAINTING_ANALYSIS | 159 ++++++++++++++++++++++++++++++++++ 1 file changed, 159 insertions(+) create mode 100644 development/PAINTING_ANALYSIS diff --git a/development/PAINTING_ANALYSIS b/development/PAINTING_ANALYSIS new file mode 100644 index 0000000000..12b9983ea8 --- /dev/null +++ b/development/PAINTING_ANALYSIS @@ -0,0 +1,159 @@ +# -*- org-mode -*- +This file tries to describe the state of the metrics/painting +mechanism, and identify the improvements that could be made. The first +section can be read alone, although the context for them is really +given in the following ones. + +Please keep this file up to date as the code evolves!!! + +Abbreviations: +bv: BufferView +pm: ParagraphMetrics +tm::TextMetrics + +* Questions / Ideas + +These questions are consequences of the description made in the +following section. Some actions are proposed. + +** Inset cache + +Why do we store inset dimensions both in bv::coordCache and in +pm::insetDimension? The later one is bad when same +buffer is shown in different views. + +I propose to get rid of pm::insetDimension. + +** SinglePar update + +The flag Update::SinglePar is set in many places but never acted on. +Instead, metrics update is skipped whenever the recorded height of +current paragraph did not change and Update::Force was not specified. +Is it better to keep that (which incurs extra work) or to condition it +on Update::SinglePar? If the first solution is kept, the flag +SingleParUpdate shall be removed. + +Moreover, I fail to see (yet) where the 'single' part of the program +is acted on. + +** Two phase drawing + +Why is it necessary to set the inset positions in the drawing phase? +It seems to me that everything can be done in the metrics phase (at +least for non-math insets). + +** Buffer::change issues + +When calling Buffer::changed outside of bv::processUpdateFlags, +how do we now that the update strategy is set correctly? It is +possible to reset the strategy at the end of bv::draw. What would be +a good value? NoScreenUpdate? + +On a related note, what is the semantics of a call to +Buffer::changed(false)? What does the caller mean? + +** Metrics outside of visible area + +Why is it required to compute metrics on page above and below the +visible area? Couldn't this be done on demand? I suspect this could be +made transparent by doing it in the proper metrics-fetching method. + +** What happens with FitCursor when the cursor is already OK? + +In this case, we invoke Buffer::change(false) with drawing disabled, +which means that the paint machinery is invoked to update inset +positions. Why is this necessary at all? + +** Merging bv::updateMetrics and tm::metrics + +While the full metrics computation tries hard to limit the number of +paragraphs that are rebroken, the version that is used for inner inset +does not try any such optimization. This can be very bad for very tall +insets. How difficult would it be to re-use the bv::updateMetrics logic? + + +* Two phase drawing + +There are two parts to drawing the work area: + + + the metrics phase computes the size of insets and breaks the + paragraphs into rows. It stores the dimension of insets (both + normal and math) in bv::coordCache, and the size of normal + insets in pm::insetDimension. + + + the drawing phase draws the contents and caches the inset + positions. Since the caching of positions is useful in itself, + there is a provision for drawing "without" drawing when the only + thing we want is to cache inset positions + (Painter::setDrawingEnabled). + + +The machinery is controlled via bv::processUpdateFlags. This method is +called at the end of bv::mouseEventDispatch and in +GuiApplication::dispatch, via the updateCurrentView method. There are +also several calls in code related to dialogs. We should evaluate +whether this is correct. + +Depending on the Update::flags passed to the method, it sets an update +strategy in (NoScreenUpdate, SingleParUpdate, FullScreenUpdate, +DecorationUpdate). It triggers a recomputation of the metrics when either: + + + Update::Force has been specified + + Update::FitCursor has been specified and there is a need to scroll + the display. + + the current paragraph, after rebreak, has the same height as in + existing metrics. Note that the Update::SinglePar flag is *never* + taken into account. + +The screen is drawn (with appropriate update strategy), except when +update flag is Update::None. + + +** Metrics computation + +This is triggered by bv::updateMetrics, which calls tm::redoParagraph for + + all visible paragraphs + + paragraph above the screen (up to one page) + + paragraphs below the screen (up to one page again) + +The paragraphs outside of the screen are required to make PageUp/Down +work. + +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. + + +** Drawing the work area. + +This is done in bv::draw. This method is triggered mainly by +Buffer::changed, which draws all the work areas that show the given buffer. + +Note that, When Buffer::changed is called outside of +bv::processUpdateFlags, it is not clear whether the update strategy +has been set to a reasonable value beforehand. + +The action depends on the update strategy: + + + NoScreenUpdate: repaint the whole screen with drawing disabled. + This is only needed to update the inset positions. I am not sure + when this is necessary, actually. This is triggered when: + - Update::FitCursor is set but the cursor is already visible. It is + not clear why something needs to be done in this case, since this + should be equivalent to Update::None. + - this is also set when update flag is Update::None, but this value + is AFAICS not acted on in the code (meaning that nothing happens + at all in this case). + + + FullScreenUpdate: repaint the whole screen. This is set when: + - updateMetrics has been invoked. + - scroll value of current row has changed (although this should not + be necessary). + + + DecorationUpdate: actually like FullScreenUpdate, although it is + intended to only update inset decorations. This triggers when: + - Update::Decoration is set (without Update::Force) as flag. + - An hovered inset is detected. + + + SingleParUpdate: only tries to repaint current paragraph in a way + that is not yet very clear to me. -- 2.39.2