Re: Re: [evolution-patches] Bounty Hunt patch: Set wallpaper from mailer

[Not Zed <notzed ximian com>]

On Wed, 2003-12-03 at 15:46, David Moore wrote:
> > Actually this really isn't a problem, it just returns a pointer (the actual name saved) vs bool.  This implicitly supplies as much info as the bool, plus useful information too, the saved filename.  The ononly difference is you have to save the return, and g_free it - not a big change.  The harder part is save_part.
>
> I'm not sure I understand what you're asking. Should the number-appending code be in save_part or in set_background?
You can leave it as is, i'm just saying that your justification, about return values, isn't really a valid one as to the complexity ...

> > don't add this whitespace stuff.  i preferred the "name(" function calls, like k&r taught me.  
>
> Okay. The coding style guidelines at developer.ximian.com/projects/evolution/patch_guidelines.html show it with the whitespace, though.
It also says ' (' is optional, and also:

Where there are slight stylistic differences, the style in the
surrounding code should be followed.

(and i prefer it without ' ' :)

> > you really can't do this, it assumes you have an extension, but you could have anything given that the value most likely came from the internet.-> crash if there's no '.' in the name.
>
> No, it works out. strrchr returns NULL if '.' is not found, and g_strdup returns NULL if passed NULL. So if there's no '.', the if(!extension) evaluates to true and extension is set to "". I could be missing something, but it works for me.
ahh ok, yeah you're right.  Still you need to g_strdup("") in the failure case if you do this, since you free it later.

Actually even better, you dont need to strdup it at all, just point to the string.  i.e.

extension = strrchr(name, '.');
if extension == null
  extension = "";

and just keep filename around.  see below anyway.

> > The above can be done in one loop, without having the encompassing if() {}
>
> Maybe I'm misunderstanding you, but removing the if() would result in extension and basename being calculated and then immediately freed when they're not needed, which is kind of a waste.

Yes and no, you can calculate it, but you dont need to allocate or free anything, you can even get by allocating less.

i.e.

+       filename = g_strdup(camel_mime_part_get_filename(t->data.part.part));
+       
+       /* if filename is blank, create a default filename based on MIME type */
+       if (!filename || !filename[0]) {
+               filename = g_strconcat (_("untitled_image."), camel_mime_part_get_content_type(t->data.part.part)->subtype, NULL);
+       }
+       
+       e_filename_make_safe (filename);

   fullpath = g_build_filename (g_get_home_dir(), ".gnome2", "wallpapers", filename, NULL);

-> we get the extension from our allocated block, and 'cut out' the filename and the extension in one go

extension = strrchr(filename, '.');
if (extension)
  *extension++ = 0;

while (g_file_test(fullpath, G_FILE_TEST_EXISTS)) {
    char *name

    name = g_strdup_printf(extension?"%s (%d).%s":"%s (%d)", filename, i++, extension);
    g_free(fullpath);
    fullpath = g_build_filename(g_get_home_dir(), ".gnome2", "wallpapers", name, NULL);
    g_free(name);
}

g_free(filename);


-> we might calculate extension unecessarily, but its cheap.
-> i think the code above is simpler and easier to understand as well as more compact


Jeff misunderstood me about the extension type, if its bogus, then so be it, its not important.