Re: [PATCH] Partial fix for Bug 30711

Miklos and Michael,

I apologize for confusing you, Miklos; I was intending to ask for
comments on partially-completed work, not submitting a final product
to be pushed to the tree. Should I have given my e-mail a different
subject to indicate that? I'm still getting used to the conventions
and culture of this list :). You can revert the patch if you want; on
the other hand, it's probably not hurting anything to leave it there
for now while I work on something more complete, so it's up to you
what to do.

Thanks for the comments, Michael; I will take them into account and
hopefully have a finished patch sometime by the end of this week, if
my commitments with school permit.

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

As I discussed above, Miklos seems already to have pushed it, but I
won't be offended if someone reverts it :)

Cheers,
Brennan

On Tue, Apr 10, 2012 at 6:24 AM, Michael Stahl <mstahl@redhat.com> wrote:
> 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 (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
>
> _______________________________________________
> LibreOffice mailing list
> LibreOffice@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/libreoffice