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 652653 - ClutterText should have a buffer similar to GtkEntry
ClutterText should have a buffer similar to GtkEntry
Status: RESOLVED FIXED
Product: clutter
Classification: Platform
Component: ClutterText
unspecified
Other Linux
: Normal enhancement
: ---
Assigned To: clutter-maint
clutter-maint
Depends on:
Blocks: 652460
 
 
Reported: 2011-06-15 14:35 UTC by Stef Walter
Modified: 2012-01-24 12:32 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
text: Implement ClutterTextBuffer (71.18 KB, patch)
2011-06-15 14:35 UTC, Stef Walter
needs-work Details | Review
Updated after review (69.41 KB, patch)
2011-06-15 17:15 UTC, Stef Walter
none Details | Review
Reworked constructors (69.90 KB, patch)
2011-06-18 08:23 UTC, Stef Walter
none Details | Review
text: Fix regression when implementing ClutterTextBuffer (924 bytes, patch)
2011-08-20 16:20 UTC, Stef Walter
none Details | Review
text: Revert strange behavior of clutter_text_delete_chars() (1.21 KB, patch)
2011-08-20 16:20 UTC, Stef Walter
none Details | Review
Rebased patch on master again, and made sure that all tests pass. (72.96 KB, patch)
2011-12-14 15:20 UTC, Stef Walter
committed Details | Review

Description Stef Walter 2011-06-15 14:35:31 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.
Comment 1 Stef Walter 2011-06-15 14:35:58 UTC
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.
Comment 2 Emmanuele Bassi (:ebassi) 2011-06-15 14:51:17 UTC
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).
Comment 3 Stef Walter 2011-06-15 17:15:18 UTC
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
Comment 4 Emmanuele Bassi (:ebassi) 2011-06-17 16:57:27 UTC
(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
Comment 5 Stef Walter 2011-06-18 08:23:23 UTC
Created attachment 190166 [details] [review]
Reworked constructors

Good points. Reworked constructors as suggested, also set proper copyrights in new files.
Comment 6 Emmanuele Bassi (:ebassi) 2011-08-02 14:11:31 UTC
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.
Comment 7 Stef Walter 2011-08-20 16:18:23 UTC
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.
Comment 8 Stef Walter 2011-08-20 16:20:34 UTC
Created attachment 194296 [details] [review]
text: Fix regression when implementing ClutterTextBuffer
Comment 9 Stef Walter 2011-08-20 16:20:38 UTC
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.
Comment 10 Matthias Clasen 2011-12-13 18:34:25 UTC
Any update on landing this ?
Comment 11 Stef Walter 2011-12-14 15:20:30 UTC
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.
Comment 12 Emmanuele Bassi (:ebassi) 2012-01-17 11:51:24 UTC
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.
Comment 13 Emmanuele Bassi (:ebassi) 2012-01-24 12:32:44 UTC
patch pushed, and released.