Created attachment 33050 [details] [review]
Feature Patch

Comment on attachment 33050 [details] [review]
Feature Patch

Some suggestions:
 - a separate useful change 
   would be that latest gtk 
   has ellipsization, so we 
   could clean out the Eel 
   stuff
 - braces should be on a line by themselves, indentation should be 2 spaces (in
emacs, set style to GNU)
 - I prefer "char" to "gchar" though I don't remember what most libwnck code
uses
 - it's nicer to avoid 
   assignment in the same 
   line as variable declaration
 - casting the const off of wnck_window_get_name isn't ideal, suggest instead
freeme = g_strdup() then window_name = freeme
 - it may be OK already, I'm too lazy to open the full source file, but should
be sure destroying a window while the menu is up won't hose us somehow

Thanks! Certainly seems like a good patch in all but nitpick aspects.

Created attachment 33102 [details] [review]
New Feature Patch

This patch should fix what you have mentioned, with the exception of
integrating Gtk's new ellipsizing features. The only thing I should is that I
changed my Emacs style to GNU, but I just followed the indentation and bracket
styles used throughout the rest of this file, as exemplified by lines 197-200.

I hope this works.

Created attachment 33154 [details] [review]
Updated Patch

This patch has better division of labor than the prior ones and makes use of
Gtk's new ellipsizing label.

Comment on attachment 33154 [details] [review]
Updated Patch

Thanks for the patch!
I read the patch quickly, but it looks good.

>+/* The returned value will need to be freed. */
>+static const char *
>+window_menu_get_window_name (WnckWindow *window)
>+{
>+	const char *return_value;
>+	const char *name;
>+
>+       	name = wnck_window_get_name (window);
>+	if (!name)
>+		name = g_strdup(_("Unknown Window"));
>+	else
>+		name = g_strdup (name);
>+	
>+	if (wnck_window_is_shaded (window))
>+		return_value = g_strdup_printf ("=%s=", name);
>+	else if (wnck_window_is_minimized (window))
>+		return_value = g_strdup_printf ("[%s]", name);
>+	else
>+		return_value = name;
>+	return return_value;
>+}

It seems you're leaking name when the window is minimized or shaded.

>+	gtk_label_set_ellipsize (GTK_LABEL (ellipsizing_label), PANGO_ELLIPSIZE_MIDDLE);

Why PANGO_ELLIPSIZE_MIDDLE? Does it look better than PANGO_ELLIPSIZE_END?

Created attachment 33172 [details] [review]
Updated Patch

This patch should fix all of the aforementioned problems. I set it to use
PANGO_ELLIPSIZE_END, but I personally think that PANGO_ELLIPSIZE_MIDDLE would
be better. This is open to discussion, though I will be fine with whatever is
chosen.

The reason that I say that I am fine with whatever is chosen is that I am
working on a large patch set to contribute to this applet that provides a
configuration dialog; so I thought that users could be given a choice, once I
submit that larger patch set
(http://www.nerp.net/~khanreaper/window-menu-patch/).

Comment on attachment 33172 [details] [review]
Updated Patch

>@@ -54,6 +53,7 @@ typedef struct {
> 	GdkPixbuf    *icon_pixbuf;
> 	WnckWindow   *icon_window;
> 	GHashTable   *window_hash;
>+	GHashTable   *label_hash;

It'd be nice to use one hash instead of these two. Not sure if it's easily
doable,
though

>+/* The results of this function will need to be freed. */
>+static const char *

You can reomve the const from here and so we'll have less casts later :-)

>+window_menu_get_window_name (WnckWindow *window)
>+{
>+	const char *return_value;
>+	const char *name;
>+
>+	name = wnck_window_get_name (window);
>+	if (!name)
>+		name = g_strdup (_("Unknown Window"));
>+	else
>+		name = g_strdup (name);
>+
>+	if (wnck_window_is_shaded (window)) {
>+		return_value = g_strdup_printf ("=%s=", name);
>+		g_free ((char *)name);
>+	} else if (wnck_window_is_minimized (window)) {
>+		return_value = g_strdup_printf ("[%s]", name);
>+		g_free ((char *)name);
>+	} else
>+		return_value = name;
>+
>+	return return_value;
>+}
>+

Ok, I suggest something like:

	const char *const_name;
	char	   *name;
	char	   *return_value;

	const_name = wnck_window_get_name (window);
	if (!name)
		name = g_strdup (_("Unknown Window"));
	else
		name = g_strdup (const_name);

	if (wnck_window_is_shaded (window)) {
		return_value = g_strdup_printf ("=%s=", name);
		g_free (name);
	} else if (wnck_window_is_minimized (window)) {
		return_value = g_strdup_printf ("[%s]", name);
		g_free (name);
	} else
		return_value = name;

	return return_value;

Easier to understand this way (well, at least for me)

>+	if (make_bold == TRUE)

"if (make_bold)" will be enough :-)

Do you have CVS access?

See also bug 155868; I'm not sure whether the ellipsization belongs in libwnck
or wnck-applet, but it appears there's competing requests...

Created attachment 33224 [details] [review]
2004-10-29

Vincent, this patch should address what you have mentioned. I appreciated your
advice and employed a new structure, getting rid of that extra hash-table.

With regard to your question about CVS access, I do not have write access to
the tree.

fwiw I don't think making ellipsization style configurable is a good idea. ;-)
But I guess that can be discussed on a separate bug about separate patches.

Matt: I committed your patch to HEAD. Oh, and I agree with Havoc, but we can
discuss about this in another bug

*** Bug 157895 has been marked as a duplicate of this bug. ***

*** Bug 158050 has been marked as a duplicate of this bug. ***

*** Bug 158597 has been marked as a duplicate of this bug. ***

*** Bug 159591 has been marked as a duplicate of this bug. ***