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 755416 - Be able to subclass GtkTextTag cleanly
Be able to subclass GtkTextTag cleanly
Status: RESOLVED FIXED
Product: gtk+
Classification: Platform
Component: Widget: GtkTextView
3.18.x
Other Linux
: Normal enhancement
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on: 755413
Blocks: 744179
 
 
Reported: 2015-09-22 13:30 UTC by Sébastien Wilmet
Modified: 2015-10-04 02:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
texttag: add a ::changed signal (5.11 KB, patch)
2015-09-22 13:34 UTC, Sébastien Wilmet
reviewed Details | Review
texttag: reduce the number of ::changed signals emitted (10.54 KB, patch)
2015-09-22 13:34 UTC, Sébastien Wilmet
none Details | Review
texttag: add gtk_text_tag_changed() (4.42 KB, patch)
2015-09-24 09:52 UTC, Sébastien Wilmet
committed Details | Review

Description Sébastien Wilmet 2015-09-22 13:30:34 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.
Comment 1 Sébastien Wilmet 2015-09-22 13:34:02 UTC
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.
Comment 2 Sébastien Wilmet 2015-09-22 13:34:07 UTC
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).
Comment 3 Sébastien Wilmet 2015-09-22 13:36:53 UTC
Note: with the second patch, bug #755413 is more often triggered.
Comment 4 Matthias Clasen 2015-09-22 17:14:10 UTC
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...
Comment 5 Sébastien Wilmet 2015-09-23 10:05:21 UTC
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.
Comment 6 Sébastien Wilmet 2015-09-23 11:11:56 UTC
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.
Comment 7 Sébastien Wilmet 2015-09-23 11:31:26 UTC
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.
Comment 8 Matthias Clasen 2015-09-23 17:56:44 UTC
(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...
Comment 9 Matthias Clasen 2015-09-23 22:38:49 UTC
(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.
Comment 10 Sébastien Wilmet 2015-09-24 09:52:07 UTC
Created attachment 312023 [details] [review]
texttag: add gtk_text_tag_changed()

The function is useful for a GtkTextTag subclass that adds new
properties.
Comment 11 Sébastien Wilmet 2015-09-24 09:57:29 UTC
The commit relies on GDK_AVAILABLE_IN_3_20 being defined.
Comment 12 Matthias Clasen 2015-09-26 02:36:42 UTC
Review of attachment 312023 [details] [review]:

looks good to me and can go in once we branch
Comment 13 Matthias Clasen 2015-10-04 02:24:56 UTC
Attachment 312023 [details] pushed as 5a561a8 - texttag: add gtk_text_tag_changed()