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
