On Fri, 22 Mar 2013 at 23:23:37 +0100, [email protected] wrote:
> From: Daniel Déchelotte <[email protected]>
>
> ---
> WPrefs.app/Configurations.c | 41 +-------------
> WPrefs.app/WPrefs.c | 55 +++++++++++++++++++
> WPrefs.app/WPrefs.h | 7 +++
> WPrefs.app/Workspace.c | 126
> +++++++++++++------------------------------
> 4 files changed, 100 insertions(+), 129 deletions(-)
All patches that remove more lines are nice.
> diff --git a/WPrefs.app/Configurations.c b/WPrefs.app/Configurations.c
> index e64d8c1..55b1537 100644
> --- a/WPrefs.app/Configurations.c
> +++ b/WPrefs.app/Configurations.c
> @@ -111,43 +111,6 @@ static void updateLabel(WMWidget *self, void *data)
> WMSetLabelText(panel->dithL, buffer);
> }
>
> -static void
> -createImages(WMScreen *scr, RContext *rc, RImage *xis, char *file,
> - WMPixmap **icon1, WMPixmap **icon2)
> -{
> - RImage *icon;
> - char *path;
> - RColor gray = { 0xae, 0xaa, 0xae, 0 };
> -
> - *icon1 = NULL;
> - *icon2 = NULL;
> -
> - path = LocateImage(file);
> - if (!path)
> - return;
> -
> --- a/WPrefs.app/WPrefs.c
> +++ b/WPrefs.app/WPrefs.c
> @@ -392,6 +392,61 @@ char *LocateImage(char *name)
> return path;
> }
>
> +void CreateImages(WMScreen *scr, RContext *rc, RImage *xis, char *file,
> + WMPixmap **icon1, WMPixmap **icon2)
> +{
> + RImage *icon;
> + char *path;
> + RColor gray = { 0xae, 0xaa, 0xae, 0 };
> +
> + path = LocateImage(file);
> + if (!path)
> + {
> + *icon1 = NULL;
> + if (icon2)
> + *icon2 = NULL;
> + return;
> + }
I think the original version of this code was much cleaner. Just set
icon1 and icon2 to NULL unconditionally in the beginning and that's it.
Using
+ if (icon2)
+ *icon2 = NULL;
is just redundant, no?
> +
> + *icon1 = WMCreatePixmapFromFile(scr, path);
> + if (!*icon1)
> + {
> + wwarning(_("could not load icon %s"), path);
> + if (icon2)
> + *icon2 = NULL;
same here
> + wfree(path);
> + return;
> + }
> +
> + if (!icon2) // Not an error, btw
> + {
> + wfree(path);
> + return;
> + }
Oh, but wait. You now use icon2 as also a parameter to decide whether
to combine it with 'xis' below. At least having the comment there
makes one realize something odd is happening.
I think it's better to be explicit about this, because the code becomes
more readable and less tricky and you avoid setting *icon2 to NULL at
various places and do it only once in the beginning.
So I think this function should have another parameter, say an integer
(or even an boolen) called 'combine'. Then at this point the code should
read
if (!combine) {
wfree(path);
return;
}
and the intention is obvious and you don't need to add the
comment "Not an error, btw".
--
To unsubscribe, send mail to [email protected].
