Something has gone wrong ;-)

I've been looking into this (distractedly) for a few days.  Do you
have any console output?  Note gok has configure/autogen options
--enable-logging-normal and --enable-logging-exceptional (which can be
used together).

Does it only happen if you have been using accessible applications
with gok?

DH, scratch that, I am seeing the bug you describe and it is not the
same behaviour I was talking about -- will investigate shortly.

I think this has to do with the need to a g_strstrip()...

Bill, where are we with the problem of spaces in our xml?  Shall we
perform the g_strstrip early and once -- when we read the keyboards?

(david heads to a meeting :-( )

daniel:  is there a simple way we can ask libxml2 to strip all
leading/trailing whitespace?

I know we can specify 'preserve', which won't strip, and 'default'
(which is whatever-the-parser-wants to do), but is there a simple
piece of API we can use to ask libxml2's parser or reader to always strip?

thanks

I think the answer is no.

In XML all white space within content are considered significant
so XML parser really should not remove white spaces, except
in some case in attributes where this is defined by the spec.
Moreover the notion of "stripping" usually means removing
leading and ending whitespaces of a string of character, it's
really not proper to do that at the parser level because this
is really a change that require application specific knowledge.

Daniel 

Created attachment 21532 [details] [review]
a proposed fix.

David: I think we should do the strip more generally.  I attach an
alternate patch

Created attachment 21535 [details] [review]
alternate patch

David:  sorry to step on toes here.  Go ahead and apply the alternate
patch if you want, or I will.

If I've missed something here and the alternate isn't better, let me
know.  thanks

previous patch accidentally includes talking-gok stuff, please ignore
that part.

The relevant part of the patch is replacing 
the third param to gok_output_new() at the end of
gok_output_new_from_xml() with 

g_strstrip (xmlNodeGetContent (pNode)), 

[instead of just passing the result of xmlNodeGetContent directly into
gok_output_new()].

I had considered that but I am a bit worried that it might break
something...  for example if we added arguments to the commands in
launcher.kbd, would this break?

Without even looking at the context,
  g_strstrip (xmlNodeGetContent (pNode))
is absolutely and 100% garanteed to leak memory :-(
http://xmlsoft.org/html/libxml-tree.html#xmlNodeGetContent
  
Daniel

g_strstrip doesn't copy the string?  yoikes.

g_strstrip() does copy, but then there's no way to free the
xmlNodeGetContent value (which is also allocated). thanks DV.
Will send revised patch.  Note that g_strstrip only strips leading and
trailing whitespace, which is exactly what we need i.e. what we cannot
count on from the XML and intltool processes.

Created attachment 21539 [details] [review]
revised patch which removes leak and another pre-existing leak of similar origin

Created attachment 21542 [details] [review]
final patch

fixed in CVS.  Thanks DV for catching the really stupid mistake ;-)

When you do this:

nodeValue = xmlNodeGetContent (pNodeKeyChild);
stringval = (nodeValue != NULL) ? g_strstrip (nodeValue) : NULL;
g_free (nodeValue);

Isn't stringval invalid after the free?  My understanding of
g_strstrip is that it doesn't duplicate the string. Consider:

(nodeValue contains "   doggie   ")
stringval would point to the d...
   doggie   
   ^
 
But freeing nodeValue, frees 
   doggie
^

So stringval now points to free'd memory.  No?
   

no, g_strstrip doesn't alter the input string AFAIK.

glib docs are spectacularly uninformative about this.  I am checking
the glib source code.

David, you are right.  The patch is wrong.  I think DV may also be
wrong, since g_strstrip returns the same memory that it's passed.  

DV, is there some reason why memmove and overwriting buyes with '/0'
will cause problems here?  We are g_free-ing the strings we get from
xmlGetNodeContents, which seems perfectly appropriate here.

I am reverting the patch for the time being while we get clarification.

I kept the g_strstrip calls but removed the g_free.  This appears to
be in line with the implementation of g_strstrip() since it works
in-place and returns the same memory location that it's passed, thus
the return value from g_strstrip can safely be g_freed directly at a
later time.

Usually xmlFree() is free(), and g_free() is probably compatible
with free() too but those are assumption ...
libxml2 allows to overwrite the memory API with a different set of
function. If g_strstrip() really reuse the string memory area
(a bit yucky but understandable) then that should work.
sometimes 100% is not 100% :-\

Daniel

Bill, does g_strstrup always return the same address it is given? I am
thinking of the case of leading whitespace... (I suppose I could hunt
down the source code too - but if you know already ;-)

If so, is this closeable now?  The visible behaviour seems correct.

g_strstrip (not 'strup') always returns the address it's given, at
least in the current glib implementation.  If that implementation
changes such that a different address is returned, then lots of stuff
will start leaking and breaking, so I think we're safe WRT
bincompat/behavior here.

closing.