Re: gtk-hp-patches
- From: Owen Taylor <otaylor redhat com>
- To: Tim Janik <timj gtk org>
- Cc: gtk-devel-list gnome org
- Subject: Re: gtk-hp-patches
- Date: 23 Jul 2000 23:57:19 -0400
Tim Janik <timj@gtk.org> writes:
> 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.
I am somewhat amused. The whole point of an assertion is that it _is_
superfluous! As soon as your assertion isn't superfluous, then your
code is broken.
I do agree that it probably isn't necessary to have an assertion at
this spot; that the condition is pretty trivially true. But,
in general, I think it if you are using assertions, they should
be asserting things that are reasonably easy to verify be hand.
The idea that assertions should be checking complex things that
you wouldn't be able to verify by hand is broken - if, like
in the old text widget, your invariants are so complexely
maintained that your assertions are non-trivial, then your
code is never going to work right.
[... more examples of rather trivial assertions omitted ...]
> +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
Yes, you just analyzed the code and determined that the assertion
was satisfied. Whee! the code isn't buggy!
The point of the assertion is that it is like writing
/* when we call g_main_quit, we destroy ri.loop and set it to NULL */
Except that the compiler is checking that that is in fact true,
and if somebody later adds some other code that calls g_main_quit()
but doesn't destroy the loop, we'll find it out.
> 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
But that's why we don't use gtk_main()...
> 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().
If the library user could violate the assertion, then g_assert()
would be wrong, yes.
> [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,
"expensive" doesn't matter here. This is gtk_dialog_run() which
get called once when showing a dialog.
> and makes the code less crufty (shorter and leaner by getting rid of his
> temporary run data).
And at the cost of introducing global state in the widget structure as
opposed to purely local state. One should definitely prefer local
state to global state.
I'd guess it's probably close to a wash in terms of total code
cleanliness - some advantages either way. It's not something I'm going
to be passionate about.
> > > 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??
Your gtk_dialog_run() does something different... you can't compare
it. To implement the same semantics, you'd be adding code into
your function and elsewhere.
> > (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.
You can abort using gtk_dialog_response (dialog, -1); (or whatever
response code you want to return.) I suggested to Havoc that he might
want to introduce a gtk_dialog_abort() or something that did the same
without emitting ::response; I forget what we decided against that or
simply forgot about it.
Using gtk_main() here would be broken, since it would be reintroducing
the problem of quiting the wrong main loop that we've had in the past
in various places. There is a reason why I didn't use a main loop
stack like gtk_main() in GLib.
> > > 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
If the lib autohides when a button is clicked, then if you want to
leave the dialog up, you need to show it again and flashes. So, then
the lib needs to have to add extra functions to set the policy and you
have increased complexity.
The way it is now, you don't need lots of different policies, and you
can get any effect I can imagine wanting in just a few lines of code.
response = gtk_dialog_run (GTK_DIALOG (dialog));
gtK_widget_hide (dialog);
response = gtk_dialog_run (GTK_DIALOG (dialog));
gtK_widget_destroy (dialog);
do
{
response = gtk_dialog_run (GTK_DIALOG (dialog));
if (response == APPLY || response == OK)
do_apply ();
if (response == OK || response == CANCEL)
break;
}
etc.
> > 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.
GnomeDialog is really nasty to use because there are all
sorts of policy settings to set. It means you can't tell what
some code using GnomeDialog is doing unless you know what
options are set on the dialog.
Also, as far as the user goes, at least 90% of the uses will be people
_either_ connecting to ::response or using gtk_dialog_run().
(So far, for me, using GnomeDialog, it's been 100%, but I'll
be open-minded here.)
Optimizing for ease of use in mixed case should not be a priority.
> 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);
There is a idiom for this with the current setup:
dialog = gtk_dialog_new_with_buttons ("My Dialog", parent, 0,
GTK_STOCK_BUTTON_APPLY, FALSE,
GTK_STOCK_BUTTON_OK, TRUE,
GTK_STOCK_BUTTON_CANCEL, TRUE);
[ make crazy connections ]
while (!gtk_dialog_run (dialog))
/* loop */;
> 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)
Why is it a bad kludge?
> 2) works well with auto-hide/auto-destroy upon response behaviour.
combining auto-hide/auto-destroy with gtk_dialog_run() is pretty
dubious. The only excuse for auto-hide or auto-destroy is when
you want to show-and-forget a dialog.
> 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.
The current approach (which is basically what Federico suggested
a while ago) is a nice balance between:
- maximum compactness of code using the dialog
- keeping things really easy to understand.
>From my perspective, any sort of policy setting that changes
behavior is evil and confusing. The current approach is
convenient without requiring policy settings.
> > > - 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...
>
> [...]
>
> > /* 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);
I'd like to avoid adding more arguments to the constructor. It makes
it hard to remember what the arguments are both when reading and
writing.
If you think the old fashion gtk_widget_destroyed connection is to
complicated, then we should introduce:
gtk_widget_null_on_destroy (GtkWidget *widget, GtkWidget **ptr);
[..]
> > > - 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.
Hmmm, these are all your functions, and rather similar ones
at that. I don't think they are representative. To present
a few counter-examples:
/* Compute a widget's path in the form "GtkWindow.MyLabel", and
* return newly alocated strings.
*/
void gtk_widget_path (GtkWidget *widget,
guint *path_length,
gchar **path,
gchar **path_reversed);
void gtk_widget_class_path (GtkWidget *widget,
guint *path_length,
gchar **path,
gchar **path_reversed);
void gtk_pixmap_get (GtkPixmap *pixmap,
GdkPixmap **val,
GdkBitmap **mask);
void gdk_query_depths (gint **depths,
gint *count);
void gdk_query_visual_types (GdkVisualType **visual_types,
gint *count);
These are are all old functions (Peter's functions, in fact) - I've
skipped the examples I found of things I've added in G[DT]K+-2.0 and
examples from Pango. There are quite a few more. The main example
I'd find of out params plus return values are things like:
gint gtk_clist_get_pixmap (GtkCList *clist,
gint row,
gint column,
GdkPixmap **pixmap,
GdkBitmap **mask);
Where the return value is a status. Not the case for
gtk_image_get_icon_set.
> > > - 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.
[...]
> > - 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.
Yes, you are probably right that curly braces are better. (Though
resemblance to C syntax is certainly only approximate...)
> > > 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):
Considering how hard it is to find unique 4 letter acronyms, I
almost tend to think that picking a random number is probably safer...
But in this case, especially since we aren't defining a file format,
it doesn't matter much and I'd be quite happy with "GdkP".
> 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" */
You've found at least one example intentionally picked magic that is
worse than random magic - 0xdeadbeef ... since that is a) the
magic "fill freed memory constant" for some malloc's b) likely
to be picked by someone else as well.
> 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
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
