GNOME Bugzilla – Bug 571496
Migrate from deprecated gnome_execute to g_spawn/xdg-terminal
Last modified: 2013-09-13 00:59:51 UTC
To get rid of libgnome and according to http://live.gnome.org/LibgnomeMustDie , gnome-exec shall be replaced by GSpawn which covers all of the useful parts of gnome-exec. gnome_execute_terminal* can be implemented using xdg-terminal (which will use "the active desktop's terminal program" rather than "the gnome-configured terminal program") See http://library.gnome.org/devel/libgnome/stable/libgnome-gnome-exec.html http://library.gnome.org/devel/glib/stable/glib-Spawning-Processes.html ./evolution/shell/e-shell-window-commands.c: pid = gnome_execute_async (NULL, extra_arg ? 2 : 1, args); ./evolution/calendar/gui/alarm-notify/alarm-queue.c: result = gnome_execute_shell (NULL, cmd); ./evolution/mail/mail-ops.c: gnome_execute_async_fds (NULL, argc, argv, TRUE);
I'd like to take a crack at this.
Created attachment 132106 [details] [review] Part 1 of 3 I couldn't figure out what extra arg was used for, but the only caller I found always passed NULL. So I removed it. Compile tested (can't test due to another bug).
Created attachment 132148 [details] [review] Part 2 of 3
Created attachment 132149 [details] [review] Part 3 of 3
Created attachment 132151 [details] [review] Remove unneeded includes This should be a nop change; evolution apparently used to use more libgnome than it does today. This patch just removes the includes that weren't removed when the functionally was. Sorry it took me a month to do the work after I said I would; I had to do it twice after removing the wrong evolution directory >.< All patches were written against evolution 2.26.0, from apt-get source evolution on Jaunty.
Awesome. Thanks. And no "Sorry" needed at all here, we're happy about any contributions and help to clean up Evolution. :)
Thanks Adam, I'll try to review the patches tomorrow.
Patches look good for the most part. Couple nitpicks in the first one: - The g_error_free() call should be within the "if (error != NULL)" block. The function doesn't take kindly to NULL arguments (I wish it did). - We use tab characters for indentation (I wish we didn't). Even though the existing code doesn't always honor that, your new code should. Marking the first patch as "needs-works" and the others as "reviewed" (though they look fine) since we'll want to commit them all together.
Created attachment 132233 [details] [review] Part 1 of 3, reformatted Replaced the spaces with tabs, and moved the free, as suggested.
Looks good. One last nitpick: if (error != NULL) { not if (error != NULL) { Otherwise, approved.
Created attachment 132234 [details] [review] Part 1 of 3, rereformatted Gah! But that's the /wrong/ way! New patch, adjusting the brace to match the rest of the nearby code.
*ping*
Dropping my grep results here too: ./shell/e-shell-window-commands.c:#include <libgnome/gnome-exec.h> ./calendar/gui/e-cal-list-view.c:#include <libgnome/gnome-exec.h> ./calendar/gui/e-week-view.c:#include <libgnome/gnome-exec.h> ./calendar/gui/alarm-notify/alarm-queue.c:#include <libgnome/gnome-exec.h> ./calendar/gui/e-day-view.c:#include <libgnome/gnome-exec.h> ./mail/mail-ops.c:#include <libgnome/gnome-exec.h>
Created attachment 134077 [details] [review] Part 4/3 - remove unneeded includes Patches 1-3 still apply against git, the last one didn't, so I rebased to HEAD. I can't test it because I haven't set up jhbuild (yet), but considering how it compiled against 2.26, I doubt there will be any problems with it now.
Approved. Do you have a GNOME account or should I commit on your behalf?
I don't have committer privlages. Or do I not need them when you mark them "commit now"? (Most of my F/LOSS work was on the Wine project where all patches went to one guy who committed everything).
Committed patches to master: http://git.gnome.org/cgit/evolution/commit/?id=8cf036faffe5985d34c310ee89b133200380b757
Thanks a lot for the patch!