[gedit-list] Re: Other comment on GtkSourceView (and the turbo patch)

[Gustavo Giráldez <gustavo giraldez gmx net>]

Hi Paolo,

(to the list members: long boring post... :-)

On Fri, 2003-02-28 at 11:37, Paolo Maggi wrote:
> Hi Gustavo.
> 
> Using test-widget I have observed the following weird behavior: 
> go toward the end of the document at line 1945. It should contain a
> "void". 
> Split the "void" into "vo id". You will observe a delay of about half a
> second before the highlight is updated.
> If you try to split a word with a bold weight, this delay disappears.
> Any idea? I think this is due to the way GtkTextView emits "expose"
> events.

It took me quite some time to fully track this down, but I got it.  Btw,
if you disable cursor blinking, the highlight is not updated until you
move the cursor or take away focus from the widget.

The problem is GtkTextLayout caches the PangoLayout (which later uses
for drawing) for the last line it's asked to construct
(gtk_text_layout_get_line_display()).  So, when inserting a space which
splits "void", the following happens:

- "insert_text" signal is emitted, and thus
gtk_text_buffer_real_insert_text is called from GtkSourceBuffer code. 
That invalidates the line where the edit happened, which in turn
produces the installation of the high priority idle validation handler
to re-validate it.
- after the GtkTextBuffer's handler has finished, we analyze the text in
update_syntax_regions() and decide that the line of modified text needs
re-highlighting, and thus install the idle worker (with lower priority
than the first_validate from text view).
- the high priority first_validate is run, re-validates the line and
caches it (in the GtkTextLayout object).
- expose_event is triggered for the text view, and the code in
gtk_source_buffer_highlight_region is executed before the text view
actually paints the text.
- that calls ensure_highlighted, which in turn calls highlight_region,
who does the actual job.
- now, since changes in color don't invalidate text (because they don't
affect size), the keyword tag is removed and we still have that old
cached PangoLayout lying around.
- the real GtkTextView handler for expose is executed, using the
incorrect PangoLayout to render the text.

So, it's a bug in GtkTextView.  I'll look into it later and try to work
out a patch for GTK+.

In the meantime if this behavior annoys people, a simple workaround is
possible: by expanding the refreshed region at the end of
update_syntax_regions to three lines (surrounding the edited one), we
make sure the one-line cache in GtkTextLayout is cleared.  I tested this
and it works.

> In general, I think we should try to improve the response time the user
> perceives while she is typing.

Absolutely agreed.  It's really annoying when you type and nothing
appears on screen.

> I think this could be achieved by highlighting text outside the idle
> worker when the user adds/deletes text in the visible area (like I did
> in my patch).

I don't understand this statement.  What do you mean by highlighting
text outside the idle worker?

> If we improve the performance of the update_syntax_regions function I
> think this could be doable in all cases. Otherwise we can do it only in
> the case adding/removing text does not modify the table of syntax
> regions.
> 
> Is it sound for you?

IMHO the function update_syntax_regions is fast enough (ugly, but fast
:-).  The problem is rebuilding the syntax tables.  I'm not sure if you
meant that?  If this is the case, the patch I told you I'm working on
which reuses the invalidated regions can help.

> There is also another bug we should try to fix (but I'm not sure how to
> fix it in an efficient way).
> In same cases we apply two or more patter tags to the same piece of
> text. 
> For example we apply two tags to "#if", the first one is applied to the
> entire word, the second one to "if" only. This is clearly a bug in the
> check_pattern function.
> A possible way to fix it is to use the same approach we used for the
> syntax tags by defining a "pattern_all" regexp (but I don't like it).
> Another way could be to use GtkTextRegion. But I'm not sure about the
> performance implications.
> The last solution, could be to write a "has_pattern_tag" function and
> apply the new pattern only if both has_pattern_tag (start_iter) an 
> has_pattern_tag (end_iter) are false. This is the easiest way to fix
> this bug but could have important performance drawbacks.
> 
> I don't know very well the performance of GtkTextRegion, in the case it
> is fast I think that using it could be the best solution.

Yeah, I've seen that myself too.  Some time ago, this was not a problem
since check_pattern tried at each character to match a pattern and
advanced when one was found.  This resulted in very poor performance, so
I changed the implementation to the one you reviewed, which for each
pattern searches the text slice for matches.

I don't think GtkTextRegion is suitable for this task, due to the
overhead it imposes.  It uses GtkTextMark to delimit regions, and while
it's great because you don't need to update the offsets yourself I don't
think it'll behave well with a huge number of subregions.

I have a fourth proposal which should be nearly as fast as the current
implementation and fix this bug.  The algorithm is more or less as
follows:

1) for all the pattern tags, do an initial regex_search recording the
matches sorted by offset; discard the patterns which don't match.
2) take the pattern with the nearest match, apply it and advance the
pointer to the ending of the match.
3) perform regex_search for all patterns which are behind the current
pointer (including the one you just used), always keeping the matches in
order; discard the patterns which don't have any more matches.
4) repeat 2) until all patterns have been discarded.

The only problem is you need to keep them in order, but otherwise you're
doing the same amount of work as the current implementation (i.e. the
number of regex_search calls should be the same, unless you have
overlapping patterns, in which case you make less calls).  I'm not sure
about the performance penalty for this.  What do you think?

Regards,
Gustavo


> 
> Lemme know what do you think.
> 
> Ciao,
> Paolo
> 
> P.S. For the member of the gedit list: we are working on a branch of
> GtkSourceView called "toward-gedit-integration". The result of this work
> will be used to implement syntax highlight in gedit.
> 
>