looking at this again, a more neutral labelling for the tab is needed.

new proposal: "Usability by"

I think a small infrastructure is needed to add persons which don't fit
into any of the given categories. Adding yet another random catrgory
doesn't fix the actual problem.

I'll hack up a patch for this.

patch never came ?

I simply forgot, will try to get this done after vacation, but clearly
before 3.0 freeze.

With the About dialog redesign there is no tabs anymore. Should this be vlosed as OBSOLETE?

Of course not; now it should mean to need to add another section to the credits display.

(In reply to comment #4)
> I simply forgot, will try to get this done after vacation, but clearly
> before 3.0 freeze.

Hey Mitch, do you plan to work on this for 3.2?

Maybe this is a bit big for a gnome-love task, but who knows.

Task is to implement an api like

void
gtk_about_dialog_add_credits (GtkAboutDialog  *dialog,
                              const gchar     *section_heading,
                              const gchar    **people);

I'm currently looking into this and have a few questions:
- After adding that method, what about get_documenters, set_documenters, etc.? removing them is no option I guess, at least not for a 3.x release.
- Are set_documenters, etc. supposed to work WITH add_credits (-> the Credits page shows people added with both add_credits and set_documenters, etc.)?
- Is add_credits supposed to be called only once? (That would be easier to implement, but the name kinda suggests that it will be called more than once)

Leave existing calls (get_documenters/set_documenters) alone. We can't remove them, and they do no harm.
Yes, add_credits is supposed to be callable multiple times, to add multiple sections, and it should not interfere with set_documenters, etc

Created attachment 205794 [details] [review]
First patch

(Aww, where did my message go?)
So, that patch above should work, but I'm not sure if the g_list_free_full is enough in gtk_about_dialog_finalize. I'm also not sure about the struct I introduced, is there something missing/required when doing that?
add_credits could also replace all the set_documenters, etc. returning the documenters would be harder.

Review of attachment 205794 [details] [review]:

Apart from the minor things mentioned below, and a missing doc comment for the new api, the patch looks about right.

::: gtk/gtkaboutdialog.c
@@ +809,3 @@
   g_strfreev (priv->artists);
 
+  g_list_free_full (priv->credit_sections, NULL);

You need a destroy notify here that frees the CreditSections

::: gtk/gtkaboutdialog.h
@@ +44,3 @@
 typedef struct _GtkAboutDialogClass   GtkAboutDialogClass;
 typedef struct _GtkAboutDialogPrivate GtkAboutDialogPrivate;
+typedef struct _CreditSection         CreditSection;

That typedef should go into the .c file, since the type is not otherwise used in the api at all.

@@ +161,3 @@
 void                   gtk_about_dialog_set_logo_icon_name     (GtkAboutDialog  *about,
                                                                 const gchar     *icon_name);
+void                  gtk_about_dialog_add_credits             (GtkAboutDialog  *about,

gtk_about_dialog_add_credit_section might be a better name, after all

Created attachment 205963 [details] [review]
Second version

Comment on attachment 205963 [details] [review]
Second version

Tried to implement all the suggestions

Review of attachment 205963 [details] [review]:

thanks for the new patch.

looks about right, except for a couple of leaks still, and a couple of style issues.

once the patch is corrected we can definitely land it. thanks again!

::: gtk/gtkaboutdialog.c
@@ +667,3 @@
 
+  priv->credit_sections = NULL;
+

I know it's been done right above, but there is no need to NULL-ify stuff here: the private data structure is zero filled by default, like the instance structure.

@@ +793,3 @@
+void destroy_credit_section (gpointer data)
+{
+	CreditSection *cs = (CreditSection *)data;

no need to cast: gpointer is void*

@@ +795,3 @@
+	CreditSection *cs = (CreditSection *)data;
+  g_free (cs->heading);
+  g_free (cs->people);

cs->people is a gchar**: g_free() is not enough, since you're copying it using g_strdupv(). you need g_strfreev() instead.

also, you're missing a call to "g_slice_free(CreditSection, data)" here.

@@ +2422,3 @@
+      GList *cs = NULL;
+      CreditSection *section = NULL;
+      for (cs = g_list_first (priv->credit_sections); cs; cs = g_list_next (cs))

you can use:

  GList *cs;

  for (cs = priv->credit_sections; cs != NULL; cs = cs->next)
    {
      CreditSection *section = cs->data;
      ...
    }

i.e. no need to use g_list_first() and g_list_last(); no need to cast to CreditSection*; and no need to declare (and initialize to NULL) the variables.

I also wonder if you need a GList: given that you just iterate it in one direction, a GSList would be enough.

@@ +2689,3 @@
+  new_entry->heading = g_strdup ((gchar *)section_name);
+  new_entry->people = g_strdupv ((gchar **)people);
+gtk_about_dialog_add_credit_section (GtkAboutDialog *about,

too much whitespace here.

@@ +2694,3 @@
+
+  update_credits_button_visibility (about);
+{

and too much whitespace here as well.

Created attachment 205997 [details] [review]
All suggestions + GSList

That's it, I hope :)

Review of attachment 205997 [details] [review]:

looks good to me now.

::: gtk/gtkaboutdialog.c
@@ +2683,3 @@
+  priv = about->priv;
+
+*/

last minor thing, sorry for not noticing it before: gtk does not allow C99, so no declarations after statements are allowed.

Created attachment 205998 [details] [review]
Moved the declaration of new_entry to the top

I hope this was the change you wanted to see(seems totally clear now that you mention it)

Review of attachment 205998 [details] [review]:

yes. looks good to me.

Review of attachment 205998 [details] [review]:

Yes, looks fine. 

Two final things that need to be done are to add the new function to gtk/gtk.symbols and docs/reference/gtk/gtk-sections.txt at a suitable place.

Created attachment 206008 [details] [review]
Added the method to gtk3-sections.txt and gtk.symbols

Complete patch. gtk_about_dialog_add_credit_section is at the top of the gtk.symbols(because it looks like everything is ordered alphabetially) and just above gtk_show_about_dialog in the gtk3-sections.txt

Yes, look good, but IMP gtk_about_dialog_add_credit_section() should
return_if_fail on section_name and people != NULL. It makes zero sense
to add empty sections.

I have some more stylistic comments, but we can play the review game
forever. Perhaps I should simply go over it once it's in git ;)

The following fix has been pushed:
d00368c GtkAboutDialog: Make credits section extensible

Created attachment 206140 [details] [review]
GtkAboutDialog: Make credits section extensible

This commit adds API that allows to add new named sections
to the Credits part of GtkAboutDialog, in addition to the
hardcoded sections for authors, documenters, artists and
translators.

Thanks for the patch, committed!