In the if(details) block you allocate a new string with g_strndup():

   applet = g_strndup (applet, strlen (applet) - strlen (detail));

but you never free it. Except that the patch is great.

Created attachment 256694 [details] [review]
create better object/applet names

Created new patch. Tested on default applets + gnome-applets.

Can you give an example for a string that needs the while loop?

Also now you allocate new strings with g_strdup in a while loop but only free it once outside the loop, I think that can potentially lead to not freed strings.

Ok I understand it now. I would suggest to add the following comment to this function:

+/**
+ * This function takes the applet name from the iid which looks like this: 
+ * 'ClockAppletFactory::ClockApplet' and converts it into a lower case,
+ * hyphen-delimited string which looks like this 'clock-applet'. To
+ * maintain readability of camel-case names a hyphen is inserted before
+ * each upper case character.
+ */
static char *
panel_layout_object_generate_id (const char *iid)

Feel free to do it. You can update and post new patch here. I have no time to do it. :(

iid can be:
1) ClockAppletFactory::ClockApplet
2) ClockAppletFactory::ClockApplet:something

I am ignoring first part, we don't need it. Next step is to add dashes before next word, not before each upper case character.

There is CPUFreqApplet. We don't want name c-p-u-freq-applet, but cpu-freq-applet. 

There is extra check for iid like this ClockAppletFactory::ClockApplet:Something. we don't want add dash before S. but maybe it is not needed. At least in default applets there is no such iid.

Created attachment 256716 [details] [review]
Add comment and fix memory issue

I have added the comment and also added another free in the while loop to free the newly allocated strings.

Review of attachment 256716 [details] [review]:

Overall I like it.

As this will aid debugging (at least for newly added applets), I'd even consider inclusion in 3.8.

::: gnome-panel/panel-layout.c
@@ -351,3 @@
                                   const char                *schema,
                                   const char                *path_prefix,
-                                  const char                *default_prefix,

better to split out the unused vars into another patch ?

@@ +761,3 @@
+    char        old;
+
+    applet = g_strrstr (iid, "::");

If the iid doesn't actually contain a "::", this will crash or create a security bug. Never know what context this function might be called in.

@@ +768,3 @@
+            dashed = g_strdup_printf("%c", applet[0]);
+        } else {
+            char* tmp = dashed;

The loop looks like tmp is never actually used (although it is saved for freeing). Maybe better to name it "dashed_old" or similar and reference it in the g_strdup_printf-s below.

Alternatively, introduce dashed_new and do the assignment at the end, after freeing.

@@ +781,3 @@
+    }
+
+    detail = g_strrstr (dashed, ":");

dito.

@@ +791,3 @@
+    } else {
+        return_id = g_ascii_strdown (dashed, -1);
+    }

I somehow feel the whole thing could be implemented prettier (and maybe simpler) with a scatter-gather approach. Then again, you'd need to manage the array memory.

Alternatively, we could just not bother with inserting -'s before in-word upper case characters, and simplify this to a character replacement / reduction loop without a lot of reallocations.

::: gnome-panel/panel-schemas.h
@@ -33,2 @@
 #define PANEL_LAYOUT_OBJECT_PATH             "/org/gnome/gnome-panel/layout/objects/"
-#define PANEL_LAYOUT_OBJECT_DEFAULT_PREFIX   "object"

undefined

Created attachment 260054 [details] [review]
Create better object/applet names

(In reply to comment #7)
> better to split out the unused vars into another patch ?

This I already committed to master so this is removed from new patch.

> @@ +761,3 @@
> +    char        old;
> +
> +    applet = g_strrstr (iid, "::");
> 
> If the iid doesn't actually contain a "::", this will crash or create a
> security bug. Never know what context this function might be called in.

Added extra check to make sure we are using correct iid as parameter. Also rewrote this function to be simpler keeping previous functionality. 

Is it good now?