Re: gtk-mailund-980804-0.patch.gz

[Owen Taylor <otaylor redhat com>]

Tim Janik <timj@gtk.org> writes:

> hi Thomas,
> 
> i'm just reading through gtk-mailund-980804-0.patch.gz, and
> am thinking about adaptions to get named property data.
> 
> void       gtk_text_set_property_data_functions (GtkText *text,
>                                     gboolean (* compare_data) (gpointer,
>                                                                gpointer),
>                                     gpointer (* clone_data) (gpointer),
>                                     void     (* free_data) (gpointer));
> needs to become:
> 
> typedef gpointer (GtkTextPropertyDataCloneFunc) (gpointer prop_data,
>                                                  gpointer func_data);
> typedef void     (GtkTextPropertyDataFreeFunc)  (gpointer prop_data,
>                                                  gpointer func_data);
> typedef gboolean (GtkTextPropertyDataCompareFunc) (gpointer prop_data_1,
>                                                    gpointer prop_data_2,
>                                                    gpointer func_data);
> void gtk_text_set_property_data_handlers (GtkText *text,
>                                           gchar   *data_key,
>                                           GtkTextPropertyDataCompareFunc comp,
>                                           GtkTextPropertyDataCloneFunc clone,
>                                           GtkTextPropertyDataFreeFunc free,
>                                           gpointer func_data,
>                                           GtkDestroyNotify destroy);
> 
> the func_data argument can be extremely handy if you for instance
> want different bahviours of these functions for different text widgets.
> or, the free() and clone() functions could allocate from a mem chunk,
> which could be passed as func_data. `destroy' is just a destroy
> notification function that is going to be invoked once with `func_data'
> as argument, usually at the end of the lifetime of a text widget (or
> if you reset the functions by calling gtk_text_set_property_data_handlers()
> with the same data_key again).

Hmmm, it would be simpler, if less efficient, to have:

typedef gpointer (GtkTextPropertyDataCloneFunc) (GtkText *text,
						 gpointer prop_data);
typedef void     (GtkTextPropertyDataFreeFunc)  (GtkText *text,
				                 gpointer prop_data);
typedef gboolean (GtkTextPropertyDataCompareFunc) (GtkText  *text,
                                                   gpointer prop_data_1,
                                                   gpointer prop_data_2);
 
And simply let the user use object data on the Text widget if
they so care. It's at least another possibility.

> the `data_key' is meant to work similar to GtkObject data keys, it is
> used to associate these function pointers with the property data keys.
> 
> we need to add `data_key' to the other functions as well, to get the
> data->data_key associations going for the actuall data pointers as well:
> 
> void     gtk_text_insert_with_data (GtkText       *text,
>                                     GdkFont       *font,
>                                     GdkColor      *fore,
>                                     GdkColor      *back,
>                                     const char    *chars,
>                                     gint           length,
>                                     gchar         *data_key,
>                                     gpointer       prop_data);

> void      gtk_text_set_property    (GtkText       *text,
>                                     guint          from,
>                                     guint          to,
>                                     GdkFont       *font,
>                                     GdkColor      *fore,
>                                     GdkColor      *back,
>                                     gchar         *data_key,
>                                     gpointer       prop_data);

Several things. To be consistent with the rest of the
Text widget API, it should start from the point and change
the next n characters.

void      gtk_text_set_property       (GtkText       *text,
			               GdkFont       *font,
				       GdkColor      *fore,
				       GdkColor      *back,
                                       gchar         *data_key,
                                       gpointer       prop_data,
				       guint          length);

The way Thomas's set_property works now, the key/value thing doesn't
really work the way that we are thinking of it, because it completely
replaces the existing properties with the new ones. So perhaps
it should be changed to gtk_text_change_properties() with the
semantics that it overlays the specified properties on to
of the existing properties for the span, creating new properties
as necessary.

(That is, if I apply the 'red' foreground to some portion of
a string in 'courier', with

 gtk_text_change_properties (text, NULL, red, NULL, NULL, NULL, 10);

then I get red courier text.

But this raises and additional problem is that there are two distinct
meanings for 'font' 'fore' and 'back' being equal to NULL -
 
 1) Same as current values for the text.
 2) Same as default.

This becomes an especially important distinction when you
allow changing the style of a text widget on the fly.
(This doesn't work now, but it will work for 1.2).

So, either we have to add a flags field, or split this into:

 gtk_text_change_font ()
 gtk_text_change_fore ()
 gtk_text_change_back ()
 gtk_text_change_data ()

(which could all be frontends to an internal function with
 a flags field)

> void    gtk_text_set_property_data (GtkText     *text,
>                                     guint        pos,
>                                     gchar       *data_key,
>                                     gpointer     prop_data);

I actually think this doesn't belong. The Text widget _API_,
as it exists now, has no concept of a property extending
over a span of text - the only concept it has is that a
certain character has certain properties. So, in that sense,
the only API that makes sense is gtk_text_change_properties().


> gpointer gtk_text_get_property_data (GtkText     *text,
>                                      guint        pos,
>                                      gchar       *data_key);
 
> for the actuall implementation, i'm going to export the core functionality
> of the GtkObject data mechanism in a new glib function, so we don't need
> to duplicate that functionality all over the place.
> we will the keep a list of name->data association pairs on the text properties,
> and another list for the name->function pointers associations in the text
> widget itself.
> for comparision of properties, we will keep the prop_data pointers sorted
> and can then compare them one by one with the compare function, if their
> data_keys match. if the data_keys don't match for e.g. the second prop_data
> on both properties, then that means that at least one of them has data
> associated with it, that the other hasn't and so we can immediatedly
> consider them not equal.
 
> [owen, could you look over the patch, especially the portions that
>  are not directly property data related, so i got a code base to work from?]

- Hmm, one problem with the patch is that it includes fixes that
are already in CVS, so it is a little bit hard to read.

- There are trivial indentation problems that you no doubt noticed

if (foo) {
  bar();
}

should be:

if (foo) 
  {
    bar();
  }

- gtk_text_set_set_property should not freeze() / thaw() unconditionally,
  since that will thaw a previously frozen text widget.

- This all conflicts somewhat some changes I have around here to
  allow properties to store the fact they have the default values.
  (But don't worry about that - they may not make it for
  another week or two, and I can deal with merging at that
  point)

- I'd rather see x >= y, than x > y - 1.

But otherwise, the changes, at least reading them, look fine.

Regards,
                                        Owen