gtk-hp-patches
- From: Owen Taylor <otaylor redhat com>
- To: hp redhat com
- Cc: gtk-devel-list gnome org
- Subject: gtk-hp-patches
- Date: 08 Aug 2000 15:23:48 -0400
Here, after much delay, is my set of comments on the
big gtk-hp-patches diff from a few weeks ago. Almost
everything in here are;
- stylistic issues
- suggestions for better ways of doing things
- bugs I noticed
There might be one or two naming issues as well.
[ I'm not sure that this will make much sense to anybody but
Havoc, and to Havoc only with a copy of the patch to
grep through for comparison, but I'm mailing it to gtk-devel-list
for completeness. ]
Regards,
Owen
======================================================================
One general comment - we need docs for all new entry points. I've been
doing them inline.
=======
+make_inline_pixbuf_LDADD = $(LDADDS) -lgmodule
=======
Get rid of -lgmodule
=====
+static void
+free_buffer (guchar *pixels, gpointer data)
+{
+ free (pixels);
+}
=====
We use a simple cast of free() instead of a wrapper function
in most such places.
=====
+static GdkPixbuf*
+read_raw_inline (const guchar *data, gboolean copy_pixels)
+{
+ GdkPixbuf *pixbuf;
+ const guchar *p = data;
+
+ pixbuf = GDK_PIXBUF (g_type_create_instance (GDK_TYPE_PIXBUF));
+
=====
g_object_new() according to Tim
=========
+/**
+ * gdk_pixbuf_new_from_inline:
+ * @data: An inlined GdkPixbuf
+ * @copy_pixels: whether to copy the pixels out of the inline data, or to use them in-place
+ *
+ * Create a #GdkPixbuf from a custom format invented to store pixbuf
+ * data in C program code. This library comes with a program called "image-to-inline"
=========
Despite comment here and ChangeLog, it's not called image-to-inline.
========
+ * The inline data format contains the pixels in #GdkPixbuf's native format.
+ * Since the inline pixbuf is static data, you don't really need to copy it.
+ * However it's typically in read-only memory, so if you plan to modify
+ * it you must copy it.
=========
Editorial: "don't need to copy it if you plan to use the image only in its
original form"
=========
+ if (read_int (&p) != GDK_PIXBUF_INLINE_MAGIC_NUMBER) {
+ g_warning ("Bad inline data; wrong magic number");
+ return NULL;
+ }
==...====
+ switch (format)
+ {
+ case GDK_PIXBUF_INLINE_RAW:
+ pixbuf = read_raw_inline (p, copy_pixels);
+ break;
+
+ default:
+ return NULL;
========
Why do you warn in one case and not the other?
==========
+void
+output_int (FILE *outfile, int number, const char *comment)
+{
+ guchar bytes[4];
+ guint32 num;
+
+ g_assert (sizeof (bytes) == sizeof (num));
===========
If you depend on the size of number, just use gint32.
=====
+ num = number;
+ *((guint32*)bytes) = g_htonl (num);
=====
I don't know if you fixed this with the other one that Darin pointed out,
but you want:
guchar *bytes;
num = g_htonl (num);
bytes = (guchar *)#
To avoid alignment problems.
+
+ fprintf(outfile, " /* %s (decimal: %u) */\n 0x%.2x, 0x%.2x, 0x%.2x, 0x%.2x,\n",
+ comment, num,
+ bytes[0], bytes[1], bytes[2], bytes[3]);
How about just /* %s (%u) */.
=====
+ fprintf (outfile, "%sconst guchar ", modifier);
+ fputs (varname, outfile);
+ fputs (" [] =\n", outfile);
+ fputs ("{\n", outfile);
=====
As a quibble - GTK+ style is not "int numbers []", it is int number[]
==
+ output_int (outfile, gdk_pixbuf_get_colorspace (pixbuf), "Colorspace (0 == RGB, no other options implemented)");
+
====
You probably should put a note in gdk-pixbuf.h to the effect that the values
of this enum have to stay constant for source compatibility now.
==========
+ while (i < argc)
+ {
+ GdkPixbuf *pixbuf;
+
+ if ((i + 1) == argc)
+ usage ();
===========
I think it is a bit odd to spit out a usage statement after writing something
to the output file. You probably should check argc % 2 initially.
===========
+ label = gtk_label_new ("");
===========
To mean "don't care", I think it is clearer to do gtk_label_new (NULL);
(Several of these)
===========
+ if (keyval && accel_group)
+ {
+ gtk_widget_add_accelerator (button,
+ "clicked",
+ accel_group,
+ keyval,
+ GDK_MOD1_MASK,
+ GTK_ACCEL_VISIBLE);
===========
While neither really has an effect here I think I would
not include GTK_ACCEL_VISIBLE and would include GTK_ACCEL_LOCKED,
since the accelerator is not visible, and cannot be changed.
====
+static void gtk_dialog_add_buttons_valist(GtkDialog *dialog,
+ const gchar *first_button_text,
+ gint first_response_id,
+ va_list args);
====
s/valist(/valist (/;
========
+static gint
+gtk_dialog_key_press (GtkWidget *widget,
+ GdkEventKey *key)
+{
+ GdkEventAny event = { GDK_DELETE, widget->window, TRUE };
========
Not valid C89. (I forget if it is valid in C99 or not. But you
can't do it anyways in GTK+.)
===========
+GtkWidget*
+gtk_dialog_new_with_buttons (const gchar *title,
+ GtkWindow *parent,
+ GtkDialogFlags flags,
+ const gchar *first_button_text,
+ gint first_response_id,
+ ...)
===========
You should probably drop first_response_id, so you can do:
gtk_dialog_new_with_buttons (title, parent, flags, NULL);
Yes, there is new_empty(), but I think being able to have 0 buttons
makes things more consistent.
=======
+ ResponseData *ad = gtk_object_get_data (GTK_OBJECT (widget),
+ "___gtk_dialog_response_data");
=======
Hmm, this reminds me of __FOCUS_STYLE 1.
GTK+ gets the "gtk-" namespace for object data as well - just do
"gtk-dialog-response-data".
=====
+static ResponseData*
+get_response_data (GtkWidget *widget)
+{
+ ResponseData *ad = gtk_object_get_data (GTK_OBJECT (widget),
+ "___gtk_dialog_response_data");
+
+ if (ad == NULL)
+ {
+ ad = g_new (ResponseData, 1);
====
====
+ ad = get_response_data (widget);
+
+ if (ad != NULL)
+ response_id = ad->response_id;
====
This code doesn't make sense - get_response_data() can't return NULL.
(Note that if there is any possiblity that the lookup would fail, you
need to fix multiple things here, since ad->response_id is junk if the
response data is created here.)
====
+static void
+connect_action_widget (GtkDialog *dialog, GtkWidget *child)
+{
====
===
+void
+gtk_dialog_add_action_widget (GtkDialog *dialog,
+ GtkWidget *widget,
+ gint response_id)
===
I think it would be a bit clearer if these functions were combined -
there is no danger of the combination taking more than one
screenful.
====
+static void
+gtk_dialog_add_buttons_valist(GtkDialog *dialog,
+ const gchar *first_button_text,
+ gint first_response_id,
+ va_list args)
=====
As above for 'valist (' and also first_response_id.
====
+typedef struct {
+ GtkDialog *dialog;
====
'{' on next line.
=====
+static void
+run_destroy_handler (GtkDialog *dialog, gpointer data)
+{
+ RunInfo *ri = data;
+
+ shutdown_loop (ri);
+}
+
+static void
+run_response_handler (GtkDialog *dialog,
+ gint response_id,
+ gpointer data)
+{
+ RunInfo *ri;
+
+ ri = data;
=====
You might as well be consistent for functions next to each
other :-) - I prefer 'RunInfo *ri = data'.
===
+ g_return_val_if_fail (dialog != NULL, -1);
+ g_return_val_if_fail (GTK_IS_DIALOG (dialog), -1);
===
The final agreement between Tim and me was to omit the first check
since the second check does it as well.
===
+ ri.response_handler =
+ gtk_signal_connect (GTK_OBJECT (dialog),
+ "response",
+ GTK_SIGNAL_FUNC (run_response_handler),
+ &ri);
===
I think putting the handler ID's in the ResponseInfo structure
obscures the code flow since they aren't used outside of gtk_dialog_run()
====
+/* Convenience enum to use for action_id's. Positive values are
+ totally user-interpreted. GTK will sometimes return GTK_ACTION_NONE
+ if no action_id is available.
+
+ Typical usage is:
+ if (gtk_dialog_run(dialog) == GTK_ACTION_ACCEPT)
+ blah();
+
+*/
====
Tim comment-style comment. :-)
====
+GtkIconFactory*
+gtk_icon_factory_new (void)
+{
+ return GTK_ICON_FACTORY (g_type_create_instance (GTK_TYPE_ICON_FACTORY));
+}
===
Again, we are being told to use g_object_new().
===
+ GtkIconSource source = { NULL, NULL, 0, 0, size,
+ TRUE, TRUE, FALSE };
==
Again, non-constant initializer for a structure member.
=======
+static GtkIconSet *
+sized_icon_set_from_inline (const guchar *inline_data,
+ GtkIconSizeType size)
+{
+ GtkIconSet *set;
+
+ GtkIconSource source = { NULL, NULL, 0, 0, size,
+ TRUE, TRUE, FALSE };
+
+ set = gtk_icon_set_new ();
+
+ source.pixbuf = gdk_pixbuf_new_from_inline (inline_data, FALSE);
+
+ g_assert (source.pixbuf);
+
+ gtk_icon_set_add_source (set, &source);
+
+ return set;
+}
=======
This leaks the pixbuf, since add_source is going to ref it.
(same for unsized_icon_set_from_pixbuf)
======
+static void
+add_sized (GtkIconFactory *factory,
+ const guchar *inline_data,
+ GtkIconSizeType size,
+ const gchar *stock_id)
=======
Parameters need to be aligned. (also a lot of other functions -
add_unsized, gtk_get_icon_size()...)
========
+ {
+ if (source->filename == NULL)
+ {
+ g_warning ("Useless GtkIconSource contains NULL filename and pixbuf");
+ return NULL;
+ }
========
Shouldn't this warning be in gtk_icon_set_add_source()?
========
+ factories = g_slist_copy (rc_style->icon_factories);
+ if (factories)
+ {
+ GSList *iter;
+
+ iter = factories;
+ while (iter != NULL)
=========
I'd marginally prefer if you stuck to the GTK+ convention and used 'tmp_list' not 'iter'.
=======
+ while (i < n_items)
+ {
+ gpointer old_key, old_value;
+ const GtkStockItem * item = &items[i];
=======
^ close up
====
+ { GTK_STOCK_DIALOG_INFO, N_("Information"), 0, 0, GTK_DOMAIN },
====
Where is GTK_DOMAIN coming from? The rest of GTK+ uses GETTEXT_PACKAGE for this...
============
+/* Internal */
+
+GdkPixbuf * _gtk_default_render_icon (GtkStyle *style,
+ const GtkIconSource *source,
+ GtkTextDirection direction,
+ GtkStateType state,
+ GtkIconSizeType size,
+ GtkWidget *widget,
+ const gchar *detail);
============
It would be better to avoid having internal functions in public headers - if nothing
else, speeds up compilation speeds. Not really sure how to handle this one
- there may not be a convenient place for it.
==============
+GdkPixbuf*
+gtk_widget_get_icon (GtkWidget *widget,
+ const gchar *stock_id,
+ GtkIconSizeType size,
+ const gchar *detail)
==============
I might want to rename both this and gtk_icon_set_get_icon() to
either fetch_icon() or render_icon(). The idea here is that
get_icon() would return an icon owned by the widget, which it doesn't.
create_icon() would imply that the icon could be modified afterwards,
while it can't be. So, that's why I like fetch_icon().
=====
+ icon = find_in_cache (icon_set, style, direction,
+ state, size);
+
+ if (icon)
+ {
+ g_object_ref (G_OBJECT (icon));
+ add_to_cache (icon_set, style, direction, state, size, icon);
+ return icon;
+ }
=====
Since add_to_cache doesn't move the icon to the beginning of the
cache, I find this rather odd.... if you look up the same icon
over and over, you fill the cache with it.
======
+static void
+add_to_cache (GtkIconSet *icon_set,
+ GtkStyle *style,
+ GtkTextDirection direction,
+ GtkStateType state,
+ GtkIconSizeType size,
+ GdkPixbuf *pixbuf)
+{
+ CachedIcon *icon;
+
+ g_object_ref (G_OBJECT (pixbuf));
+
+ /* We have to ref the style, since if the style was finalized
+ * its address could be reused by another style, creating a
+ * really weird bug
+ */
+
+ if (style)
+ g_object_ref (G_OBJECT (style));
======
Ref'ing the style is going to cause memory bloat for theme switching...
Would it be better to use object data to get destroy notification
on styles? If you put a list of iconsets using that style in
object data for each style, then the memory overhead isn't that
bad, though there are some significant speed hits to keep those
lists up to date.
=====
+static GSList*
+direction_state_size_matches (GSList *sources,
+ GtkTextDirection direction,
+ GtkStateType state,
+ GtkIconSizeType size)
=====
A more efficient way of implementing this is to sort
the icon sources as you add them by priority of direction
state, and size, in each category with the ordering being
wildcarded after non-wildcarded.
That is, the sort function is:
gint
compare_sources (GtkIconSource *a, GtkIconSource *b)
{
if (b->any_direction && !a->any_direction)
return -1;
else if (a->any_direction && !b->any_direction)
return 1;
else if (b->any_state && !a->any_state)
return -1;
else if (a->any_state && !b->any_state)
return 1;
else if (b->any_size && !a->any_size)
return -1;
else if (a->any_size && !b->any_size)
return 1;
}
Then, you simply pick the first match in the list. Inserting sources
is going to be a little slower, but you avoid a lot of allocation of
temporary lists, etc.
======
+struct _GtkIconSource
+{
+ /* Either filename or pixbuf can be NULL. If both are non-NULL,
+ * the filename gets ignored.
+ */
+ const char *filename;
+ GdkPixbuf *pixbuf;
======
Sort of a weird comment ... 'the /source/ gets ignored' right?
===
+void gtk_icon_set_add_source (GtkIconSet *icon_set,
+ const GtkIconSource *source);
===
Header alignment
=====
+void
+gtk_image_get_pixmap (GtkImage *image,
+ GdkPixmap **pixmap,
+ GdkBitmap **mask)
+{
+ g_return_if_fail (GTK_IS_IMAGE (image));
+ g_return_if_fail (image->storage_type == GTK_IMAGE_PIXMAP ||
+ image->storage_type == GTK_IMAGE_EMPTY);
+
+ if (image->storage_type == GTK_IMAGE_EMPTY)
+ {
+ image->data.pixmap.pixmap = NULL;
+ image->data.pixmap.mask = NULL;
+ }
+
+ if (pixmap)
+ *pixmap = image->data.pixmap.pixmap;
+
+ if (mask)
+ *mask = image->data.pixmap.mask;
+}
====
Hmmm, I think it is considerably cleaner to do:
if (pixmap)
*pixmap = (image->storage_type == IMAGE_EMPTY) ? image->data.pixmap.pixmap : NULL;
if (mask)
*mask = (image->storage_type == IMAGE_EMPTY) ? image->data.pixmap.mask : NULL;
or something like that.
======
+ if (gtk_stock_lookup (stock_id, &item))
+ {
+ gtk_image_set_from_stock (GTK_IMAGE (dialog->image), stock_id, GTK_ICON_DIALOG);
+
+ gtk_window_set_title (GTK_WINDOW (dialog), item.label);
+ }
=======
The title should only be set here if the user hasn't set it explicitely
for the GtkWindow... That, of course, will be hard to implement - the
best I can think if is:
static const char *gtk_message_dialog_title_key = "gtk-message-dialog-title";
char *old_title = gtk_object_get_data (window, gtk_message_dialog_title_key);
if (!GTK_WINDOW (dialog)->title ||
(old_title && strcmp (GTK_WINDOW (dialog)->title, old_title == NULL)))
{
gtk_window_set_title (GTK_WINDOW (dialog), item.label);
gtk_object_set_data (GTK_WINDOW (data), gtk_message_dialog_title_key,
g_strdup (item.label), (GDestroyNotify)g_free);
}
Or, virtualize gtk_window_set_title(). But I think we should support
creating a message dialog that has "GIMP PNG Error" in the title in
some fashion...
======
+GtkTextBTree* _gtk_text_buffer_get_btree (GtkTextBuffer *buffer);
======
This might be a place where:
#define _GTK_TEXT_BUFFER_BTREE (buffer->tree ? buffer->tree :
_gtk_text_buffer_get_btree (buffer));
could be justified... there are a _lot_ of these calls.
=========
+gboolean
+gtk_text_iter_backward_search (GtkTextIter *iter,
+ const char *str,
+ gboolean visible_only,
+ gboolean slice)
+{
+ g_return_val_if_fail (iter != NULL, FALSE);
+ g_return_val_if_fail (str != NULL, FALSE);
+
+
+
+}
+
/*
* Comparisons
*/
=========
Needs a FIXME or a fix.
========
- /* pango_layout_index_to_pos() expects the index of a character within the layout,
- * so we have to special case the last character. FIXME: This should be moved
- * to Pango.
- */
- if (gtk_text_iter_ends_line (iter))
========
Don't forget to commit this change to Pango.
=========
+gtk_text_tag_table_foreach(GtkTextTagTable *table,
+ GtkTextTagTableForeach func,
+ gpointer data)
{
+ struct ForeachData d;
+
g_return_if_fail(GTK_IS_TEXT_TAG_TABLE(table));
g_return_if_fail(func != NULL);
- g_hash_table_foreach(table->hash, func, data);
+ d.func = func;
+ d.data = data;
+
+ g_hash_table_foreach(table->hash, hash_foreach, &d);
}
=========
Shouldn't this foreach over table->anonymous as well?
============
+gchar*
+g_convert (const gchar *str,
+ gint len,
+ const gchar *to_codeset,
+ const gchar *from_codeset,
+ gint *bytes_converted)
============
This looks basically pretty good and ready for GLib - a few problems:
- The output string might be wide character. In this case, the null
termination isn't sufficient to tell how much was converted.
Perhaps there should be a *bytes_written parameter as well?
======
+ if (errno == E2BIG)
+ {
[...]
+ }
+ else
+ g_warning ("iconv() failed: %s", strerror (errno));
======
- At least when bytes_converted != NULL, you should not warn about
EINVAL, since that just means that the input string contains
incomplete characters.
======
@@ -1948,7 +2049,7 @@
*end = *center;
if (gtk_text_iter_backward_find_char (start, not_whitespace, NULL))
- gtk_text_iter_forward_char (start); /* we want the first whitespace... */
+ gtk_text_iter_next_char (start); /* we want the first whitespace... */
if (whitespace (gtk_text_iter_get_char (end), NULL))
gtk_text_iter_forward_find_char (end, not_whitespace, NULL);
======
- This looks like it isn't properly converted to using log_attrs...
=============
@@ -13,9 +13,9 @@
GTK_TEXT_MOVEMENT_CHAR, /* move by forw/back chars */
GTK_TEXT_MOVEMENT_POSITIONS, /* move by left/right chars */
GTK_TEXT_MOVEMENT_WORD, /* move by forward/back words */
- GTK_TEXT_MOVEMENT_LINE, /* move up/down lines (wrapped lines) */
- GTK_TEXT_MOVEMENT_PARAGRAPH, /* move up/down paragraphs (newline-ended lines) */
- GTK_TEXT_MOVEMENT_PARAGRAPH_ENDS, /* move to either end of a paragraph */
+ GTK_TEXT_MOVEMENT_WRAPPED_LINE, /* move up/down lines (wrapped lines) */
+ GTK_TEXT_MOVEMENT_LINE, /* move up/down paragraphs (newline-ended lines) */
+ GTK_TEXT_MOVEMENT_LINE_ENDS, /* move to either end of a paragraph */
GTK_TEXT_MOVEMENT_BUFFER_ENDS /* move to ends of the buffer */
} GtkTextViewMovementStep;
==========
if you find using LINE too confusing for wrapped line, maybe this
shoudl be WRAPPED_LINE/PARAGRAPH? It's a little hard for me to
figure out which of LINE/WRAPPED_LINE is which.
I guess with these changes:
void gtk_text_layout_move_iter_to_previous_line (GtkTextLayout *layout,
GtkTextIter *iter);
should be
void gtk_text_layout_move_iter_to_previous_wrapped_line (GtkTextLayout *layout,
GtkTextIter *iter);
=====
+void gtk_text_view_set_editable (GtkTextView *text_view,
+ gboolean setting);
+gboolean gtk_text_view_get_editable (GtkTextView *text_view);
+
+void gtk_text_view_set_cursor_visible (GtkTextView *text_view,
+ gboolean setting);
+gboolean gtk_text_view_get_cursor_visible (GtkTextView *text_view);
=====
Do we need set_cursor_visible() separate from set_editable()? Examples?
=====
+GtkAccelGroup*
+gtk_window_get_accel_group (GtkWindow *window)
+{
+ GtkAccelGroup *group;
+
+ g_return_val_if_fail (GTK_IS_WINDOW (window), NULL);
+
+ group = gtk_object_get_data (GTK_OBJECT (window),
+ "__gtk_accel_group");
+
=====
As above, we just use "gtk-accel-group"
=========
gtk_window_set_transient_for (GtkWindow *window,
GtkWindow *parent)
{
- g_return_if_fail (window != 0);
+ g_return_if_fail (GTK_IS_WINDOW (window));
+ g_return_if_fail (parent == NULL || GTK_IS_WINDOW (parent));
+ g_return_if_fail (window != parent);
+
+ gtk_signal_emit (GTK_OBJECT (window),
+ window_signals[SET_TRANSIENT_FOR],
+ parent);
+}
- if (window->transient_parent)
- {
- if (GTK_WIDGET_REALIZED (window) &&
==========
We agreed to remove the virtualization of set_transient_for() from
your patch, right?
=========
--- gtk/testgtk.c 2000/06/24 22:32:05 1.186
+++ gtk/testgtk.c 2000/06/28 19:43:16 1.186.4.3
@@ -166,8 +166,13 @@
if (!window)
{
+ GtkAccelGroup *accel_group;
+
window = gtk_window_new (GTK_WINDOW_TOPLEVEL);
+ accel_group = gtk_accel_group_new ();
+ gtk_window_add_accel_group (GTK_WINDOW (window), accel_group);
+
gtk_signal_connect (GTK_OBJECT (window), "destroy",
GTK_SIGNAL_FUNC (gtk_widget_destroyed),
&window);
=======
You might as well use gtk_window_get_acel_group(), Also, that
will avoid the leak of the accel group you have here.
=====
+style "myicons"
+{
+ stock["Gtk_Warning_Dialog"] = { ("stock-icons/dialog_error.png",
+ *,
+ *,
+ *) }
+}
+
+class "GtkImage" style "myicons"
====
You probably don't want to leave this in - it will look like an
bug. There are more wacky pixmaps around in the gtk/ directory you
could use, or we could draw something up special for the test.
=====
int
-main(int argc, char** argv)
+main (int argc, char** argv)
{
Buffer *buffer;
View *view;
int i;
- gtk_init(&argc, &argv);
-
+ gtk_init (&argc, &argv);
+ gdk_rgb_init (); /* FIXME remove this */
+
====
yes, fix this and remove this :-). gdk_rgb_init() is a no-op in
HEAD.
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
