Re: gtk-hp-patches

[Tim Janik <timj gtk org>]

On 23 Jul 2000, Owen Taylor wrote:

> Tim Janik <timj@gtk.org> writes:

> > - you use a couple of g_assert()s in your code. if you watch current
> > gtk closely, we usually don't use g_assert() at all, because we
> > want prefer to keep the library warning&working instead of dying.
> > get rid of your g_assert()s where superflous, and use g_return_if_fail()
> > instead if the assertion is required.
> 
> I think it is fine to have g_assert() in places where whatever
> the user of the library does the assertion should never be hit.
> I don't think
> 
>  g_return_if_fail (library_is_broken)
> 
> Makes any real sense. g_return_if_fail() should be when the  
> user gives incorrect input, but g_assert() is fine if it
> is asserting things abou tthe internal state of the library.
> (As long as the assertions are correct - incorrect assertions
> are bad news - speaking from experience with the old GtkText 
> widget.)

urm, while we don't normaly use assertions, you make me dig up
examples, but ok:

+GdkPixbuf*
+gtk_icon_set_get_icon (GtkIconSet      *icon_set,
+                       GtkStyle        *style,
+                       GtkTextDirection   direction,
+                       GtkStateType     state,
+                       GtkIconSizeType  size,
+                       GtkWidget       *widget,
+                       const char      *detail)
[...]
+  if (source->pixbuf == NULL)
+    {
+      if (source->filename == NULL)
+        {
+          g_warning ("Useless GtkIconSource contains NULL filename and pixbuf");
+          return NULL;
+        }
+
+      source->pixbuf = gdk_pixbuf_new_from_file (source->filename);
+
+      /* This needs to fail silently and do something sane.
+       * A good argument for returning a default icon.
+       */
+      if (source->pixbuf == NULL)
+        return NULL;
+    }
+
+  g_assert (source->pixbuf != NULL);

superfluous.

+  while (token != G_TOKEN_RIGHT_CURLY)
+    {
+      if (icon_set == NULL)
+        icon_set = gtk_icon_set_new ();
+
+      token = gtk_rc_parse_icon_source (scanner, icon_set);
+      if (token != G_TOKEN_NONE)
+        {
+          g_free (stock_id);
+          gtk_icon_set_unref (icon_set);
+          return token;
+        }
+
+      token = g_scanner_get_next_token (scanner);
+
+      if (token != G_TOKEN_COMMA &&
+          token != G_TOKEN_RIGHT_CURLY)
+        {
+          g_free (stock_id);
+          gtk_icon_set_unref (icon_set);
+          return G_TOKEN_RIGHT_CURLY;
+        }
+    }
+
+  g_assert (token == G_TOKEN_RIGHT_CURLY);

superfluous.

+void
+gtk_window_set_destroy_with_parent  (GtkWindow *window,
+                                     gboolean   setting)
+{
+  g_return_if_fail (GTK_IS_WINDOW (window));

-      gtk_window_unset_transient_for (window);
-    }
+  if (window->destroy_with_parent == (setting != FALSE))
+    return;

-  window->transient_parent = parent;
+  if (window->destroy_with_parent)
+    {
+      g_assert (!setting);

-  if (parent)
+      disconnect_parent_destroyed (window);
+    }
+  else
     {
-      gtk_signal_connect (GTK_OBJECT (parent), "destroy",
-                         GTK_SIGNAL_FUNC (gtk_widget_destroyed),
-                         &window->transient_parent);
-      gtk_signal_connect (GTK_OBJECT (parent), "realize",
-                         GTK_SIGNAL_FUNC (gtk_window_transient_parent_realized),
-                         window);
-      gtk_signal_connect (GTK_OBJECT (parent), "unrealize",
-                         GTK_SIGNAL_FUNC (gtk_window_transient_parent_unrealized),
-                         window);
+      g_assert (setting);

-      if (GTK_WIDGET_REALIZED (window) &&
-         GTK_WIDGET_REALIZED (parent))
-       gtk_window_transient_parent_realized (GTK_WIDGET (parent),
-                                             GTK_WIDGET (window));
+      connect_parent_destroyed (window);
     }
+
+  window->destroy_with_parent = setting;
 }

two superfluous assertments, and then the code even manages to
fall into the setting > TRUE trap.

+gint
+gtk_dialog_run (GtkDialog *dialog)
[...]
+  g_main_run (ri.loop);
+
+  g_assert (ri.loop == NULL);

there are two cases where g_main_run() will return:
1) an action widget was activated, in which case ri.loop is NULL, enforced
   by the dialog code
2) someone else called g_main_quit (ri.loop) which isn't possible because
   ri.loop isn't exposed
suppose you'd make the above g_main_run (ri.loop); gtk_main(); instead,
you'd abort if somewhere else g_main_quit() is called. in that case you
wouldn't want the program to simply abort, but either:
a) puke and return some default result code
b) support this situation, remain silent and return some default result code

for gtk_main() and a) you'd use g_return_if_fail() or g_warning(), but in
no case should there be a g_assert().

[reordering the quoted email here, to preserve context]
> > - on gtk_dialog_run(), you still use signal connections/disconnections
> >   instead of overriding GtkDialog class methods here.
> 
> Since the desired effect is different behavior during the
> run, than at other times, and since 

that's best covered by a state flag in the widget structure.
using one state variable here gets rid of expensive connect/disconnect,
and makes the code less crufty (shorter and leaner by getting rid of his
temporary run data).

> >  also, you still go the complicated route of setting up a new main
> >  loop.
> 
> What's so complicated about setting up a main loop? Why not 
> prefer that to implementing your own?

simply because using a state field and proper own member functions that
maintain the state field gets rid of the mainloop maintenance code plus
the signal connection stuff. isn't the easy of that implementation
obvious from the pure _size_ difference of my gtk_dialog_run() and
havoc's??

>  (Note that if you implement
> your own, then run() will break in anything  but the main thread,
> while I'm going to fix that in GMain...)

well, if you want proper abortion support, you need to call gtk_main()
anyways, instead of your own g_main_run (myloop), as explained above.

> >   since your GtkDialog version allowes the dialog to stay shown even
> >   after gtk_dialog_run() exits, you don't need to rely on
> >   GTK_WIDGET_VISIBLE() for the abortion condition, a widget member
> >   variable would be just as easy.
> 
> I think Havoc's implementation is fine. Since we aren't
> requiring the dialog to be hidden (which be considerably
> less convenient and require more complexity in button
> handling to prevent hide/show flashing), I think it work

i'm not arguing against the approach of being able to leave
the dialog shown after response, but i think functionality
should be provided to auto-hide upon response and to
auto-destroy either on hide or on response.
i don't get your complexity point wrg "button flashing" though

> out nicer to use GMainLoop - which is just a gboolean
> really (except for the thread issues mentioned above) 
> than to do it yourself. 

that boolean would just be part of the above mentioned state field,
but that doesn't need to be GTK_WIDGET_VISIBLE as i said already.

> > - another point you missed is that you may want to have buttons in
> >   the action area that don't abort gtk_dialog_run(), suppose you
> >   have an mp3 selection dialog, with three action area buttons "Ok",
> >   "Dismiss" and "Prehearse", the first would abort gtk_dialog_run()
> >   with their appropriate response code, while "Prehearse" would just
> >   play part of the mp3 through a normal ::clicked connection and
> >   not affect the dialogs behaviour at all (don't nail me on the mp3
> >   thingy here, there are countless other examples if you don't like
> >   this one).
> 
> If you don't want it to abort, do it in a loop. It keeps
> things a _lot_ simpler, then try to make some buttons abort,
> and some not abort... 

woowoo, wait here...
it _might_ make things easier on gtkdialog implementator side,
but not on the user side.
regarding the API, there's one small change/addition required,
either:

 void gtk_dialog_add_action_widget  (GtkDialog *dialog,
                                     GtkWidget *widget,
+                                    gboolean   abort_run,
                                     gint       response_id);
or alternatively:

 /* doesn't abort gtk_dialog_run() */
 void gtk_dialog_add_action_widget  (GtkDialog *dialog,
                                     GtkWidget *widget);
 /* does abort gtk_dialog_run() */
 void gtk_dialog_add_response_widget  (GtkDialog *dialog,
                                       GtkWidget *widget,
                                       gint       response_id);

and that
1) gets rid of user code being in the need to do special
   casing to support "loop" invocations of gtk_dialog_run()
   (which is a bad cludge anyways)
2) works well with auto-hide/auto-destroy upon response behaviour.

havoc's current approach being "a _lot_ simpler" is simply not
true, not even for the gtkdialog.c implementation, as all
you really have to do is to _skip_ one signal connection you
currently make.

> > - also, you still don't support a GtkDialog **dialog_p, upon
> >   creation that helps people keeping track of dialog caching
> >   (i.e. reuse the same dialog for the same questions instead
> >   of recreating it over and over) and could cure a lot of cases
> >   where dialogs ala "Really close this Window?" keep popping up
> >   for the same window.
> 
> I don't follow what you are suggesting here ... could you be more
> explicit? 

lemme dig up the relevant email...

Date: Sat, 25 Mar 2000 02:09:13 +0100 (CET)
From: Tim Janik <timj@gtk.org>
Reply-To: gtk-devel-list@redhat.com
To: gtk-devel-list@redhat.com
Subject: Re: dialog enhancements

> mostly from using/looking at other people's code, i figured that many
> applications get the widget creation of their labels/entries etc. right,
> what is often done in a wrong way though (or simply not paid
> attention to), are:
[...]
> - lifetime specification; this is the decision of creating a dialog on demand
>   and destroying it upon hiding, or keeping it around for later reuse
[...]
> - pointer/memory maintenance; some apps don't keep track of dialog invokation,
>   which can lead to multiple dialogs of the same kind being popped up (e.g.
>   the above "Really close this window? Y/N" type). some applications that do
>   try to keep track get the destruction notification wrong (or don't bother),
>   or attempt to use reference counting (which is utterly wrong for this case)

[...]

> /* gtk_aux_dialog_new() creates an auxillary dialog
[...]
>  * aux_dialog_p
>  *      pointer to a widget pointer which will be set to NULL once the
>  *      dialog is destroyed, may be NULL
[...]
>  */
> GtkWidget* gtk_aux_dialog_new (GtkWidget        *context_parent,
>                                const gchar      *dialog_title
>                                GtkWidget        *content_child,
>                                GtkAuxDialogFlags flags,
>                                GtkWidget       **aux_dialog_p);


> > - you introduce a couple of new rcfile tokens, without giving an example
> > rcfile that uses them or outlining their usage somewhere:
> > @@ -211,6 +215,14 @@
> >    { "highest", GTK_RC_TOKEN_HIGHEST },
> >    { "engine", GTK_RC_TOKEN_ENGINE },
> >    { "module_path", GTK_RC_TOKEN_MODULE_PATH },
> > +  { "stock", GTK_RC_TOKEN_STOCK },
> > +  { "LTR", GTK_RC_TOKEN_LTR },
> > +  { "RTL", GTK_RC_TOKEN_RTL },
> > +  { "MENU", GTK_RC_TOKEN_MENU },
> > +  { "BUTTON", GTK_RC_TOKEN_BUTTON },
> > +  { "SMALL_TOOLBAR", GTK_RC_TOKEN_SMALL_TOOLBAR },
> > +  { "LARGE_TOOLBAR", GTK_RC_TOKEN_LARGE_TOOLBAR },
> > +  { "DIALOG", GTK_RC_TOKEN_DIALOG }
> >  };
> > i'd like to see how you intend these to be used and why they should
> > be uppercase tokens.
> 
> These correspond to enum values, so having them uppercase
> corresponds to bg[NORMAL]. 

ok, i'd still like to see examples for this though.

> > - change:
> > void       gtk_image_get_icon_set (GtkImage        *image,
> >                                    GtkIconSet     **icon_set,
> >                                    GtkIconSizeType *size);
> > to:
> > GtkIconSet* gtk_image_get_icon_set (GtkImage        *image,
> >                                     GtkIconSizeType *size);
> 
> IMO - if need to return more then result, then you should all 
> be out parameters; it's confusing to pick one as the "return value".
> I like Havoc's  like version better and it corresponds to most
> of the similar API's in GTK+. 

similar APIs in gtk?

GtkArg*         gtk_args_query           (GtkType       class_type,
                                          ...,
                                          guint        *n_args_p);
GtkArg* gtk_container_query_child_args     (GtkType            class_type,
                                            guint32          **arg_flags,
                                            guint             *nargs);
GtkArg* gtk_object_query_args   (GtkType          class_type,
                                 guint32        **arg_flags,
                                 guint           *n_args);

are the first ones i find that return a structure pointer and an
assorted integer.

> > - in struct _GtkWindow, transient_parent should be of type GtkWidget*
> > so we can get rid of a bunch of needless casts.
> 
> I used GtkWindow because that is the required type for that member
> variable, but I really don't care one the issue. (Except that changing
> it breaks source compat for no good reason.)

source compat breakage amounts to a warning here for direct structure
readouts which i expect to happen pretty rarely for transient_parent.
getting rid of casts (especially superfluous ones) counts as a good
reason in my book though.

> It definitely should stay
> a GtkWindow as a parameter to gtk_window_set_transient_for()

sure.

> > - in the rcfile example to specify icon sources, you use parens () as
> > element delimieters make that consistent with the rest of the rc
> > syntax (inspired by C syntax) by using braces {}.
> 
> I don't know - I prefer the parens here. We don't use braces.
> to group lists of heterogenous elements anywhere else in gtkrc
> files - most of the time we are just grouping statements.

no, we lean towards C, and there you use nested levels of braces
for sub-structure initializations which is what these rc statements
come closest to.
note that we also lean towards C when specifying signal arguments,
similar to how you specify function arguments in C:

binding "clist-test"
{
  bind "j" {
    "scroll-vertical" (step-backward, 0.0)
  }
  bind "k" {
    "scroll-vertical" (step-forward, 0.0)
  }
}

and havoc's stock values don't resemble the process of calling
something for me, it's far closer to your example below:

> The only place I can think of using braces to group a list
> is:
> 
>  bg[NORMAL] = { 1.0, 0.5, 0.3 }

[deleted lots of clunky examples]

> But that strikes me as a little cumbersome. (The formatting 
> in the example is a little odd, something like:
>  
>    stock["Gtk_Warning_Dialog"] = {
>     /* Filename                              Direction  Size    State */
>      ("stock-icons/dialog_error.png",        *,         * ,     *),
>      ("stock-icons/dialog_error_button.png", LTR,       BUTTON  *)
>    }
> 
> Would make it clearer why I think Havoc's approach is less clunky.

that does look more C-ish as:

    stock["Gtk_Warning_Dialog"] = {
     /* Filename                               Direction  Size    State */
      { "stock-icons/dialog_error.png",        *,         *,      * },
      { "stock-icons/dialog_error_button.png", LTR,       BUTTON, * }
    }

and also avoids reminding of function calls. further more, it scales
to deeper nestings ala:

    stock["Gtk_Warning_Dialog"] = {
     /* Filename                               Direction  Size        State */
      { "stock-icons/dialog_error_button.png", LTR,       { 16, 32 }, * }
    }

using parens instead of braces at some arbitrary nesting level is just a
funky idea that doesn't fit in with the rest of the syntax.

> > on a general note, there are lots of things missing in your patch
> > that you already got elaborated comments on in gtk-devel threads.
> > to avoid further lossage, process things in small chunks as i said above,
> > and do write up you ChangeLog entries before suggesting a patch.
> > that'll also help peer reviewers like owen and me to make sure you
> > didn't miss things and fix things small-scale.
> > on the ChangeLog entries, i'd specifically like to see commentary on
> > the purpose of things like the icon factory, GtkStockItem and GtkIconSet.
> > this is not only for you, owen and me, but also to enlighten people that
> > did not review this patch and need/want to fix/change stuff in the future
> > and refer to the ChangeLog to figure original intent.
> 
> Yes, we need ChangeLog entries, and I think Havoc just didn't
> put them in yet so he wouldn't have to write them and then
> go back and edit them. But for future maintainers, what we
> need are _docs_ not ChangeLog entries. That's what's missing
> for me in this patch - API docs.

urm, we need API docs _besides_ ChangeLog entries, not _instead_!

> > things that i spotted, that are still unfinished:
> > 
> > - despite the email discussion you spawned about inlined pixbufs,
> >   your code still contains that random magic
> 
> I don't see the problem with random magic. (OK, it was my
> suggestion). But if it will solves the issue, it would be fine to
> change it to "GdkPxbuf" (it's intentional to make it 8 bytes
> long.)

if you want to keep it at 4 bytes, make it "GdkP", it's just a good idea
to use somewhat meaningfull values for magics and to try to provide
namespaces there as well. a couple examples (random fgrep):

bse/bseglobals.h:#define BSE_MAGIC    (('B' << 24) | ('S' << 16) | ('E' <<  8) | ('!' <<  0))
glame-0.020/src/swapfile/swapfileI.h:#define SWAP_MAGIC "GLAMESWAP0000001"
glame-0.020/src/swapfile/swapfileI.h:#define RECORD_MAGIC "GLRC"
DeCSS/4/LiVid/ac3dec/decode.h:#define DECODE_MAGIC_NUMBER 0xdeadbeef
glibc-2.1.2/timezone/tzfile.h:#define      TZ_MAGIC        "TZif"
gimp/app/pattern_header.h:#define GPATTERN_MAGIC   (('G' << 24) + ('P' << 16) + ('A' << 8) + ('T' << 0))
gdb-4.18/include/aout/adobe.h:#define      ZMAGIC  0xAD0BE         /* Cute, eh?  */
glibtop/gnuserv.h:#define MCOOKIE_NAME   "MAGIC-1" /* authentication protocol name */
glibtop/gnuserv.h:#define MCOOKIE_X_NAME "MIT-MAGIC-COOKIE-1"  /* as needed by X */
mpeglib/lib/tplay/tplayfunctions.h:#define WAVE_MAGIC    0x45564157      /* ASCII: 'WAVE' */
csl/cslmcop.h:  guint32            mcop_magic;     /* 0x4d434f50 "MCOP" */

sure there're probably as many counterexamples, but since you
used rand() anyways, "GdkP" doesn't hurt and could have been
spit out i the first place anyways, right? ;)


> 
> Regards,
>                                         Owen
> 

---
ciaoTJ