After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 435405 - text view recreates pangolayouts all the time
text view recreates pangolayouts all the time
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkTextView
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2007-05-03 05:16 UTC by Behdad Esfahbod
Modified: 2007-06-01 06:26 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
some patch (9.19 KB, patch)
2007-05-03 08:26 UTC, Behdad Esfahbod
none Details | Review
some other patch (26.65 KB, patch)
2007-05-23 10:57 UTC, Yevgen Muntyan
none Details | Review
patch (25.28 KB, patch)
2007-05-24 18:19 UTC, Yevgen Muntyan
none Details | Review
patch, cleaned up (23.23 KB, patch)
2007-05-26 22:15 UTC, Yevgen Muntyan
accepted-commit_now Details | Review

Description Behdad Esfahbod 2007-05-03 05:16:54 UTC
That is when the cursor blinks too.  Caching even one layout helps A LOT.
Comment 1 Behdad Esfahbod 2007-05-03 05:18:41 UTC
I've added a test file to pango/pango-view/test-long-paragraph.txt that has a 10000 char paragraph of Persian text.  
When opening it in gedit and moving cursor around the huge paragraph, with each move or blink of cursor pango has to relayout the paragraph again and again just because gtk+ disposes layouts after rendering them.  This is very slow and you can feel.
Comment 2 Behdad Esfahbod 2007-05-03 06:38:30 UTC
Ok, it has a cache of one.  It just happens that it invalidates it on cursor visibility change.  Working on fixing it.  Seems pretty easy.
Comment 3 Behdad Esfahbod 2007-05-03 07:31:10 UTC
typo
Comment 4 Behdad Esfahbod 2007-05-03 07:36:32 UTC
Humm, not as easy as I expected since the structs involved are all public and with no padding :(.
Comment 5 Behdad Esfahbod 2007-05-03 08:26:07 UTC
Created attachment 87437 [details] [review]
some patch

Attached patch uses a stripped down version of gtk_text_layout_get_line_display() that only extracts and updates the cursors.  This removes need for invalidation on cursor visibility change.  However, on cursor movement we still get a full invalidation from the gtktextbtree layer and that doesn't seem easy to fix :(.

A much simpler aprpoach equivalent to this patch is to always create the cursors, and in gtktextdisplay.c check layout->cursors_visible before rendering them.  The drawback of that approach is that it changes semantics of public api, that is, that display->cursors is always filled regardless of cursor visibility.

Anyhow, seems like there's not much we can do to fix the cursor movement issue.  Having marks embedded into other buffer items doesn't seem to help.
Comment 6 Matthias Clasen 2007-05-03 11:37:56 UTC
Which structs are we talking about ? gtktextlayout.h, gtktextdisplay.h, etc
all fall into the "semi-private without compat guarantee" class
Comment 7 Behdad Esfahbod 2007-05-03 19:07:42 UTC
Ok, that's good to know.  GtkTextLineDisplay is the key one.  But seems like I need to add new methods to some classes to avoid invalidating the entire layout on things like cursor movement.
Comment 8 Yevgen Muntyan 2007-05-23 10:57:15 UTC
Created attachment 88657 [details] [review]
some other patch

This patch seems to fix the cursor movement issue. It introduces a new field in TextLineDisplay which tells whether cursors are invalid, and invalidates only cursors where appropriate, in layout and btree code. It adds a virtual function and new method to GtkTextLayout to invalidate only cursors; I did it because I thought backwards compatibility would be preserved. Real compatibility isn't preserved, but if someone using GtkTextLayout only calls gtk_text_layout_invalidate or gtk_text_layout_changed (as opposed to poking at the class virtual functions or expecting what and when is called), then nothing should break.
Does it make sense?
Comment 9 Yevgen Muntyan 2007-05-23 15:43:08 UTC
Actually, it also should optimize selection, since it doesn't affect text attributes (text renderer simply ignores colors for selected text, and nothing else, right?).
Comment 10 Yevgen Muntyan 2007-05-24 18:19:57 UTC
Created attachment 88750 [details] [review]
patch
Comment 11 Behdad Esfahbod 2007-05-24 18:27:39 UTC
Thanks.  Looks good, except that like my original patch, it makes a trimmed down copy of an existing function as gtk_text_layout_update_display_cursors().  I'm not sure if we can be smarter without code duplication.  If it can't be fixed, I'm fine with this going in.

Humm, and now that we are changing API semantics I think we better do this too:

"A much simpler aprpoach equivalent to this patch is to always create the
cursors, and in gtktextdisplay.c check layout->cursors_visible before rendering
them.  The drawback of that approach is that it changes semantics of public
api, that is, that display->cursors is always filled regardless of cursor
visibility."

So cursor blinks become really cheap.
Comment 12 Yevgen Muntyan 2007-05-24 18:49:33 UTC
(In reply to comment #11)
> Thanks.  Looks good, except that like my original patch, it makes a trimmed
> down copy of an existing function as gtk_text_layout_update_display_cursors(). 
> I'm not sure if we can be smarter without code duplication.  If it can't be
> fixed, I'm fine with this going in.

I don't think it can be fixed, since the code needs to iterate over all marks, and skip invisible intervals. Perhaps it can be simplified though, it's not something I tried to do.

> Humm, and now that we are changing API semantics I think we better do this too:
> 
> "A much simpler aprpoach equivalent to this patch is to always create the
> cursors, and in gtktextdisplay.c check layout->cursors_visible before rendering
> them.  The drawback of that approach is that it changes semantics of public
> api, that is, that display->cursors is always filled regardless of cursor
> visibility."

It won't work since there are visible marks, and selections. I.e. it'd be possible to optimize cursor blinking specifically in this way; also btree can actually tell layout what mark has hidden/shown and where; but it'd make code even more clumsy and it's ot clear if it's worth it (you can actually feel that not recreating layout helps; with cursors it's not that clear).
Comment 13 Yevgen Muntyan 2007-05-26 22:15:39 UTC
Created attachment 88869 [details] [review]
patch, cleaned up

There is still copy of gtk_text_layout_update_display_cursors(), though simplified (so it seems clear what it's doing). I looked at gtk_text_layout_update_display_cursors(), and couldn't make it update-cursor-only-if-needed, there is way too much stuff in it.

What I also would like to do is to cache more than one line. When you move mouse over the widget, textview wants the display cache for the line under the pointer; so for instance if you have mouse over one line and cursor in another line, then moving mouse will do constant re-creating one_display_cache thing, because cursor blinker wants the cache too. But it's a separate problem, of course.
Comment 14 Behdad Esfahbod 2007-05-28 06:25:13 UTC
Nice.  Is it done from your POV?  If yes, Matthias, any comments on it?
Comment 15 Yevgen Muntyan 2007-05-28 06:42:59 UTC
I have a wonderful idea: make (almost) everything private. GnomeCanvas only accesses GtkTextLayout.default_style, and few methods. In particular it doesn't use gtk_text_layout_invalidate(). So, e.g. it's possible to make gtk_text_layout_invalidate() accept additional parameters and get rid of gtk_text_layout_invalidate_cursors() nonsense without breaking canvas. How much private is this semi-private API?
Comment 16 Behdad Esfahbod 2007-05-28 18:52:09 UTC
I'm in favor.

Do some Google code search and see who is using the API...
Comment 17 Yevgen Muntyan 2007-05-30 10:11:19 UTC
OK to commit the patch, without those API cuts or anything?
Comment 18 Behdad Esfahbod 2007-05-30 15:21:42 UTC
Fine with me.  Matthias?
Comment 19 Matthias Clasen 2007-05-30 15:29:20 UTC
Fine with me, too.

But please put a note about this change of semi-private api in README.in,
in the 2.12 section
Comment 20 Yevgen Muntyan 2007-06-01 06:26:16 UTC
2007-06-01  Yevgen Muntyan  <muntyan@tamu.edu>

	Avoid recreating pangolayouts in GtkTextView on cursor movement
	(#435405, Behdad Esfahbod).

	* gtk/gtktextlayout.c:
	* gtk/gtktextlayout.h: new GtkTextLayout method invalidate_cursors(),
	and functions gtk_text_layout_invalidate_cursors() and
	gtk_text_layout_cursors_changed(), to use when invalidation is due
	to moved marks or changed selection.

	* gtk/gtktextbtree.c:
	* gtk/gtktextbtree.h: use what's appropriate when invalidating layout.

	* gtk/gtk.symbols: add new functions.

	* README.in: added a note about changed GtkTextLayout API.

API break goes to block cursor bug :)