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 624791 - GtkTextBuffer doesn't emit signal "notify" on change of property "text"
GtkTextBuffer doesn't emit signal "notify" on change of property "text"
Status: RESOLVED OBSOLETE
Product: gtk+
Classification: Platform
Component: Widget: GtkTextView
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2010-07-20 04:08 UTC by mchtly
Modified: 2018-04-15 00:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
textbuffer: emit notify signal for the "text" property (1.36 KB, patch)
2014-07-20 14:00 UTC, Sébastien Wilmet
needs-work Details | Review
Test performances (587 bytes, text/plain)
2014-07-31 09:04 UTC, Sébastien Wilmet
  Details

Description mchtly 2010-07-20 04:08:34 UTC
When the text is changed in a TextBuffer widget, it will not emit a "notify" signal on the "text" property. The property is different before and after, so the signal should be emitted. I'm not sure if the signal should be emitted every time the character position changes, but at least on unfocus would be nice.
Comment 1 Matthias Clasen 2010-08-05 03:30:51 UTC
The text buffer has a dedicated "changed" signal for this purpose.
Comment 2 Sébastien Wilmet 2014-07-20 14:00:33 UTC
Created attachment 281237 [details] [review]
textbuffer: emit notify signal for the "text" property

Although there is the "changed" signal, it is more correct to notify the
"text" property too. It can be useful for a small text view, where the
text is saved e.g. to gsettings with a binding to the text property.

The "text" property includes only the text, not child widgets or images,
so the notify signal is sent too many times (also for child widgets and
images), but it's not a big problem.
Comment 3 Paolo Borelli 2014-07-26 12:52:30 UTC
Review of attachment 281237 [details] [review]:

The patch looks correct to me and makes sense imho from a functional point of view.

The only issue is whether this has major performance implications: I do not recall the current state of gobject internals... does building that paramspec involve accessing the whole buffer text?

I seem to recall that at some point desrt was working on an optimization to not actually build the paramspec and emit the notification if there are no handlers connected, but I do not recall the state of that...
Comment 4 Sébastien Wilmet 2014-07-29 13:23:26 UTC
The ParamSpec contains only the metadata, not the data, so by default the whole buffer is not fetched when emitting the notify::text signal. I've tested, and the get_property is not called, even if a signal handler is connected.

The commit is pushed to the master branch.
Comment 5 Emmanuele Bassi (:ebassi) 2014-07-30 10:04:13 UTC
Sébastien, the patch was reviewed but not marked as "accepted-commit_now". could you please revert your commit?
Comment 6 Emmanuele Bassi (:ebassi) 2014-07-30 10:07:41 UTC
Review of attachment 281237 [details] [review]:

to me, the words "the notify signal is sent too many times" are already a warning bell. not because the signal builds data or not, but because anything can be connected to `notify::text` — GSetting mappings and GObject binding being the two most egregious and they are very much getting widely used.

notification for property changes should *only* be emitted if the property content *actually* changed. we have various short-circuits in place inside GObject and GTK+ to avoid over-notification, and excessive signal emissions.

this patch should be reworked so that it emits a notification when the content of the :text property changes.
Comment 7 Sébastien Wilmet 2014-07-30 10:22:47 UTC
Yes, sorry, Paolo has reviewed the patch at guadec and said that I can push the patch, I should have said that here.

The GObject::notify signal documentation reads:

"Note that getting this signal doesn't guarantee that the value of the property has actually changed"

Notifying the text property only when the text is modified is more difficult. Does it really worth the effort? Are the performances really important here? Does it decrease significantly the performances of GSettings mappings and GObejct bindings? I think keeping the code simple is better, if nobody complains.

If needed, I can anyway write another patch to improve the previous one, no need to revert in my opinion (unless I forget to work on this for a while).
Comment 8 Matthias Clasen 2014-07-30 22:24:00 UTC
I would suggest that the first step here should be a testcase that demonstrates missing notify emissions.
Comment 9 Sébastien Wilmet 2014-07-31 09:04:15 UTC
Created attachment 282127 [details]
Test performances

Here is a simple test for performances. It adds 100k characters to a text buffer one by one.

Before the patch (notify::text is not emitted):
time: 0.413092

After the patch:
time: 0.553343

But the test is an extreme case. When a file is loaded in gedit, the text is inserted by chunks, not character by character.

Images and child widgets are quite rare in a text buffer. It's mostly text. So it's not the end of the world if notify::text is sent uselessly for, say, 1% of the time.
Comment 10 Emmanuele Bassi (:ebassi) 2014-07-31 09:10:11 UTC
(In reply to comment #9)
> Created an attachment (id=282127) [details]
> Test performances
> 
> Here is a simple test for performances. It adds 100k characters to a text
> buffer one by one.

thanks for the test case, but that was not the point I was trying to make in comment 6.

people can add arbitrary GObject property or GSettings key bindings to property notification, and we have escape hatches in place to avoid excessive (or redundant) work in that case. emitting notifications if nothing has changed will cause things that should not happen more than once to happen multiple times.
Comment 11 Sébastien Wilmet 2014-07-31 13:58:59 UTC
> emitting notifications if nothing has changed
> will cause things that should not happen more than once to happen multiple
> times.

The documentation says that a notify signal can be emitted even if the value didn't change. It's likely the case at *lots* of places in GObject-based code, in GNOME and outside GNOME.

If it's no longer the case, it is an undocumented API break.

> things that should not happen more than once

Do you have an example? It seems that such code have undesirable side effects. If the notify signal is sent a second time, exactly the same code is executed in the callbacks, how can it be a problem?
Comment 12 Matthias Clasen 2018-02-10 04:58:00 UTC
We're moving to gitlab! As part of this move, we are moving bugs to NEEDINFO if they haven't seen activity in more than a year. If this issue is still important to you and still relevant with GTK+ 3.22 or master, please reopen it and we will migrate it to gitlab.
Comment 13 Matthias Clasen 2018-04-15 00:05:59 UTC
As announced a while ago, we are migrating to gitlab, and bugs that haven't seen activity in the last year or so will be not be migrated, but closed out in bugzilla.

If this bug is still relevant to you, you can open a new issue describing the symptoms and how to reproduce it with gtk 3.22.x or master in gitlab:

https://gitlab.gnome.org/GNOME/gtk/issues/new