GNOME Bugzilla – Bug 141993
Text insertion undo manager should not assume text length
Last modified: 2005-01-24 16:05:22 UTC
Undo manager for text insertion assumes the text argument to contain the same exact byte number as given in the length argument. This is quite inflexible for some code (such as GtkTextView patch in Attachment #27330 [details] of Bug #119891, in which emerging problem about undo buffer is quite unexpected). And the change is quite easy, unless there are some reasons against it.
Created attachment 27421 [details] [review] Proposed patch
Oops, in fact, the calculation of undo_action.action.insert.char has already covered the case in which length < strlen(text). So, just remove the g_return_if_fail() line is sufficient. Index: gtksourceundomanager.c =================================================================== RCS file: /cvs/gnome/gtksourceview/gtksourceview/gtksourceundomanager.c,v retrieving revision 1.4 diff -u -r1.4 gtksourceundomanager.c --- gtksourceundomanager.c 27 Sep 2003 22:54:08 -0000 1.4 +++ gtksourceundomanager.c 6 May 2004 10:03:37 -0000 @@ -614,8 +614,6 @@ if (um->priv->running_not_undoable_actions > 0) return; - g_return_if_fail (strlen (text) == (guint)length); - undo_action.action_type = GTK_SOURCE_UNDO_ACTION_INSERT; undo_action.action.insert.pos = gtk_text_iter_get_offset (pos);
Created attachment 29504 [details] [review] proposed patch version 2 Make the patch an attachment and replace the previous one.
Created attachment 32596 [details] irc log of owen and matthias discussing the bug
<mclasen> paolo: the patch in #141993 makes gedit not crash if you use preedit and undo together, but it still doesn't behave correctly <paolo> mclasen: it is not clear to me what preedit is. How can I find more info? <mclasen> paolo: preedit is what happens if you select an input method which allows you to preedit text before it is committed, e.g. to select one of multiple possible glyphs. <mclasen> paolo: if you select the "Amharic" input method and type an a, you get an underlined glyph. <mclasen> paolo: thats the preedit text. repeatedly pressing a loops through a set of possible glyphs. When the text is committed, the underline goes away <-- rodo has quit (Read error: 104 (Connection reset by peer)) <mclasen> paolo: preedit is a feature of the text view, not of the buffer, that is why the undo manager can't handle it (since it knows only the buffer) <paolo> mclasen: hmmm... ok. So undo cannot work while you are preediting text maba mace magnon mathrick|Uni mclasen meebey mgA mgatto michael mitch mjg59 mmc <mclasen> it may work to simply insert gtk_im_context_reset (GTK_TEXT_VIEW (view)->im_context) in gtk_source_view_undo/_redo <mclasen> or simply disable undo/redo, while a preedit operation is going on <paolo> what is the effect of calling gtk_im_context_reset? <paolo> or how can I know when a preedit operation is going on? --> rodo (~rodo@195.47.114.203.adsl.nextra.cz) has joined #gtk+ <mclasen> paolo: im_context_reset "will typically cause the input method to clear the preedit state" <paolo> mclasen: hmmm... ok. So if undo/redo is activated, the active preedit operation is resetted too <mclasen> paolo: yes, that would be the effect. We also reset the preedit if the cursor is moved, or a button is pressed, eg * paolo wonders whether this is the effect the user is expecting <mclasen> paolo: to find out wether there is preedit text, i guess you can call gtk_im_context_get_preedit_string() and see what it returns <mclasen> paolo: there is also a preedit-changed signal on GtkIMContext which may be useful for that <paolo> oh, ok <paolo> BTW, you said that ATM any "external" operation resets the preedit... so resetting it when undo/redo is activated will be the expected behavior too. Is this right? <mclasen> paolo: Hard to say without being a native preedit user. But I guess it wouldn't be a big surprise <paolo> mclasen: right, thanks you very much for you help <paolo> mclasen: can I put the IRC log on bugzilla? <mclasen> paolo: sure <paolo> mclasen: thanks
*** Bug 149128 has been marked as a duplicate of this bug. ***
Sorry for the late reply, but it is not completely clear to me why the assertion the patch should fail. Could you please add step by step instruction on how to reproduce this problem? I only want to be sure there are no side effects in removing the assertion. I really don't remember why I added it.
OK. Sorry for the missing information. The problem occurs when a combining character is deleted with backspace (which has just been supported in GTK+ 2.5, by the patch mentioned above). Here are steps to reproduce: - setxkbmap us,th -option grp:alt_shift_toggle - Open gedit, choose Default input method. - Type Thai text: "¡Õ" (same keys as [d][u] for QWERTY). - Press Backspace so the combining character is deleted. - Notice a warning message: (gedit:1440): GtkSourceView-CRITICAL **: gtk_source_undo_manager_insert_text_handler: assertion `strlen (text) == (guint)length' failed - Then, undo the deletion. Expected result: the combining character gets back and the text becomes "¡Õ" What actually happens: excessive character, yielding text "¡Õ¡"
> Then, undo the deletion. > Expected result: the combining character gets back and the text becomes "¡Õ" > What actually happens: excessive character, yielding text "¡Õ¡" Do you obtain the desired behavior when applying your patch? Have you tested it deeply? I'm a bit worried about side effects (there is probably a reason why the check was there, but I don't remember it and reading the code I don't see the reason).
Yes, I get the desired behavior by the patch. And the removal of assertion causes no harm so far. However, reading the code, I can see your worry, as g_strdup(), rather than g_strndup(), is used heavily in duplicating strings. Hence the assertion, maybe? Fortunately, the length and chars fields are used everywhere when length is critical, such as in merging and re-doing inserting actions. Another possible case is when strlen(text) < length, which is not tested yet. (In this case of backspace, strlen(text) >= length.) Maybe, the patch just works, but my "unless.." clause in the report also applies. Should we add some code to ensure that the length and chars fields satisfy the assertion condition? For example: undo_action.action.insert.length = length; undo_action.action.insert.chars = g_utf8_strlen (text, length); undo_action.action.insert.length = g_utf8_offset_to_pointer (text, undo_action.action.insert.chars) - text; This is for the case in which strlen(text) < length. And should g_strdup() be replaced with g_strndup()?
Created attachment 35773 [details] [review] Proposed patch v.3, with strlen(text) < length case covered, and strdup() replaced as needed For g_strdup() and g_strdup_printf() issue, only GTK_SOURCE_UNDO_ACTION_INSERT case needs the replacements. GTK_SOURCE_UNDO_ACTION_DELETE is just irrelevant here.
> - setxkbmap us,th -option grp:alt_shift_toggle > - Open gedit, choose Default input method. > - Type Thai text: "��" (same keys as [d][u] for QWERTY). Sorry, I cannot reproduce your problem. It is no clear to me what you mean with: "Type Thai text: "��" (same keys as [d][u] for QWERTY)." If I press d, u, it adds "du". > strlen(text) < length case covered Is this case possible? BTW, it is still not clear to me why in the case you reported strlen(text) > length. May you please run it in a debugger and report the values of *text and length?
> Sorry, I cannot reproduce your problem. > > It is no clear to me what you mean with: "Type Thai text: "กี" (same keys as > [d][u] for QWERTY)." > If I press d, u, it adds "du". I mean, the Thai (th) XKB layout superposes the keys on those keys of QWERTY. You need to change keyboard group with Alt-Shift first to get Thai keys. > > strlen(text) < length case covered > > Is this case possible? I don't know if there would be such case. It's to maintain the precondition implied by the assertion, and to make sure the program won't see string end before its expected length when working with undo actions, just in case. > BTW, it is still not clear to me why in the case you reported > strlen(text) > length. Please see Attachment #27330 [details] (patch to GTK+ that enabled the combining character deletion by backspace, Bug #119891). The removal is done by deleting the whole cluster, before inserting back a normalized string of the same cluster, with the last character excluded. The relevant line in the patch is: gtk_text_buffer_insert_interactive_at_cursor (get_buffer (text_view), normalized_text, g_utf8_offset_to_pointer (normalized_text, len - 1) - normalized_text, text_view->editable); > May you please run it in a debugger and report the values of *text and > length? Sure. I'll do it tonight (Thailand local time). I'm just away from my machine now.
> May you please run it in a debugger and report the values of *text and > length? Here're the values: - text="ก",length=3 // when inserting [ก] - text="ี", length=3 // when inserting [ ี] - text="กี",length=3 // when pressing backspace When pressing backspace, the cluster "กี" was deleted, before being inserted back with the last character excluded, thus the length 3 instead of 6.
Sprry for the late reply and thank for the patch. I have just reviewed it. If you agree I'd prefer to not cover the case "strlen(text) < length" for two reasons: - minimizimg the code changes - it does not seem this case can happen For this reasons, I think we should simply changes the assertion to + g_return_if_fail (strlen (text) >= (guint)length); Do you agree?
Yes, I agree with the minimal change, and with skipping the rare case. Anyway, what do you think about the g_strdup() and g_strdup_printf() replacements? Wouldn't it be defensive not to assume the text buffer to be null-terminated when the length argument is also passed?
I have committed the + g_return_if_fail (strlen (text) >= (guint)length); I'm going to close this bug report but, please, lemme know if you do not agree on the committed patch. I have opened a bug against gtk+ (bug #165101) for the problem discussed in comment #4 and comment #5.
oh, we committed at the same time our comments. > Anyway, what do you think about the g_strdup() and g_strdup_printf() > replacements? Wouldn't it be defensive not to assume the text buffer to be > null-terminated when the length argument is also passed? AFAIK, the text buffer should always be null terminated. Thanks again for you help with this bug.