Re: Understanding the painting process (André, Abdel, Lars, please read)
On 03/10/2015 23:46, Jean-Marc Lasgouttes wrote: 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... Sure. I do not understand your point here. And currently something is wrong: http://www.lyx.org/trac/ticket/9756 In this ticket you say pm is buffer dependent, this should not be the case. Paragraphs are Buffer dependent and pms are bv dependent, this is my point. Obviously there is a bug here, is it only for SplitView? Or also for mutiple windows? 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. MathInset should not be special in theory but, AFAIR, MathInset mixes up the contents and the metrics and it was too difficult to split that like for Paragraph and PM. This was also not fully necessary because MathInsets do not adapt they form depending on the bv size. In any case, I don't see why pm could not contains dimensions and positions for inner math insets as well. A math inset is always inside a paragraph or another math inset. * 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. AFAIU the only difference between the global bv cache and what I am proposing is that the bv will ask its tm which will ask its pm which will ask its inset metrics. But maybe I don't follow you. 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). My point is that each insets or paragraph should know if something has changed inside it. SingleParUpdate is a top->down mechanism because the LFUN mechanism is always top down. I guess it should be possible for each tm to ask its pms if it needs to be recalculated. That's why I think the global SingleParUpdate flag should not necessary. 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. Buffer::changed(bool) means that buffer contents and only that. And what does it means to have a buffer which contents is changed without having updated metrics? I don't remember exactly... maybe that this change is not visible and thus doesn't need new metrics? 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. With the pre-computed pm you don't have to compute again if you stay within the metrics limit of this paragraph when scrolling a bit. Abdel.
Re: Understanding the painting process (André, Abdel, Lars, please read)
Le 04/10/2015 12:07, Abdelrazak Younes a écrit : And currently something is wrong: http://www.lyx.org/trac/ticket/9756 In this ticket you say pm is buffer dependent, this should not be the case. Paragraphs are Buffer dependent and pms are bv dependent, this is my point. You are right, I indicated this in the ticket. AFAIU the only difference between the global bv cache and what I am proposing is that the bv will ask its tm which will ask its pm which will ask its inset metrics. But maybe I don't follow you. Actually with the new code, the row object contains the dimension of the insets it contains, so that it is not really necessary to have a per pm cache. With the bv::coordCache system, an inset can now what is its dimension in a buffer view, without knowing where it is in the document. This is therefore more powerful than a per-pm cache. My point is that each insets or paragraph should know if something has changed inside it. SingleParUpdate is a top->down mechanism because the LFUN mechanism is always top down. I guess it should be possible for each tm to ask its pms if it needs to be recalculated. That's why I think the global SingleParUpdate flag should not necessary. I see. I am not sure how to do it though. 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. Buffer::changed(bool) means that buffer contents and only that. And then, what is the expected action? And what does it means to have a buffer which contents is changed without having updated metrics? I don't remember exactly... maybe that this change is not visible and thus doesn't need new metrics? In this case, I would say that nothing needs to be done. JMarc
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: 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
Re: Understanding the painting process (André, Abdel, Lars, please read)
Le 02/10/2015 14:38, Jean-Marc Lasgouttes a écrit : Hello, 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. Thanks for the documentation effort -- 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!!! This file and the warning should be referred to at the beginning of relevant .cpp and .h files IMO. Guillaume
