GNOME Bugzilla – Bug 342084
Portability problem: **environ
Last modified: 2008-03-20 11:19:37 UTC
libnautilus-private/nautilus-program-choosing.c uses extern char **environ; to access the environment in my_gdk_spawn_make_environment_for_screen and make_spawn_environment_for_sn_context. However, OS X doesn't have that symbol. It remains undefined in the compiled binaries, which leads to linker or runtime errors when it is used. For a work-alike, we have *_NSGetEnviron() in crt_externs.h. For more discussion and solutions, search bugzilla for nsgetenviron: http://bugzilla.gnome.org/buglist.cgi?long_desc_type=substring&long_desc=nsgetenviron&cmdtype=doit
Created attachment 65637 [details] [review] darwin work-around for environ** symbol The source contains a comment that the code was taken from gdkspawn-x11.c, so here is the patch I use for gtk+2 on darwin. There are better patches in the other bugzilla items. Better to test for the .h and _NS symbol and use them if present than to use a platform test; even better to rewrite to use the g_*env funtions from glib2...
Nautilus bug -> Reassigning!
*** Bug 344197 has been marked as a duplicate of this bug. ***
Created attachment 96598 [details] [review] Proposed patch Proposed patch attached, which duplicates glib's code. There is no glib helper to obtain the environment variable, so we'll have to redefine it ourselves.
I should probably be reporting this separately, but I noticed while taking a look at your patch: I think the AC_CHECK_FUNCS(setenv unsetenv putenv) check can be removed from configure.in while at it, a quick grep suggests that they are not used anymore in nautilus (the corrisponding glib functions are used).
Can't you use g_listenv(), g_getenv(), g_setenv()? http://library.gnome.org/devel/glib/stable/glib-Miscellaneous-Utility-Functions.html#g-listenv
hpj: that's exactly what I meant. We *do* use them, so we can remove the old check from configure.in
I think this can be solved in a better way using GAppInfos and GDesktopAppInfos instead of setting our own envp and using gnome_desktop_item_launch_with_env ()...modifying fields.
Created attachment 107311 [details] [review] patch Attached patch replaces the use of gnome_* functions in nautilus-program-choosing.c using of GDesktopAppInfos, getting rid of |environ|.
I think thats a generally good approach. Comments: * You don't need slowly_and_stupidly_obtain_timestamp(), as the eel app launch context will do that by default * I think it makes more sense to change the args to nautilus_launch_desktop_file() to be a list of GFiles. That is what almost all uses have already, and we never use this API to pass in weird external uris that are problematic wrt roundtripping via GFile.
Created attachment 107478 [details] [review] patch v2 Attached patch reworks a bit the handling of DnD selection of this code path using GFiles instead of char uris from as early as we can, as for last comment from Alex.
I don't have time to do a full review atm, but it seems kinda weird that functions like icon_view_move_copy_items() frees passed in arguments. Thats totally against expected behaviour, passed in arguments are generally owned by the caller.
Created attachment 107579 [details] [review] patch v3 You're right...Updated patch according to your last comment.
Hmm, there is a duoble free of a file in fm_directory_view_handle_netscape_url_dr.. (both in list and manually freed). However, looking at this patch it seems i was wrong. We do in fact end up with this code when dropping a uri from an external source (like firefox or whatnot) onto a launcher, so converting it to gfile is wrong (since that would mangle e.g. a mailto link). So, i guess its back to fixing the first patch instead. However, it does seem like you found a leak of item_uris in paste_clipboard_data that needs fixing.
I commited that with some leak fixes and coding style cleanups.