GNOME Bugzilla – Bug 652653
ClutterText should have a buffer similar to GtkEntry
Last modified: 2012-01-24 12:32:44 UTC
The GtkEntryBuffer allows us to: * Implement stuff like redo/undo without modifying GtkEntry code. * Have two entries use the same buffer. * Use non-pageable memory (or other storage) for a GtkEntry. I'm integrating keyring prompts into the shell, and we need to be able to have passwords in entries stored in non-pageable memory. So I've ported GtkEntryBuffer to ClutterText. But it has several other benefits.
Created attachment 189986 [details] [review] text: Implement ClutterTextBuffer * Abstracts the buffer for text in ClutterText * Allows implementation of undo/redo. * Allows use of non-pageable memory for text in the case of sensitive passwords. * Implement a test with two ClutterText using the same buffer.
Review of attachment 189986 [details] [review]: you should really set the name and email in your git configuration options ::: clutter/clutter-text-buffer.c @@ +345,3 @@ + * Since: 1.8 + */ + g_object_class_install_property (gobject_class, you should use g_object_class_install_properties() and an array of GParamSpec like the rest of Clutter. @@ +445,3 @@ +ClutterTextBuffer* +clutter_text_buffer_new (const gchar *initial_chars, + gint n_initial_chars) n_initial_chars should be a gssize, but this makes sense only if initial_chars is a binary blob (const guchar*). I'd just remove the n_initial_chars. @@ +449,3 @@ + ClutterTextBuffer *buffer = g_object_new (CLUTTER_TYPE_TEXT_BUFFER, NULL); + if (initial_chars) + clutter_text_buffer_set_text (buffer, initial_chars, n_initial_chars); you should strndup() and pass the string to the text property. @@ +474,3 @@ + g_return_val_if_fail (klass->get_length != NULL, 0); + + return (*klass->get_length) (buffer); no need to do (* function_pointer).
Created attachment 189992 [details] [review] Updated after review Whoops, forgot to config my name/email. First commit on this module. Thanks for the review. Made those changes. Avoided doing a strndup() since that's exactly what we don't want. One of the use cases of this is not duping passwords into non-buffer memory. That way we can implement a buffer with non-pageable memory and be reasonably sure that the only place the (full) password is sitting around is in the buffer non-pageable memory. Branch available at: http://cgit.collabora.com/git/user/stefw/clutter.git/log/?h=clutter-text-buffer
(In reply to comment #3) > Created an attachment (id=189992) [details] [review] > Updated after review > > Whoops, forgot to config my name/email. First commit on this module. > > Thanks for the review. Made those changes. thanks, looks really good (I forgot to say that in the first comment :-)). > Avoided doing a strndup() since that's exactly what we don't want. I understand that, but then you should remove writable bit from the :text property because you only want to control it through the set_text() method and never through the property. Bindings will always create a TextBuffer instance through g_object_new() so having the default constructor taking a text+text_len won't help them. I'd rejig it as: ClutterTextBuffer * clutter_text_buffer_new (void); ClutterTextBuffer * clutter_text_buffer_new_with_text (const gchar *text, gssize text_len); and with_text() will call set_text() internally. > Branch available at: > http://cgit.collabora.com/git/user/stefw/clutter.git/log/?h=clutter-text-buffer
Created attachment 190166 [details] [review] Reworked constructors Good points. Reworked constructors as suggested, also set proper copyrights in new files.
so, I tried a rebased clutter-text-buffer branch, and running the conformance test suite fails in a couple of places, namely: /conform/text/text_cache: ** ERROR:test-text-cache.c:157:do_tests: assertion failed: (check_result (data, "Change the font", TRUE) == FALSE) /conform/text/text_delete_chars: ** ERROR:test-clutter-text.c:211:text_delete_chars: assertion failed (clutter_text_get_cursor_position (text) == 1): (2 == 1) which indicates at least two regressions - one in the layout invalidation, and the other in the cursor invalidation.
Thanks for catching that. (In reply to comment #6) > so, I tried a rebased clutter-text-buffer branch, and running the conformance > test suite fails in a couple of places, namely: > > /conform/text/text_cache: ** > ERROR:test-text-cache.c:157:do_tests: assertion failed: (check_result (data, > "Change the font", TRUE) == FALSE) Fixed this one. > /conform/text/text_delete_chars: ** > ERROR:test-clutter-text.c:211:text_delete_chars: assertion failed > (clutter_text_get_cursor_position (text) == 1): (2 == 1) The behavior of clutter_text_delete_chars() seems broken. It deletes characters in front of the cursor, but then moves the cursor backwards the same number of spaces (not taking into account that it could be near the beginning). But I've implemented the old behavior to keep compatibility. Let me know if I should open another bug to actually fix this, or if somehow this is the expected behavior. Rebased, and available in the branch at the same location.
Created attachment 194296 [details] [review] text: Fix regression when implementing ClutterTextBuffer
Created attachment 194297 [details] [review] text: Revert strange behavior of clutter_text_delete_chars() * This was fixed in passing by the ClutterTextBuffer changes. * Added comment documenting abnormal behavior.
Any update on landing this ?
Created attachment 203483 [details] [review] Rebased patch on master again, and made sure that all tests pass. text: Implement ClutterTextBuffer * Abstracts the buffer for text in ClutterText * Allows implementation of undo/redo. * Allows use of non-pageable memory for text in the case of sensitive passwords. * Implement a test with two ClutterText using the same buffer.
Review of attachment 203483 [details] [review]: looks good. I'm sure we can improve things once this lands, so let's get that out of the way for 1.9.4.
patch pushed, and released.