Created attachment 80993 [details] [review]
the patch

This touches various files and does the following:

* switch the two tests from gnome to gtk_init and manual invocation of goption parsing. Additionally make the inhibit test exit when closing the window.
* add a gpm_help_display() function to gpm-common.c and use it in preference to
gnome_help_display variants. Call preferences and statistics with the specific help subsection.
* replace gnome_show_url with invoking xdg-open or gnome-open
* except the daemon switch to gtk_init + manual option handling
* config and makefile changes to support a new GTK_LIBS flag and to add gpm-common to statistics build.

the applets were not touched they are GNOME applets anyway and have additional deps.

I plan to use the new libeggsm code as well, making the non-applet bits of g-p-m
entirely independent of libgnome unless someone shouts :)

the preferences and statistics apps are linked against ~25 .so-s less on an ubuntu feisty system. Measured with exmap the resident memory is a few hundred Kb less but not consistent across measurements. Launch time for both is noticably less.

comments welcome

I like the patch. Thanks for looking into this.

General comments:

* I always like to use braces, even for single line conditionals:

if (moo == TRUE) {
    foo();
}

* I never use the shorthand true false ? shorthand, e.g.

link_id ? "?" : "" makes little sense to me. Can you make this a proper if statement please.

* is there any problems with GNOME session management if we don't use gnome_program_init()?

* I tried to apply the patch for testing but get:

hughsie@hughsie-laptop:~/gnome-power-manager$ patch --dry-run -p0 < /home/hughsie/Desktop/libgnome.diff 
patching file configure.in
patching file src/gpm-statistics.c
patching file src/gpm-inhibit-test.c
patching file src/gpm-tray-icon.c
patch: **** malformed patch at line 177:  /**

Cheers,

Richard.

Created attachment 80996 [details] [review]
corrected patch

thanks for the comments.
I added braces for the two if statements, dropped the ? conditional, added an extra g_free I missed last time and resisted cosmetising the diff so it should actually apply now :)

losing gnome_program_init() does not affect session management but means no bug buddy integration. Is that ok?

(In reply to comment #3)
> losing gnome_program_init() does not affect session management but means no bug
> buddy integration. Is that ok?

That is a big loss. Have you done some profiling on exactly how much memory we save and how the startup speed increases? I can post to desktop-development mailing list and get some comments.

Thanks.

Startup speed I did not profile I just noticed that while both times were sub-second, without the processing of extra ~25 dsos the diff is noticable.
As for memory usage, this is probably the same as the difference between a hello world app which uses gtk_init vs one that uses gnome_program_init and nothing more but stary in the main loop.

Here are the measurements done with exmap for effective resident memory:

           patched        svn
g-p-prefs: 3182K           3933K
g-p-stats: 4614K           4714K

though I don't know how to explain the 100K vs 750K difference when the same kind of dep removal was applied to both.

For various other apps I got 100K-1.1M diffs, it may be that exmap is not precise enough or I am not interpreting it right.

The desktop list would be a good idea as there are a few devs who blogged about similar measurements in the past and they may have something to add or help profiling g-p-m on their own to get data from multiple people. There are a few other apps in gnome svn which rely in the same way on libgnomeui so profiling changes against those would help estimate how much would a complete average desktop benefit if these changes added up.

Richard, if you're uncomfortable with replacing gnome_program_init with gtk_init because of bug buddy, would you mind applying the rest of the bits from the patch?
I'd also submit a patch which adds an --enable-gnome configure option which would only ifdef on g_p_i() and _gtk_init() so it would be small. This way it would make it a lot more likely g-p-m is accepted in Xfce and distros based on it. It may even make sense in stock Ubuntu where apport, the external crash handler is preferred over bug buddy.

thanks

A few comments:

+	g_option_context_add_group (context, gtk_get_option_group (FALSE));
+	g_option_context_parse (context, &argc, &argv, NULL);
[...]
+	gtk_init (&argc, &argv);

Why the gtk_init call? You already added gtk's option group to the context and parsed it. And you're ignoring errors from the g_option_context_parse call.


+		command = g_strconcat ("gnome-help ghelp://", uri, "?", link_id, NULL);
+	} else {
+		command = g_strconcat ("gnome-help ghelp://", uri,  NULL);

I think one should use gnome-open ghelp://..., not gnome-help directly.

Created attachment 87791 [details] [review]
a subset of the original patch

this just replaces the use of gnome_help_display and gnome_show_url
calls gnome-open not gnome-help as per Christian's suggestion
it does not require ifdefs since it works with or without having a GNOME platform (yelp, gnome-open) installed.
The only libgomeui dependency left is gnome_program_init, but that depends on the status of bug buddy being a GTK_MODULE or not in 2.20

See http://bugzilla.gnome.org/show_bug.cgi?id=436832#c2 about hardcoding BINDIR.

Created attachment 90439 [details] [review]
replace gnome_help

subset of original patch, factors out calling to help and makes it take the gscreen into account. This is similar to what has recently been commited in gcalctool

Created attachment 90440 [details] [review]
replace gnome_url_show

patch independent of previous one, replaces gnome_url_show by invoking gnome-open and if that fails xdg-open. Fixed the BINDIR problem that Sven pointed out, works with gscreens and pops up dialog on error.

Now that bug-buddy can be activated without gnome_program_init in 2.20 would you consider applying these? thanks.

Jani, how do we activate bug buddy? If you can add the activation into the patches above then sure, you can merge these for 2-19.

it happens just like adding support for a11y via the GTK_MODULES mechanism. gnome-session sets up the GTK_MODULES env var and adds bug-buddy to it if it's in the path

svn trunk:
./gnome-session/main.c for bug-buddy
./gnome-session/gsm-at-startup.c for a11y

so it's taken care at the GTK level.

Jani, please commit your latest patches. Many thanks.

I do not have commit access to GNOME svn :)

Ohh, no problem. Can you generate a patch for todays svn please, and include a ChangeLog entry so you get credit. Thanks.

Created attachment 93007 [details] [review]
original patch regenerated

Here's the original patch against current svn, with Christian's comment regarding gtk_init and g_option parsing taken into account.
It swithces two tests, g-p-m-preferences andg-p-m- statistics away from gnome_program_init(), and replaces gnome_show_url and gnome_help_display with
calls to gnome-open.

There are a few other pieces of GNOME APIs and autoconf nits left, but it is a lot easier doing and testing this piece by piece.

I don't know why but this patch makes "make check" fail in the self test code. For some reason the DPMS checks fail. Do you see this also?

make check fails here with or without the patch, but in the RECALL not the DPMS test.

> check #82     GpmCell:        make sure we got a single recall notice...FAILED [did not get recall (install fdi?)]
FAIL: gnome-power-self-test
===================
1 of 1 tests failed
===================

not sure if it reaches DPMS test

which is the DPMS test exaclty, I cannot find it in the source.

Created attachment 93046 [details] [review]
updated patch, fixes crash

add back gtk_init() or else the inhibit test crashes.

Committed, many thanks. Do you want to make some changes in configure.in also?

yes, I'll take a look at the last bits of libgnome (particluarly gnome client) and then the configure bits.  Can you describe briefly what the use for gnome client is in g-p-m? I wonder if it can be easily replaced and I'll look at the code but your comments would give me a headstart. There are some apps that used gnome client because it was copy-pasted but was mostly a noop (sets up an empty "die" callback, or is obsoleted by the fdo autostart mechanism), n-m is one that recently got rid of it. But g-p-m may have a valid reason.

thanks

Nope, I don't use gnomeclient for anything anymore IIRC. We used to use it for autostarting, but I think that got ripped out ages ago.

Thanks for looking into this.

Created attachment 93051 [details] [review]
drop unused gnome client frm main.c

the GnomeClient usage in main.c is a noop currently (commented out
code, and a "die" callback that does not seem useful)

also do GOption parsing via gtk and drop gnome_program_init()

there are 3 more use left of GnomeClient in gpm-manager.c and gpm-control.c they appear to be requesting a global session save on reboot, shutdown and logout. I am not sure how that can be replaced yet. If it really is needed probably via X11 XSM calls?

From attachment (id=93046):
+	g_option_context_add_group (context, gtk_get_option_group (FALSE));
+	g_option_context_parse (context, &argc, &argv, NULL);
+	gtk_init (&argc, &argv);

That really shouldn't be necessary. Maybe the problem is with the |FALSE| in the gtk_get_option_group call which makes it not open the default display after parsing the arguments, while gtk_init does open it?

Richard, can you please commit the patch in comment #26? It is independent of the other uses of GnomeClient in g-p-m.

As for the latter it is used to request a global session save in case of reboot, shutdown and interactive_action (I am not sure what this does). IIRC at one point they were ifdeffed out in the ubuntu package, somewhat supporting your view that GC is not necessary for anything anymore, but they're still there in the code, and probably not just legacy code.

>can you please commit the patch

It's now hard code freeze for gnome 2-20, but I'll hopefully get these patches tested and upstream when svn re-opens.

Richard.

@hughsie: *ping*

Thanks for the ping, applied to trunk:

2007-12-21  Richard Hughes  <richard@hughsie.com>

	* src/gpm-main.c: (main):
	Drop unused gnome client from main.c

	The GnomeClient usage in main.c is a noop currently (commented out
	code, and a "die" callback that does not seem useful)

	Patch from Jani Monoses <jani@ubuntu.com>, many thanks.

Richard,

I think this bug can be closed. All patches seem to be applied

Jaap