Created attachment 136283 [details]
Screenshot-2.png

I guess the best fix would be to make wnck_window_get_name() return the filtered title, and add some wnck_window_get_raw_name() in case people need the unfiltered title.

Created attachment 137514 [details] [review]
Filter newline and tab from title

Vincent, is it something like this you had in mind?
(Note that my c skills are not the best, but it seems to work)

Created attachment 137515 [details]
Screenshot of before and after patch

Here's a screenshot of a window having tab and linefeed in title, before and after applying my patch.

That won't work with wide-character (eg. anything that's not ASCII). Use g_strdelimit() instead.

Thanks, I'll update the patch.

Created attachment 138663 [details] [review]
Updated patch

Bastien Nocera, is it something like this you meant?
The patch is now using g_strdelimiter and adheres to coding standard.

+const char*

You're not returning a const here.

+_wnck_sanitize_window_name (const char* name)
+{
+  gchar* gname;
+  gname = g_strdup_printf ("%s", name);

There's g_strdup for that...

+  gname = g_strdelimit(gname, "\n", ' ');
+  gname = g_strdelimit(gname, "\r", ' ');
+  gname = g_strdelimit(gname, "\t", ' ');

Did you read the docs for g_strdelimit?

That should do:
gname = g_strdelimit(gname, "\n\r\t", ' ');

+  return gname;
+}

That look odd:
-    return window->priv->icon_name;
+    return _wnck_sanitize_window_name (window->priv->icon_name);
   else if (window->priv->name)
-    return window->priv->name;
+    return wnck_window_get_name (window);

Bastien, thanks for your comments!

> Did you read the docs for g_strdelimit?
No, sorry. Will do better in the future.

> That look odd:
> -    return window->priv->icon_name;
> +    return _wnck_sanitize_window_name (window->priv->icon_name);
>    else if (window->priv->name)
> -    return window->priv->name;
> +    return wnck_window_get_name (window);

Well, my thought was to use the icon name if available otherwise use the standard (sanitized) function to get the name. Is that wrong? Should I use  _wnck_sanitize_window_name (window->priv->name) directly instead?

Created attachment 138669 [details] [review]
Updated patch

Comment on attachment 138669 [details] [review]
Updated patch

>diff -u libwnck-2.26.0.orig/libwnck/window.c libwnck-2.26.0/libwnck/window.c
>--- libwnck-2.26.0.orig/libwnck/window.c	2009-03-16 23:26:49.000000000 +0100
>+++ libwnck-2.26.0/libwnck/window.c	2009-07-18 16:07:03.000000000 +0200
>@@ -603,11 +603,22 @@
>   return window->priv->name != NULL;
> }
> 
>+const char*

You're *still* not return a const here. If you allocated memory, it's not const.

>+_wnck_sanitize_window_name (const char* name)
>+{
>+  gchar* gname;
>+  const gchar* replaced;
>+
>+  gname = g_strdup (name);
>+  replaced = g_strdelimit(gname, "\n\r\t", ' ');

g_strdelimit can be nest so:
gname = g_strdup (name);
return g_strdelimit(gname, "\n\r\t", ' ');
<snip>
>   if (window->priv->icon_name)
>-    return window->priv->icon_name;
>+    return _wnck_sanitize_window_name (window->priv->icon_name);
>   else if (window->priv->name)
>-    return window->priv->name;
>+    return wnck_window_get_name (window);

That's still bizarre. Can you add some comments as to why you're sanitising the icon name, but not the window name?

> That's still bizarre. Can you add some comments as to why you're sanitising the
> icon name, but not the window name?

Both are sanitized, wnck_window_get_name () is sanitized - or have I missed something?

I'll read up on const and stuff and get back with a patch later (yes, I'm new to C). 

Hmm. Not really sure what I did wrong because "replaced" was marked as const. Please let me know if you got time to explain.

Anyway I've removed const from the function, but should it be marked static instead or could _wnck_sanitize_window_name be used somewhere else in libwnck?

Do we want a wnck_window_get_icon_raw_name function as well?

Created attachment 138684 [details] [review]
Updated patch (removed const)

Thanks for the patch. A few comments:

===

+char*
+_wnck_sanitize_window_name (const char* name)

It should be "static char*", unless you move it to util.c and name it _wnck_util_sanitize_name(). (The right thing to do since it will be needed elsewhere, see below). In this case, the declaration should go in private.h.

===

wnck_window_get_name() returns a "const char*". However, with your code, it's returning a string that is strdupped, so we'll be leaking memory. That's bad. The easiest solution would probably be to add a new field raw_name in the _WnckWindowPrivate structure. To save some memory, something special should be done when name and raw_name are the same string. You'd probably implement get_name() like this:

if (name)
  return name;
if (raw_name)
  return raw_name;
return FALLBACK_NAME;

===

I would think that wnck_window_get_raw_name() should never return FALLBACK_NAME: if you're interested in the raw name, then you really want it, even if it's NULL. (This would require an update to the API doc)

===

The API documentation for wnck_window_get_raw_name() and wnck_window_get_name() should reference the other function, and the one for get_raw_name() should explain what is a raw name.

===

We probably want to have a wnck_window_get_raw_icon_name() function too.

===

It might be worth looking if this is necessary for WnckApplication, WnckClassGroup and even WnckWorkspace. (It probably is.)

Vincent, thanks for the great feedback! I will look in to this as soon as possible!

Created attachment 139891 [details] [review]
Filters name in win, app, classgroup and workspace

Sorry for the delay but here's an updated patch that filters the name from Wnck[Window, Application, ClassGroup, Workspace]. 

All of them behaves the same, save a copy of the filtered name in priv if it's different from raw and when requested return filtered name if exists otherwise raw.

I did have a problem testing all of it but Workspace and Window is working just fine.

Also I'm not that good with writing documentation so please comment on what to change and I'll fix it.

Note; I did not implement wnck_application_get_raw_icon_name () as it's identical to wnck_application_get_raw_name (). 

Also set the since attribute to 2.30 since we're past feature freeze (right?).

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/libwnck/issues/110.