GNOME Bugzilla – Bug 435405
text view recreates pangolayouts all the time
Last modified: 2007-06-01 06:26:16 UTC
That is when the cursor blinks too. Caching even one layout helps A LOT.
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.
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.
typo
Humm, not as easy as I expected since the structs involved are all public and with no padding :(.
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.
Which structs are we talking about ? gtktextlayout.h, gtktextdisplay.h, etc all fall into the "semi-private without compat guarantee" class
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.
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?
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?).
Created attachment 88750 [details] [review] patch
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.
(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).
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.
Nice. Is it done from your POV? If yes, Matthias, any comments on it?
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?
I'm in favor. Do some Google code search and see who is using the API...
OK to commit the patch, without those API cuts or anything?
Fine with me. Matthias?
Fine with me, too. But please put a note about this change of semi-private api in README.in, in the 2.12 section
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 :)