GNOME Bugzilla – Bug 624791
GtkTextBuffer doesn't emit signal "notify" on change of property "text"
Last modified: 2018-04-15 00:05:59 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.
The text buffer has a dedicated "changed" signal for this purpose.
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.
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...
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.
Sébastien, the patch was reviewed but not marked as "accepted-commit_now". could you please revert your commit?
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.
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).
I would suggest that the first step here should be a testcase that demonstrates missing notify emissions.
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.
(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.
> 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?
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.
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