GNOME Bugzilla – Bug 755416
Be able to subclass GtkTextTag cleanly
Last modified: 2015-10-04 02:25:02 UTC
Outside GTK+, adding new properties to GtkTextTag is sometimes desirable. But creating a subclass of GtkTextTag is currently not possible cleanly when it comes to send the GtkTextTagTable::tag-changed signal. Currently GtkTextTag and GtkTextTagTable are tightly coupled internally. The GtkTextTagTable::tag-changed signal is actually emitted by GtkTextTag, because a tag knows the tag table where it is included. I'll attach two patches. The first fixes the design problem but performances are a bit degraded. The second patch improves performances by avoiding sending the signal unnecessarily when several properties are set at once.
Created attachment 311861 [details] [review] texttag: add a ::changed signal Before this commit, GtkTextTag emitted itself the GtkTextTagTable::tag-changed signal. This signal needs to be sent so that the GtkTextView updates the layout and/or drawing when a tag property changes. This commit adds a ::changed signal to GtkTextTag. GtkTextTagTable listens to that signal and emits ::tag-changed. This change permits to subclass GtkTextTag cleanly (without hacks and with an API that is consistent with GtkTextTag). GtkTextTag was too tightly coupled with GtkTextTagTable. More precisely, the tag->priv->table is set by gtk_text_tag_table_add(). That's how a GtkTextTag knows its tag table. On the other hand, a subclass of GtkTextTag cannot do that, or it would need to take the tag table as a parameter for example. But, as a general principle, an object should not know who contains it. It's better to not expose that in an API.
Created attachment 311862 [details] [review] texttag: reduce the number of ::changed signals emitted There was a FIXME comment about it, so let's fix it. When g_object_set() is used to set several properties at the same time, GObject freezes the notify signal. When the notify signal is thawed, the ::changed signal is emitted (only once).
Note: with the second patch, bug #755413 is more often triggered.
Review of attachment 311861 [details] [review]: Tight coupling like the one you badmouth here is often for a reason... this patch doubles the signal connection and emission overhead of using tags...
Changing tag properties is a quite rare operation. Most tags properties are set at creation time and never change afterwards, and when the feature is no longer needed the tag is removed. And when a tag property is changed, it's generally after a user event like pressing a button. Examples: changing the color scheme, disabling highlighting of search matches, etc. When performance can be a problem is when doing an animation, like changing the color every frame. But I think GObject is able to send one more signal every 16ms. Actually I don't know why optimizing this is useful, the second patch is probably not very useful, and the FIXME comment was not really worth fixing it.
Do you prefer adding a gtk_text_tag_get_tag_table() function? That way: 1. a subclass of GtkTextTag can easily send GtkTextTagTable signals. 2. the subclass can have the same API as GtkTextTag, there is no need to pass the tag table as a parameter to the constructor for example.
Or, add a gtk_text_tag_changed() function that sends the GtkTextTagTable::tag-changed signal. I prefer that solution, it's better to have smarter classes than dumb data classes with only setters and getters. To keep related data and behavior in one place.
(In reply to Sébastien Wilmet from comment #5) > Changing tag properties is a quite rare operation. Most tags properties are > set at creation time and never change afterwards, and when the feature is no > longer needed the tag is removed. And when a tag property is changed, it's > generally after a user event like pressing a button. Examples: changing the > color scheme, disabling highlighting of search matches, etc. > > When performance can be a problem is when doing an animation, like changing > the color every frame. But I think GObject is able to send one more signal > every 16ms. > > Actually I don't know why optimizing this is useful, the second patch is > probably not very useful, and the FIXME comment was not really worth fixing > it. Yes, you may be right that this does not happen very often. But doesn't that make it even more questionable to do all the overhead of connecting signal handlers, when it basically never happens ? gsignal.c stores all handlers in global datastructures, and blowing those up has a cost too...
(In reply to Sébastien Wilmet from comment #7) > Or, add a gtk_text_tag_changed() function that sends the > GtkTextTagTable::tag-changed signal. > > I prefer that solution, it's better to have smarter classes than dumb data > classes with only setters and getters. To keep related data and behavior in > one place. Yes, this is something we in a couple of other places: export an "emit-foo" function for the explicit purpose of subclassing/extending.
Created attachment 312023 [details] [review] texttag: add gtk_text_tag_changed() The function is useful for a GtkTextTag subclass that adds new properties.
The commit relies on GDK_AVAILABLE_IN_3_20 being defined.
Review of attachment 312023 [details] [review]: looks good to me and can go in once we branch
Attachment 312023 [details] pushed as 5a561a8 - texttag: add gtk_text_tag_changed()