Re: [PATCH] Partial fix for Bug 30711

On 08/04/12 23:47, Brennan T Vincent wrote:
> Hi,
>
> I have modified the patch somewhat; please take a look at this new version.
>
> On Sat, Apr 7, 2012 at 2:32 AM, Brennan T Vincent
> <brennanv@email.arizona.edu> wrote:
> > Hi,
> >
> > Attached is a patch partially fixing Bug 30711. When the user has not
> > chosen to use OpenDocument format with extensions, text input fields
> > are now exported in a format OOo understands.

hi Brennan,

first, thanks for working on this bug.

hmmm... at first i wasn't sure if exporting as bookmark is the right
approach, but the other alternative is to export as an ODF text field,
and this doesn't permit having formatting in the field, and also the
fieldmarks may cross paragraph breaks, which is not supported at all in
ODF text fields, so bookmark indeed looks like the best fall-back.

now for some specific nit-picking:

in the git commit message, please refer to freedesktop.org bugs as
fdo#30711 because that enables automatic bugzilla notifications.

> +                            const Any aValue = xParameters->getByName("Name");
> +                            const Type aValueType = aValue.getValueType();
> +                            if (aValueType == ::getCppuType((OUString*)0))
> +                            {
> +                                OUString sValue;
> +                                aValue >>= sValue;
> +                                GetExport().AddAttribute(XML_NAMESPACE_TEXT, XML_NAME, sValue);
> +                            }

this could be simpler: the operator>>= returns a boolean to indicate
success, so this is equivalent:

const Any aValue = ...
OUString sValue;
if (aVaule >>= sValue)
{
   // use sValue
}

> +                        GetExport().StartElement(XML_NAMESPACE_TEXT, XML_BOOKMARK_START, 
> sal_False);
> +                        GetExport().EndElement(XML_NAMESPACE_TEXT, XML_BOOKMARK_START, 
> sal_False);

(and others) could also be simpler and less error prone: there is a
class that can be put on the stack and which starts the element in its
ctor and ends it in its dtor:

>     SvXMLElementExport aElem( GetExport(), !bAutoStyles,
>         XML_NAMESPACE_TEXT, XML_BOOKMARK_START, sal_False, sal_False );

also, i think the way you wrote it is even wrong, because there is a
non-obvious parameter here that has to be taken into account: bAutoStyles.

the ODF export actually iterates over all content twice, once with
bAutoStyles=true, to export the hard formatting attributes as automatic
styles, and second with bAutoStyles=false, to export the actual content.
 so the StartElement/EndElement must not be executed if bAutoStyles is
true (it may actually happen to work currently because the Writer is the
only application where the auto style pass is optimized away in some
cases because Writer can do auto styles itself, but it's still wrong).
passing the second boolean parameter to SvXMLElementExport ensures this.

if you fix these problems then i'll push the patch :)

> +                if (openFieldMark == TEXT)
> +                {
> +                    GetExport().StartElement( XML_NAMESPACE_TEXT, XML_TEXT_INPUT, sal_False );
> +                }
>                  exportText( aText, rPrevCharIsSpace );
> +                if (openFieldMark == TEXT)
> +                {
> +                    GetExport().EndElement( XML_NAMESPACE_TEXT, XML_TEXT_INPUT, sal_False );
> +                }
> +                openFieldMark = NONE;

this will cause the first (and only the first) span contained in the
fieldmark to be an ODF input field; i'm not really sure if that is a
good idea or not because it only works in the simplest case (but
generally field marks may contain multiple paragraphs...).

regards,
 michael