GNOME Bugzilla – Bug 576801
A model/buffer for GtkEntry
Last modified: 2009-07-09 01:51:03 UTC
A while back I tackled the possibility of having GtkEntry store it's text in a application configurable memory buffer. This makes GtkEntry very useful for use with gnome-keyring and seahorse passwords/secrets. I've given it another shot, and come up with what I think is a much better approach. I've implemented a GtkEntryBuffer class (or GtkEntryModel) which segregates all the text storage and conversion to displayable text. Will attach a rough initial patch.
Created attachment 131404 [details] [review] Initial rough patch All in one initial rough patch.
Created attachment 131405 [details] [review] Initial rough patch Whoops the previous patch was backwards :( Here's what it looks like: gtk/Makefile.am | 2 + gtk/gtk.h | 1 + gtk/gtkentry.c | 925 +++++++++++++++----------------------- gtk/gtkentry.h | 18 +- gtk/gtkentrybuffer.c | 960 ++++++++++++++++++++++++++++++++++++++++ gtk/gtkentrybuffer.h | 153 +++++++ gtk/gtkfilechooserentry.c | 12 +- gtk/gtkmarshalers.list | 2 + gtk/gtkspinbutton.c | 10 +- modules/other/gail/gailentry.c | 27 +- 10 files changed, 1517 insertions(+), 593 deletions(-)
The filechooser entry, spinbutton and gail entry parts are just cleanups that can be committed right away.
The approach of separating out a subclassable buffer object makes a lot of sense to me. (It might also help with the undo support that I've studied in another bug yesterday) Too late already to do a full review tonight, but here are some things I noticed: We are storing the password hint in object data to keep regular entries as lightweight as possible, since there can be tons of entries in an application. Might be worth to keep it that way. The widget hook thing looks a bit hackish to me, and not quite correct. At the very least, we would have to recompute the invisible char on style-set as the current code does, and reget the hint timeout at least for every screen change (the current code gets it from the settings object every time). I would propose to keep the find-the-best-invisible-char functionality in the entry itself, and just have the entry set the invisible char on the buffer. With that approach, you don't need the unset_invisible_char and have_invisible_char functions on the buffer, either. For the hint timeout, we might just make that a regular property on the buffer and leave it to the entry to sync that with the setting at suitable times. + P_("Text buffer object which actually stores entry text"), + GTK_TYPE_ENTRY_BUFFER, + G_PARAM_READWRITE | G_PARAM_CONSTRUCT)); Should use GTK_PARAM_READWRITE here + g_object_notify (G_OBJECT (entry), "invilible-char"); + g_object_notify (G_OBJECT (entry), "invilible-char-set"); Typos
(In reply to comment #3) > The filechooser entry, spinbutton and gail entry parts are just cleanups that > can be committed right away. Committed these parts. I hope make to soon make the other changes you noted.
Created attachment 132526 [details] [review] Patch for review Reimplemented, keeping the 'invisibility' logic in GtkEntry and have GtkEntryBuffer be a simple storage buffer with insertion/removal/max-length logic. gtk/Makefile.am | 2 + gtk/gtk.h | 1 + gtk/gtkentry.c | 929 +++++++++++++++++++++++++----------------------- gtk/gtkentry.h | 14 +- gtk/gtkentrybuffer.c | 638 +++++++++++++++++++++++++++++++++ gtk/gtkentrybuffer.h | 124 +++++++ gtk/gtkmarshalers.list | 2 + Open questions: * What do we do with the following (sealed, public) fields of GtkEntry: text_length, text_max_length * Where should GtkEntryBuffer the class be documented? * Should any of the following become deprecated (I don't think so)? gtk_entry_get_text gtk_entry_set_text gtk_entry_get_text_length gtk_entry_set_max_length gtk_entry_get_max_length
Created attachment 137878 [details] [review] Latest patch I've reworked things a bit more and added gtk_entry_buffer_emit_inserted_text() and gtk_entry_buffer_emit_deleted_text() functions for use when subclassing. Makefile.am | 2 gtk.h | 1 gtkentry.c | 929 +++++++++++++++++++++++++++-------------------------- gtkentry.h | 14 gtkentrybuffer.c | 676 ++++++++++++++++++++++++++++++++++++++ gtkentrybuffer.h | 133 +++++++ gtkmarshalers.list | 2 7 files changed, 1311 insertions(+), 446 deletions(-)
Looks good on first reading. Some minor comments: + { + gtk_entry_draw_cursor (GTK_ENTRY (widget), CURSOR_STANDARD); + } We don't use {} around single statements in GTK+ coding style. +static void +gtk_entry_real_delete_text (GtkEditable *editable, + gint start_pos, + gint end_pos) +{ + /* + * The actual insertion into the buffer. This will end up firing the Copy-paste error ? The comment should probably talk about deletion, not insertion... +static void +buffer_inserted_text (GtkEntryBuffer *buffer, guint position, + const gchar *chars, guint n_chars, GtkEntry *entry) GTK+ coding style calls for arguments to be lined up like this: static void buffer_inserted_text (GtkEntryBuffer *buffer, guint position, const gchar *chars, guint n_chars, GtkEntry *entry) + g_object_class_install_property (gobject_class, PROP_MAX_LENGTH, + g_param_spec_uint ("max-length", P_("Maximum length"), + P_("Maximum number of characters for this entry. Zero if no maximum"), + 0, GTK_ENTRY_BUFFER_MAX_SIZE, 0, G_PARAM_READWRITE)); Should use GTK_PARAM_READWRITE here. + signals[INSERTED_TEXT] = g_signal_new ("inserted-text", GTK_TYPE_ENTRY_BUFFER, + G_SIGNAL_RUN_FIRST, G_STRUCT_OFFSET (GtkEntryBufferClass, inserted_text), + NULL, NULL, _gtk_marshal_VOID__UINT_STRING_UINT, + G_TYPE_NONE, 3, G_TYPE_UINT, G_TYPE_STRING, G_TYPE_UINT); We generally align these signal_new calls with one argument per line. - gchar *GSEAL (text); + GtkEntryBuffer* GSEAL (buffer); Sadly, I am not sure that this is compatible enough. I wonder if we can do something semi-tricky, like move the buffer to GtkEntryPrivate, and keep text pointing at the actual content, as long as gtk_entry_set_buffer has not been explicitly called (or maybe even as long as the buffer is of type GtkEntryBuffer, as opposed to a subclass). Should be doable by doing text = gtk_entry_buffer_get_text () whenever the buffer is modified, right ?
Created attachment 137951 [details] [review] Updated patch An updated patch with these changes: * Document validity of gtk_entry_buffer_get_text() returned value. * Keep old fields in place for compatibility. * Use gint instead of guint in gtk_entry_buffer_new. * Use GTK_PARAM_READWRITE instead of G_PARAM_READWRITE * Whitespace and comment fixes. As far as the old text pointer (and text_length, text_max_length). I added code to update them whenever we receive a notify event on those properties from the buffer. We were already tracking those events... The following should be ABI compatible, no? - gchar *GSEAL (text); + const gchar *GSEAL (text);
Looks pretty good to me now. Some more small things: + * @position: the position the text was deletedd at. You got an extra d there + * @initial_chars: initial buffer text, or NULL Should be marked up as %NULL +/** + * gtk_entry_buffer_get_text: + * @buffer: a #GtkEntryBuffer + * + * Retrieves the length in bytes of the buffer. + * See gtk_entry_buffer_get_length(). + * + * Return value: The byte length of the buffer. + **/ +gsize +gtk_entry_buffer_get_bytes (GtkEntryBuffer *buffer) Copy paste error here: get_text vs get_bytes + * The memory pointer returned by this call will not change + * unless this object emits a signal. Which is not totally true, since it will also get freed when the object is finalized. Should add "Since: 2.18" to all GtkEntryBuffer functions (some are currently missing this). Might be cool to add an example of two entries sharing a buffer to gtk-demo, just to see that that works correctly. Also, we'll need to add a section about GtkEntryBuffer to docs/reference/gtk/gtk-sections.txt and the corresponding include to gtk-docs.sgml and list gtk_entry_buffer_get_type in gtk.types. Finally, it would be good to mention the purpose of this whole excercise in the long description of GtkEntryBuffer: to support secure memory handling for applications handling important secrets. The following should be ABI compatible, no? - gchar *GSEAL (text); + const gchar *GSEAL (text); It might cause problems in C++, I think. Probably best avoided.
Created attachment 137984 [details] [review] Updated patch A new patch: * GtkEntryBuffer length property is not writable. * Add demo for shared GtkEntryBuffer. * Add gtk_entry_new_with_buffer() method. * Use GTK_DISABLE_DEPRECATED instead of COMPAT comments. * Complete gtk docs for GtkEntryBuffer. demos/gtk-demo/Makefile.am | 1 demos/gtk-demo/entry_buffer.c | 65 + docs/reference/gtk/gtk-docs.sgml | 1 docs/reference/gtk/gtk-sections.txt | 31 docs/reference/gtk/gtk.types | 1 docs/reference/gtk/tmpl/gtk-unused.sgml | 19 docs/reference/gtk/tmpl/gtkentry.sgml | 32 docs/reference/gtk/tmpl/gtkentrybuffer.sgml | 168 ++++ gtk/Makefile.am | 2 gtk/gtk.h | 1 gtk/gtkentry.c | 967 +++++++++++++++------------- gtk/gtkentry.h | 20 gtk/gtkentrybuffer.c | 741 +++++++++++++++++++++ gtk/gtkentrybuffer.h | 133 +++ gtk/gtkmarshalers.list | 2 15 files changed, 1749 insertions(+), 435 deletions(-)
Pretty close. The one thing I don't think is right is the use of GTK_DISABLE_DEPRECATED here. It is meant as a flag that you can set when building something against GTK+ to remove stuff from the headers. It is not meant as something to use at compile time inside GTK+. So these things: +#ifndef GTK_DISABLE_DEPRECATED + entry->text = (gchar*)gtk_entry_buffer_get_text (buffer); +#endif should not be ifdefed like that, but rather should be there unconditionally, until we break compat, ie 3.0. There should probably a marker comment at these places that instructs us to remove this come 3.0. If those ifdefs go away, the ones in the header: +#ifdef GTK_DISABLE_DEPRECATED + void *padding_0; +#else gchar *GSEAL (text); +#endif also need to go away. Other than that, looks ready to go in.
Fixed the GTK_DISABLE_DEPRECATED problem, squashed and pushed to git.gnome.org. Thanks for your reviews and help getting all the details figured out.