Re: Understanding the painting process (André, Abdel, Lars, please read)
On 02/10/2015 15:38, Jean-Marc Lasgouttes wrote: Hello, After a gentle push by Guillaume and a boring meeting, I decided to actually read the code that handles metrics computing and contents drawing. The result is the document below, which is available in development/PAINTING_ANALISYS. It starts with questions that I had when writing the other sections of the document. I'd e glad if some of our old timers (and all current devs too, of course) could find time to read it and probe their brain to answer some of them. We cannot remain in a situation where we do not really know who paints what and when. Will try... Have a nice week-end. JMarc -- 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. pm is bv dependent; so it should nicely adapt to it containing bv. I think you should keep pm::insetDimension and remove bv::coordCache completely instead. Design should be in the end like this: * bv contains one tm * tm contains multiple pm (tm can refer to the top InsetText or not) * pm contains multiple insetDimension, one per inset in the paragraph. ** 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. SingleParUpdate would not be necessary if all elements did their job correctly. I also guess that SingleParUpdate assumes that there is only one InsetText, which is of course not true. We should aim to remove this flag. Moreover, I fail to see (yet) where the 'single' part of the program is acted on. Maybe in some mouse triggered action... ** 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). What you say seems right, maybe it was not possible to that at the time. ** 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? That the buffer contents and only the content content is changed. I guess this signal is abused for some other purpose. ** Metrics outside of visible area Why is it required to compute metrics on page above and below the visible area? I think this was for scroll optimization, the idea is to not to compute the full buffer metrics in case a small scrolling is needed. Couldn't this be done on demand? I suspect this could be made transparent by doing it in the proper metrics-fetching method. Maybe yes. You would have to assess if the on-demand computation does not slow down the minimal scrolling case. I remember Tomaso wanted to compute all bv metrics in one go to improve the scrolling time; computing small amount of metrics above and below visible area was seen as a good subjective compromise I guess. ** 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? Maybe for Mouse Over? In order to draw the visible hints like the corners around the insets or the completion arrow? ** 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? We should, definitely. And I think this was my plan actually at the time: * transfer all the logic of bv::updateMetrics to tm. * Main InsetText should not be special. * Two phase drawing There are two parts to drawing the work area: + the metrics phase computes the size of insets
Re: Size of box insets
Le 03/10/2015 01:27, Pavel Sanda a écrit : Jean-Marc Lasgouttes wrote: Why not do it in InsetText::Metrics directly? Is it a box-only problem? Hmm, is there another insettext which allows fixing to certain percentage of screen size width? Wrapping float came to my mind but apparently it does not try to visualize the width set. No other inset came to my mind. You should not have to care about it. The code in InsetText::metrics has provision for too narrow inset, it can be improved for all cases IMO. Note that all our explicit pixel counts should be updated to take zoom into account, or even current font em size (think retina display). JMarc
Re: Understanding the painting process (André, Abdel, Lars, please read)
Le 02/10/2015 21:10, Guillaume Munch a écrit : Please keep this file up to date as the code evolves!!! This file and the warning should be referred to at the beginning of relevant .cpp and .h files IMO. I do not think that people read these headers anyway. In any case, this file is a moving target for now. When it stabilizes to something, we'll decide where to move it. JMarc
Re: Understanding the painting process (André, Abdel, Lars, please read)
Le 03/10/2015 11:50, Abdelrazak Younes a écrit : I propose to get rid of pm::insetDimension. pm is bv dependent; so it should nicely adapt to it containing bv. I would think that bv::cordCache is bv dependent too... I do not understand your point here. And currently something is wrong: http://www.lyx.org/trac/ticket/9756 I think you should keep pm::insetDimension and remove bv::coordCache completely instead. Design should be in the end like this: * bv contains one tm * tm contains multiple pm (tm can refer to the top InsetText or not) * pm contains multiple insetDimension, one per inset in the paragraph. Note that: * coordcache also contains dimensions and positions for inner math insets. * having a global bv coordinate cache allows to know where a mouse click lands (hovering, clicking...). It is doable with bv>tm>pm too, but this defeats the idea of a good old cache. SingleParUpdate would not be necessary if all elements did their job correctly. I also guess that SingleParUpdate assumes that there is only one InsetText, which is of course not true. We should aim to remove this flag. In some cases, I still believe that it makes sense for the code to indicate that only the current paragraph has been changed. This is something that the code cannot guess without rebreaking the paragraph (which is expensive). Moreover, I fail to see (yet) where the 'single' part of the program is acted on. Maybe in some mouse triggered action... I'll dig more. ** 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). What you say seems right, maybe it was not possible to that at the time. Now I understand how it works: inset size needs to be computed in a inner to outer way: each inset size depends on the size of the insets that it contains. OTOH the position of an inset depends on the position of its enclosing inset. Here we need a second loop that goes in the opposite direction. So at least for "normal" insets, setting position requires a bit of code but is doable. On a related note, what is the semantics of a call to Buffer::changed(false)? What does the caller mean? That the buffer contents and only the content content is changed. I guess this signal is abused for some other purpose. I cannot parse your first sentence. And what does it means to have a buffer which contents is changed without having updated metrics? Why is it required to compute metrics on page above and below the visible area? I think this was for scroll optimization, the idea is to not to compute the full buffer metrics in case a small scrolling is needed. This I agree with. Couldn't this be done on demand? I suspect this could be made transparent by doing it in the proper metrics-fetching method. Maybe yes. You would have to assess if the on-demand computation does not slow down the minimal scrolling case. I do not really see how it could slow it down. But implementing that is maybe not that easy. Currently I am trying to (1) understand how things work and (2) see what simple fixes can be proposed to improve the situation. I'll add that to the file. ** 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? Maybe for Mouse Over? In order to draw the visible hints like the corners around the insets or the completion arrow? No, decorations are handled separately. But I see now that the buffer.changed has been used for horizontal scrolling %-| I'll have to investigate whether it was necessary. [...] This all seems correct... but last time I touch this code was more than 7 or 8 years ago so please take my comments with caution. Personnally, I never really touched that code. This is a very nice documentation effort in any case, thanks for that! You're welcome. Thanks for the comments. JMarc
