Re: bonobo-ui-node re-write ...

[Darin Adler <darin bentspoon com>]

On Wednesday, August 22, 2001, at 10:50  AM, Michael Meeks wrote:

[ Removed my retort to your remarks about GQuark since you recanted your 
objection to it. :-) ]

> > Overall, the change seems like a great idea. It does seem gratuitous
> > to make the code rely on the BonoboUINode structure rather than
> > continuing to use function calls to get things like next, previous,
> > and parent. If that change results in a measurable speedup, then I
> > guess it's worth it, but I doubt it does. Keeping the abstraction if
> > it's not prohibitively expensive might make it easier to make a future
> > change in this area.
>
> 	Gnome 1.4 is end of development lifetime and well into maintenance
> mode I hope, and I cannot envisage us switching to another UI node
> structure any time soon - so this doesn't worry me. But I agree, I only
> did the switch where I was supicious that I'd seen the method on a profile
> in the past ( not a good optimization heuristic though ).

I assumed you were planning on rolling these changes to GNOME 2, hence I 
didn't consider the "GNOME 1 end of life" aspect.

> > > +	if (!(txt = bonobo_ui_node_get_attr_by_id (node, id_id))) {
> > >
> > > +		txt = bonobo_ui_node_get_attr_by_id (node, verb_id);
> > >
> > > +		if (txt && txt [0] == '\0')
> > > +			txt = bonobo_ui_node_get_attr_by_id (node, name_id);
> > > +	}
> >
> > Looks to me like the above code is missing a ! in the if statement.
>
> 	I don't follow you - there was no functional change here. It is to
> allow verb="" to fallback to the node name ( AFAICS ).

My mistake.

> > You don't need the ? : here. All strdup functions (g_strdup,
> > xmlStrdup) in GNOME are designed to work on NULL too. So you can just
> > do:
>
> 	Why cut and paste the same message ? as soon as I realized this I
> fixed it everywhere - naturaly.

Great!

(I actually wrote this message twice, believe it or not. Sometimes I can't 
understand what's going on in that brain of mine.)

> 	Several reasons; firstly it is reasonable to pass "" to this
> method, and it is fair to get NULL back as the XML representation of an
> empty string I feel.

I agree. Especially if the function's comment for gtk-doc mentions this.

I think it would be nicer to be consistent about the handling non-empty 
strings, though. A warning for "xxxxx" but not on "xx" strikes me as 
slightly sloppy. But this is "angels on the head of a pin"-type stuff.

> 	Secondly, if you examine the memory parser you see our string soon
> hits:
>
> 	xmlCharEncoding
> 	xmlDetectCharEncoding(const unsigned char* in)
> 	{
> 	    if ((in[0] == 0x00) && (in[1] == 0x00) &&
> 	        (in[2] == 0x00) && (in[3] == 0x3C))
> 		return(XML_CHAR_ENCODING_UCS4BE);
>
> 	So it seems good to me to protect the user from the potential of a
> hidden seg fault as it runs over the end of the string - a little known
> libxml feature.

I think this warrants a comment in your code, to explain that this is a 
workaround for a specific bug, not just an arbitrary constant. Something 
along the lines of:

    /* libxml looks at the first four bytes unconditionally -- protect our 
callers from that */

Even knowing that I'm being incredibly over-pedantic, I can't help noting 
that this is not a problem for strings of length 3, only of length 2 or 
less.

> > > +	contains = node->content || (node->children && recurse);
> >
> > Why put this in a local variable? You should just put this test in the
> > if statement below.
>
> 	Indeed, originaly I used this twice hence the variable, but since
> refactoring I don't. The compiler will optimize it away nicely - it seems
> reasonable to leave it since it makes the method more readable.

I think it makes it less readable, because the suitability of this test is 
much clearer when you're down at that part of the function that's about to 
do the work, rather than here up top.

But since it's a matter of opinion and we disagree, I'll drop it.

    -- Darin