Re: gtk-hp-patches

[Tim Janik <timj gtk org>]

On Mon, 17 Jul 2000, Tim Janik wrote:

> On 17 Jul 2000, Havoc Pennington wrote:
> 
> > 
> > Havoc Pennington <hp@redhat.com> writes: 
> > > It would be good to merge to HEAD soon. The patch fixes a bunch of
> > > text widget stuff, among other things. Also George is sort of waiting
> > > on it to do some gnome-libs work.
> > > 
> > 
> > Owen says I should revise this to "we will merge the branch tomorrow,
> > if there are no objections."
> 
> i'd like to ask for a few more days off. i haven't had a chance yet to
> comment detailedly on either your patch or on owen's main loop stuff
> or his theme-programming write up (which is a badly needed thing, and
> i really aprechiate that he finally wrote something up now).
> 
> unfortunately i'll be busy with my new apartement the next few days,
> so i'll only be able to get around to detailed reviews later this week.

ok, early monday night younts as "later this week", so here are my comments ;)


hi havoc,

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.


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.

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

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

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

- regarding your stockid naming:
#define GTK_STOCK_DIALOG_INFO      "Gtk_Info_Dialog"
let's stick to either of the "GtkInfoDialog" or "gtk-info-dialog" (use
underscores for the latter if you prefer) namingschemes which are well
established throughout the code base already.

- also, keep the namespace prefixing for the stock ids:
+#define GTK_STOCK_DIALOG_INFO      "Gtk_Info_Dialog"
+#define GTK_STOCK_DIALOG_WARNING   "Gtk_Warning_Dialog"
+#define GTK_STOCK_BUTTON_APPLY     "Gtk_Apply_Button"
+#define GTK_STOCK_BUTTON_OK        "Gtk_OK_Button"
should be:
+#define GTK_STOCK_DIALOG_INFO      "gtk-dialog-info"
+#define GTK_STOCK_DIALOG_WARNING   "gtk-dialog-warning"
+#define GTK_STOCK_BUTTON_APPLY     "gtk-button-apply"
+#define GTK_STOCK_BUTTON_OK        "gtk-button-ok"

- in gtk_style_render_icon() you g_return_val_if_fail() if the rendered
pixbuf is != NULL, i suspect you need a
g_return_val_if_fail (source != NULL) right at the beginning to make
sure the rendering can succeed.

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

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

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

- 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 {}.


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.

things that i spotted, that are still unfinished:

- despite the email discussion you spawned about inlined pixbufs,
  your code still contains that random magic, rowstride (owen made a
  point for this, but i'm not sure we need that for simple transfers),
  has_alpha+colorspace+n_channels and bits_per_sample. did you ever
  read my email about GdkPixdataType?

- we also need a file describing the resulting inline pixbuf stream format,
  much like you had it in one of your original emails.

- on gtk_dialog_run(), you still use signal connections/disconnections
  instead of overriding GtkDialog class methods here. also, you still go
  the complicated route of setting up a new main loop. 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.

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

- 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'm sure there's more stuff, but i need to take a break here ;)

after pointing out quite a bit of things that still need work,
i also don't want to leave the following unmentioned ;)
you patch contains lots of very valuable and good stuff, a stock
repo, inlined images and GtkDialog/ProgressBar overhauls are badly
needed and i'm glad you took the time to work on this. however,
there's still room for improvement, both coding and process
wise, so plaese take this email as encouragement for improvement
on both scales ;)


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

---
ciaoTJ