[LyX features/feature/docbook] DocBook: avoid using isspace in StartTag::writeTag.

[Thibaut Cuvelier]

On Fri, 11 Sep 2020 at 12:34, Pavel Sanda <sanda at lyx.org> wrote:

> On Thu, Sep 10, 2020 at 11:49:29PM +0200, Thibaut Cuvelier wrote:
> > diff --git a/src/xml.h b/src/xml.h
> > index ebb8b8e..568c5e5 100644
> > --- a/src/xml.h
> > +++ b/src/xml.h
> > @@ -162,6 +162,15 @@ docstring cleanID(docstring const &orig);
> > +///
> > +docstring removeSpace(docstring const & str);
> > +
> > +///
> > +docstring trimLeft(docstring const & str);
>
> Please add comments what exactly are these functions are supposed to do
> (even if it sometimes sounds as simple rephrasing the function name).
>

Done.


> > --- a/src/output_docbook.cpp
> > +++ b/src/output_docbook.cpp
> > @@ -518,8 +528,8 @@ void makeParagraph(
> >       auto nextpar = par;
> >       ++nextpar;
> >       auto pars = par->simpleDocBookOnePar(buf, runparams,
> text.outerFont(distance(begin, par)), 0, nextpar == end, special_case);
> > -     for (auto & parXML : pars) {
> > -             if (!std::all_of(parXML.begin(), parXML.end(), ::isspace))
> {
> > +     for (docstring const & parXML : pars) {
> > +             if (isNotOnlySpace(parXML)) {
> >                       if (open_par)
> >                               openParTag(xs, &*par, prevpar);
>
> Was this change intended?
>

Yes, it was: this call to ::issspace was the source of some crashes (I have
not been able to find the root cause).


> > diff --git a/src/output_docbook.cpp b/src/output_docbook.cpp
> > index 0dc5257..ff47aa6 100644
> > --- a/src/output_docbook.cpp
> > +++ b/src/output_docbook.cpp
> > @@ -308,8 +308,32 @@ void closeParTag(XMLStream & xs, Paragraph const *
> par, Paragraph const * nextpa
> >       if (nextpar != nullptr) {
> >               Layout const & nextlay = nextpar->layout();
> >               if (nextlay.docbookwrappertag() != "NONE") {
> > -                     closeWrapper = nextlay.docbookwrappertag() ==
> lay.docbookwrappertag()
> > -                                     &&
> !nextlay.docbookwrappermergewithprevious();
> > +                     if (nextpar->getDepth() == par->getDepth()) {
> > +                             // Same depth: the basic condition applies.
> > +                             closeWrapper = nextlay.docbookwrappertag()
> == lay.docbookwrappertag()
> > +                                            &&
> !nextlay.docbookwrappermergewithprevious();
> > +                     } else if (nextpar->getDepth() > par->getDepth()) {
> > +                             // The next paragraph is deeper: no need
> to close the wrapper, only to open it (cf. openParTag).
> > +                             closeWrapper = 0;
> > +                     } else {
> > +                             // This paragraph is deeper than the next
> one: close the wrapper,
> > +                             // disregarding
> docbookwrappermergewithprevious.
> > +                             // Hypothesis: nextlay.docbookwrappertag()
> == lay.docbookwrappertag(). TODO: THIS IS WRONG! Loop back until a layout
> with the right depth is found?
> > +                             closeWrapper = 1L + (long long)
> par->getDepth() - (long long) nextpar->getDepth(); // > 0, as
> nextpar->getDepth() < par->getDepth()
>
> Huh. Do we realistically need long long anywhere?
>

This was just to get rid of a warning at some point. However, this code has
been heavily refactored since then. (I've had a hard time to find it back,
I think these changes have been squashed with a cleaner version of the
code.)


> > +             } else {
> > +                     if (nextpar->getDepth() == par->getDepth()) {
> > +                             // This is not wrapped: this must be the
> rest of the item, still within the wrapper.
> > +                             closeWrapper = 1;
> > +                     } else if (nextpar->getDepth() > par->getDepth()) {
> > +                             // The next paragraph is deeper: no need
> to close the wrapper, only to open it (cf. openParTag).
> > +                             closeWrapper = 0;
> > +                     } else {
> > +                             // This paragraph is deeper than the next
> one: close the wrapper,
> > +                             // disregarding
> docbookwrappermergewithprevious.
> > +                             // Hypothesis: nextlay.docbookwrappertag()
> == lay.docbookwrappertag(). TODO: THIS IS WRONG! Loop back until a layout
> with the right depth is found?
> > +                             closeWrapper = 1L + (long long)
> par->getDepth() - (long long) nextpar->getDepth(); // > 0, as
> nextpar->getDepth() < par->getDepth()
>
> ditto.
>

Ditto :).


> > diff --git a/lib/scripts/layout2layout.py b/lib/scripts/layout2layout.py
> > index 2bb62e3..1164a12 100644
> > --- a/lib/scripts/layout2layout.py
> > +++ b/lib/scripts/layout2layout.py
> > @@ -11,7 +11,7 @@
> >  # This script will update a .layout file to current format
> >
> >  # The latest layout format is also defined in src/TextClass.cpp
> > -currentFormat = 83
> > +currentFormat = 84
> >
> >
> >  # Incremented to format 4, 6 April 2007, lasgouttes
> > @@ -284,6 +284,11 @@ currentFormat = 83
> >  # Incremented to format 83, 2 August 2020 by dourouc05
> >  # New tags DocBookWrapperMergeWithPrevious and DocBookAbstract
> >
> > +# Incremented to format 84, 17 August 2020 by dourouc05
>
> Would you mind to use something closer to your real name
> so people going through this don't need to guess?
>

Done too (by amending the right commit if it did not happen too long ago).


> > diff --git a/src/xml.h b/src/xml.h
> > index b585a48..e63623c 100644
> > --- a/src/xml.h
> > +++ b/src/xml.h
> > @@ -18,6 +18,8 @@
> >  #include <deque>
> >  #include <memory>
> >
> > +#include <iostream>
> > +
>
> Still needed? Heavy includes into headers are no good.
>

Actually, it was never required, I don't really know what it's doing here.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lyx.org/pipermail/lyx-devel/attachments/20200912/1b16dd35/attachment.html>