[compiz] Annotate, guiding line patches with questions

[Gerd Kohlberger]

>> Another approach could be to add an option to the screenshot plugin
>> which launches an external app. This could launch your helper app or
>> something else. I'm using this to upload screenshots to my flickr account.
>>
>> Here is a patch for screenshot.c which adds a launch_app option.
>> What do you think?
> 
> Seems like a simple and useful solution. Some comments to the patch:
> 
> allocating storage for the command line using:
> 
> + comm = malloc (sizeof(app) + sizeof(dir) + sizeof(name) + 2);
> 
> is not going to work well as "app" and "dir" are just pointers and the
> size of the memory that they are pointing to is not known at compile
> time. You also need to make sure that you allocate enough memory to
> store the terminating `\0' character.

haha, yes of course.

> I would change that line to:
> 
> + comm = malloc (strlen (app) + strlen (dir) + strlen (name) + 3);
> 
> You should use:
> 
> runCommand (s, comm);
> 
> instead of calling execl directly. runCommand does things like exporting
> the DISPLAY environment variable and setting the session id.
> 
> I don't like to have "eog" as the default. I'd rather like to see it be
> set to nothing by default.

I thought eog is a good default, as it should be present on all distros,
but that's not true if you don't you use gnome.

btw, the patch wouldn't have worked anyway, cause I forgot to change SetDisplayOption.
I should have tested it before submitting. Sorry.

Here is another one.
Thanks, Gerd.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: screenshot-launch_app.diff
Type: text/x-patch
Size: 2247 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/compiz/attachments/20070111/289f642f/screenshot-launch_app.bin