Re: get/set_prop patch

[Alex Larsson <alexl redhat com>]

On Tue, 26 Jun 2001, Michael Meeks wrote:

>
> Hi Alex,
>
> On Tue, 26 Jun 2001, Alex Larsson wrote:
> > After some profiling of Nauilus and discussion with Michael i have
> > come up with a patch that basically removes Bonobu UI handler from the
> > Nautilus open new window profiles.
>
> 	The patch looks great overall, just a few comments:
>
> > +	full_path = g_malloc (strlen (path) + 1 + strlen (prop) + 1);
>
> 	I'm a fan of alloca, this is no problem though.
I can use alloca.

> > +		if ((property = strrchr (path, '/')) &&
> > +		    (property = strrchr (property, '#'))) {
>
> 	I wonder why you do the strrchr (path, '/') - it doesn't seem
> instantly useful to me - and again in the 'get'

Well, it was more of a safety thing, if you have something like
"/path/is#one/foo#property". But i guess this is kinda silly, since it
would break if you want to set a property on "/path/is#one".

> > +		property = property + 1; /* Skip the '#' */
> > +
> > +		xml = bonobo_ui_engine_xml_get_prop (engine, real_path, property);
> > +		g_free (real_path);
>
> 	you can't return NULL from a CORBA method [ for a string ] without
> getting a seg fault, so ... perhaps you want to split out the exception
> set below for both branches:
Yes.

> > +		xml = bonobo_ui_engine_xml_get (engine, path, nodeOnly);
> > +		if (!xml) {
> > +			CORBA_exception_set (ev, CORBA_USER_EXCEPTION,
> > +					     ex_Bonobo_UIContainer_InvalidPath, NULL);
> > +			return NULL;
> > +		}
>
> >  CORBA_char      *bonobo_ui_engine_xml_get         (BonoboUIEngine   *engine,
> >  						   const char       *path,
> >  						   gboolean          node_only);
> > +CORBA_char *     bonobo_ui_engine_xml_get_prop    (BonoboUIEngine   *engine,
>
> 	if you could knock the '*' across that'd be sweeter.
>
> 	Apart from that: lovely.

Good. Will fix up and commit.

> > This patch might also help evolution performance, and any app that
> > does a lot of bonobo_ui_component_set_prop.
>
> 	Which is wonderful - thanks again for all the work you've put in,
> if you could address the above and commit that'd be great - I'll do a new
> release shortly - but I'd like to get Darin's patch in too - Darin ?

I think Darin is out of town. He should be back shortly though.

/ Alex