Re: [lmi] wx assertion failure: invalid font

[Vadim Zeitlin]

On Wed, 10 Oct 2018 16:17:02 +0000 Greg Chicares <address@hidden> wrote:

GC> >  Yes, it does, and the reason for it is explained in the commit message 
and
GC> > a comment in https://github.com/vadz/lmi/pull/98
GC> I have one question--where I had tried this:
GC> 
GC>  TAG_HANDLER_BEGIN(page_header, "HEADER")
GC>      TAG_HANDLER_PROC(tag)
GC>      {
GC> +        // As usual, reuse the current container if it's empty.
GC>          auto container = m_WParser->GetContainer();
GC> +        if (container->GetFirstChild())
GC> +            {
GC> +            // It isn't, we need to open a new one.
GC> +            m_WParser->CloseContainer();
GC> +            container = m_WParser->OpenContainer();
GC> +            }
GC> 
GC> which conditionally closes and opens the current container,
GC> PR 98 does that unconditionally:
GC> 
GC> -        auto container = m_WParser->GetContainer();
GC> +        // Although the header typically occurs at the very beginning of 
the
GC> +        // HTML template, it doesn't mean that the current container is 
empty,
GC> +        // quite on the contrary, it typically isn't [...snip rest of 
block comment...]
GC> +        m_WParser->CloseContainer();
GC> +        const auto container = m_WParser->OpenContainer();
GC> 
GC> Would it be more robust to do this conditionally?

 Not really more robust, no. At best, slightly more efficient, by avoiding
an unnecessary empty container in the final HTML DOM structure, but in
practice I don't think this can happen, i.e. AFAICS it's impossible for the
<header> tag to come just after opening a new container. So I decided to
simplify the code by omitting to optimize for this case, which never
happens in practice.

GC> Would some bad thing happen if the current container actually is empty?

 No.

GC> I'm afraid of relying on today's "typical" usage when we might find
GC> other use cases in the future. Putting the question differently:
GC> suppose we add an assertion to the patch above (comments completely
GC> elided for clarity, and extra additions marked with doubleplus):
GC> 
GC> -        auto container = m_WParser->GetContainer();
GC> ++       // Precondition: current container must not be empty.
GC> ++       LMI_ASSERT(m_WParser->GetContainer()->GetFirstChild());
GC> +        m_WParser->CloseContainer();
GC> +        const auto container = m_WParser->OpenContainer();

 I wouldn't add such an assertion because if it does happen, it would still
be completely harmless, but we could do it and it shouldn't trigger. I'd
rather add the condition right now if you really want to have consistency
between this tag handler and the unbreakable_paragraph one, however. OTOH I
don't believe there is any particular reason for the different tag handlers
to be consistent with each other.

GC> and further suppose that someday that assertion fires; on that day,
GC> will we need to reintroduce the condition?

 Probably, yes. But it's difficult to be sure because, again, right now
this is just not going to happen, so something will have to change and it's
difficult to discuss what would need to be done to address a change about
which nothing is known yet.

GC> If it is harmless unconditionally to close the current container and
GC> open a new one, then shouldn't we make a similar change to the <P>
GC> handler...
GC> 
GC> TAG_HANDLER_BEGIN(unbreakable_paragraph, "P")
GC>     TAG_HANDLER_PROC(tag)
GC>     {
GC>         // Note: this code mimics what TAG_HANDLER_PROC()s for "div" and "p"
GC>         // tags in wxHTML itself do by copying their code because there is
GC>         // unfortunately no way to delegate to them currently.
GC> 
GC>         // As usual, reuse the current container if it's empty.
GC>         auto container = m_WParser->GetContainer();
GC>         if (container->GetFirstChild())
GC>             {
GC>             // It isn't, we need to open a new one.
GC>             m_WParser->CloseContainer();
GC>             container = m_WParser->OpenContainer();
GC>             }
GC> 
GC> ...which still has the condition? Is reusing an empty container merely
GC> an optimization? Or is it necessary for correct behavior?

 In the <P> case it can indeed happen and a quick check shows that it does
happen, in the simplest case right after closing one paragraph and starting
another one, i.e. the condition will be false (== container will be empty)
for the second tag in "<p>foo</p><p>bar</p>" input. I didn't measure how
significant is this optimization, but it does seem better to avoid
unnecessary cells in the HTML DOM tree, so I'd rather keep it.

 Regards,
VZ