Re: gtk-hp-patches

[Owen Taylor <otaylor redhat com>]

I have my own, much more detailed-oriented commentary on
the patch set, but I wanted to comment on some of Tim's
comments here.

Tim Janik <timj@gtk.org> writes:

> the way you let things pile up creates a huge workload now.
> for the future, do _one_ thing -say stock icons- first, lets
> discuss that in enough detail and then commit. then walk on
> to the next topic, e.g. GtkDialog. that process makes sure
> we hash one thing out as detailed as needed, and comments/
> suggestions don't get lost on the way. it also avoids owen
> and me wading through a 14k patch trying to keep all of its
> contents in mind while commenting on it in a detailed fashion.
> 
> with what we have now, _here_ is a 14k line patch you wait to
> be approved, and _there_ is a huge email archive with the latest
> corrections/suggestions, spread out through various threads that
> you didn't bother incorporating.
> 
> if you don't believe one thing at a time gets us better and
> cleaner results in the end, i refer you to linus responses on
> huge patches, he even denies to review those untill someone bites
> the apple to split them up into appropriate chunks again.
> and for the patch at hand, things can very well be split up in
> a meaningfull order, i'm making appropriate suggestions on that
> at the end of this email.

I'd agree it's a bigger patch than I like, but since both 
of us (especially me) have posted patches of this size
which _don't_ split into neat chunks, I'm not too worried
in the big picture. The real problem is basically that we
never gave Havoc a final gohead to commit various parts
of this patch as he finished them.  
 
> for now, lets try to make the best out of what we have here, i'll
> start out with direct comments on the patch first:
> 
> - inside the pixbuf inlining code, you use plain malloc/free and not the
> glib equivalents.

This is consistent with the rest of gdk-pixbuf, and until
we have g_malloc_check() or whathever you wanted to call it,
necessary.

> - use gchar instead of plain "char" throughout your code (and similar for
> the other C types), i think i outlined that often enough now. i don't
> want to go over every single line you committed, just to fix the g*
> prefixing, i've got loads of other stuff to do.
> 
> - don't use g_type_create_instance() but g_object_new() instead.

I'm not in a position to disagree, but, some day, you are going to
have to explain this one ;-)
 
> - 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.)
 
> - 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]. 
 
[...]

> - 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+. 
 
> - don't provide gtk_window_get_accel_group(), rename that to
> gtk_window_get_default_accel_group() instead.
> 
> - 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.) It definitely should stay
a GtkWindow as a parameter to gtk_window_set_transient_for()

> - for GTK_WIN_POS_CENTER_ON_PARENT, it might be a good idea to double
> check for the screen extends (i.e. gdk_screen_width() and
> gdk_screen_height()) and the window size vs. its parent size.
> a better name would be nice here as well ;)

What's wrong with the name?
 
> - 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.

The only place I can think of using braces to group a list
is:

 bg[NORMAL] = { 1.0, 0.5, 0.3 }

But that isn't really parallel. Using the parens also makes
things easier to read by avoiding multiple levels of nested
braces.


You could also change the syntax from:

> style "myicons"
> {
>   stock["Gtk_Warning_Dialog"] = { ("stock-icons/dialog_error.png", 
>         *, 
>         *, 
>         *) }
> }

to:

 style "myicons"
 {
   stock["Gtk_Warning_Dialog"] 
     {
       icon "stock-icons/dialog_error.png" {}
       icon "stock-icons/dialog_error_button.png" { 
         size = BUTTON
         direction = LTR
     }
 }

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.

> 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.
 
> 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.)

>  rowstride (owen made a
>   point for this, but i'm not sure we need that for simple transfers),

rowstride is needed if you want to do no-copy. Unaligned lines
slow things down quite a bit. 

> - 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 

>  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?  (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...)

>  what was wrong with
>   the last version i mailed you? (...digging it up):
> > your version of gtk_dialog_run() can greatly be simplified,
> > based on what i mailed earlier in this thread:
> > 
> > gint
> > gtk_dialog_run (GtkDialog *dialog)
> > {
> >   GtkWidget *widget;
> >   gboolean was_modal;
> >   gint last_action_id;
> > 
> >   g_return_val_if_fail (GTK_IS_DIALOG (dialog), 0);
> > 
> >   widget = GTK_WIDGET (dialog);
> > 
> >   gtk_widget_ref (widget);
> > 
> >   was_modal = GTK_WINDOW (dialog)->modal;
> >   gtk_window_set_modal (GTK_WINDOW (dialog), TRUE);
> > 
> >   dialog->last_action_id = 0;
> >   gtk_widget_show (widget);
> > 
> >   do
> >     {
> >       GDK_THREADS_LEAVE ();
> >       g_main_iteration (TRUE);
> >       GDK_THREADS_ENTER ();
> >     }
> >   while (GTK_WIDGET_VISIBLE (dialog));
> > 
> >   last_action_id = dialog->last_action_id;
> >   gtk_window_set_modal (GTK_WINDOW (dialog), was_modal);
> > 
> >   gtk_widget_unref (widget);
> > 
> >   return last_action_id;
> > }
>   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
out nicer to use GMainLoop - which is just a gboolean
really (except for the thread issues mentioned above) 
than to do it yourself. 
 
> - 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... 
 
> - 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? 

> to get the majority of your code synced, i suggest processing things in
> the following order (assuming owen hasn't further comments that hold
> things off):

My comments are all small scale, immediate fix type stuff.

> - you write up detailed ChangeLog entries on all your stuff
> - you commit your text widget changes
> - you commit your progress (bar) changes
> - you reread the relevant portions on the pixbuf inlining thread (and btw,
>   tell me where to plug in that RLE code), do the appropriate fixes, commit
>   that and email gtk-devel about what we have then. (we can still change
>   format etc. after that then)
> - you do the stock/icon set changes, spawn a discussion on the rc
>   file format, and then post that seperatedly for review
> - you commit your GtkImage changes
> - we discuss the remainings of GtkDialog

This is an OK plan, as long as it doesn't drag out too long.
Though, IMO, what Havoc has is definitely good enough to commit
and work from there on fine tuning. And I think doing it that
way would save quite a bit of effort. 

Regards,
                                        Owen