Re: Question about display engine

[Ergus]

On Fri, Sep 06, 2019 at 11:55:19AM +0300, Eli Zaretskii wrote:
> > Date: Thu, 5 Sep 2019 19:26:09 +0000 (UTC)
> > From: Ergus <address@hidden>
> > Cc: address@hidden, address@hidden
> >
> > I attach here a new patch with all the changes I have just made. After fixing 
> > the latest Martin's issue there was
> > exposed a new issue maybe related with the initialization per line. (picture 
> > attached)
>
> Thanks.  I admit that I'm confused by your patch: I don't understand
> your design of calculating and applying the face used for EOL
> extension.  E.g., where's the code that merges only the non-extensible
> attributes of the face at buffer position and assigns that face ID of
> the calculated face to it->extend_face_id?

1) This is something confusing even for me. because I was doing that the
other way around as in my initial mental scheme the "extensible" part of
the face should be associated (somehow, contained or mapped) with every
extensible face || else 0 (DEFAULT_FACE_ID). So it will be easy to check
that and go forth and back at EOL.

> And why did you copy all
> the lines that assign to it->face_id with similar lines that assign
> something similar (sometimes identical) to it->extend_face_id?

2) This was a desperate try to check where I was caching something
wrong. In some cases it will be needed to do that because there are
points were the face_id is set and reset latter and it is faster to
cache also the extend_face_id instead of calculating it because there is
not conversion method. (explication at the end)

> And
> why do you have to save and restore extend_face_id during some
> operations, like we do with it->face_id?  Etc. etc.

3) Same than above. Hopefully this will be removed.

For some reason before Martin faced the issue he reported on yesterday,
the code was somehow working for me more or less as expected. So it
seems to be that something was not initialized and hiding the bigger
issues in my part.

> Can you post a description of the design and the implementation, to
> help me find the light here?  In particular, I don't think I
> understand the meaning of "the face should be extended after EOL", if
> that face is merged with other faces to realize the face to be
> actually used in display.  This semantics seems not to be explained
> anywhere, so it's hard to judge whether the implementation satisfies
> the requirements/expectations.
4) Sorry for that.

the face should be extended after EOL means (somehow) that the
attributes specified in the face are merged with the ones in other
extensible faces to extend after EOL. The conditional to merge (probably
wrong) is in merge_extend_glyph_face in xdisp.c.

> > The issue is actually related with the fact that extend_face_id is never 
> > restarted to face_id when going back
> > from an extend to a non_extend face between lines (for example when mark is 
> > active and the iterator crosses
> > the pointer to outside the selected region like in the  picture).
>
> I cannot answer this question because my mental model is the opposite:
> that the code should temporarily set face_id to be equal to
> extend_face_id when producing glyphs beyond EOL, then reset face_id
> back to its previous value.  But your code doesn't fit this mental
> model of mine, so I'm probably missing something.

5) This is actually what happens more or less. in exted_face_to_end... I
set the face_id = extend_face_id and then I reset it back at the end (as
usual).

The issue in my code is probably that I am not calculating the
extend_face_id at EOL correctly. (Mainly because I know I am missing
something crucial about where and how to do that.)

Actually extend_face_id; once it is set to an extensible face it only
merges forth and forth... so even after the region finishes it never
resets.... which is actually wrong (my bad)... but I don't know where
this happen for the face_id; that's why I was reassigning everywhere to
see if I could find it.

> > I made all the  updates of the extend_face_id in the same places where face_id 
> > was updated (even when
> > uneeded for now.)
>
> This is another place for confusion: I don't understand why
> extend_face_id should be updated in all those places.  In my metal
> model, extend_face_id is independent of many/most of the factors that
> cause us update face_id.
I know. I just couldn't find a condition... sorry for that.

> > But I don't see any special function that is called before
> > display_line.
>
> face_id is initialized in init_iterator, which is always called once
> before the first call to display_line.  Thereafter, any subsequent
> call to display_line "inherits" the value of face_id left in the
> iterator object at the end of the previous call to display_line.
I understood this later actually.
> Whether this fits the logic of using extend_face_id, I cannot say yet;
> see the above questions that describe my confusion.
It actually does... but when the it->face_id changes (for example the
region ends in the middle of a line) the extend_face_id should know. 

> Thanks for working on this.

I am actually rethinking the whole code... but I need to understand
better some details that are unclear for me. Like how to get the
"extensible" face_id from a non extensible mixed merged face. Lets say

e = (a + b + c + d) where only a and c were extensible. Because if I don't
have a cache/face I will need to recalculate that every time and find a
way to remember how a face was composed... (remember that e was composed
by a; b; c; d and then iterate over those ids, get_face_from_id and do a
loop that if EXTENSIBLE-P will merges in extend_face_id. This will be
sub efficient.

The other problems I see with this is that in general after several
merges the resulting face_id could be the same for different sequences
of a, b, c, d, f, g, r. So the relation face_id -> extend_face_id is not
even injective; as we lost information in the middle.

The simplest case: suppose that we have (h == b) but h is extensible and
b is not. they both will have different face_id because the vectors are
different.

Merging (a + b + c + d) == (a + h + c + d) -> same face id
but the extensible faces (a + c) != (a + h + c) -> different face_id

So I don't know how to face this if I want to do it at the EOL
only. Because of that I was somehow searching for a method that could
give me (a + h + c) or (a + c) on the fly every time... but this seems
to be wrong implemented; so I need MORE help here.