Re: [lmi] wx assertion failure: invalid font

[Greg Chicares]

On 10/10/18 12:16 AM, Vadim Zeitlin wrote:
> On Tue, 9 Oct 2018 21:31:12 +0000 Greg Chicares <address@hidden> wrote:
> 
[...snip my crude patch...]
> 
> GC> It's just a wild guess that I arrived at by comparing the TAG_HANDLERS
> GC> for <P> and <HEADER>. This does seem to prevent the problem for our
> GC> reproducible 'sample2gpp' test case.
> 
>  Yes, it does, and the reason for it is explained in the commit message and
> a comment in https://github.com/vadz/lmi/pull/98
I have one question--where I had tried this:

 TAG_HANDLER_BEGIN(page_header, "HEADER")
     TAG_HANDLER_PROC(tag)
     {
+        // As usual, reuse the current container if it's empty.
         auto container = m_WParser->GetContainer();
+        if (container->GetFirstChild())
+            {
+            // It isn't, we need to open a new one.
+            m_WParser->CloseContainer();
+            container = m_WParser->OpenContainer();
+            }

which _conditionally_ closes and opens the current container,
PR 98 does that _unconditionally_:

-        auto container = m_WParser->GetContainer();
+        // Although the header typically occurs at the very beginning of the
+        // HTML template, it doesn't mean that the current container is empty,
+        // quite on the contrary, it typically isn't [...snip rest of block 
comment...]
+        m_WParser->CloseContainer();
+        const auto container = m_WParser->OpenContainer();

Would it be more robust to do this conditionally? Would some bad thing
happen if the current container actually is empty?

I'm afraid of relying on today's "typical" usage when we might find
other use cases in the future. Putting the question differently:
suppose we add an assertion to the patch above (comments completely
elided for clarity, and extra additions marked with doubleplus):

-        auto container = m_WParser->GetContainer();
++       // Precondition: current container must not be empty.
++       LMI_ASSERT(m_WParser->GetContainer()->GetFirstChild());
+        m_WParser->CloseContainer();
+        const auto container = m_WParser->OpenContainer();

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

If it is harmless unconditionally to close the current container and
open a new one, then shouldn't we make a similar change to the <P>
handler...

TAG_HANDLER_BEGIN(unbreakable_paragraph, "P")
    TAG_HANDLER_PROC(tag)
    {
        // Note: this code mimics what TAG_HANDLER_PROC()s for "div" and "p"
        // tags in wxHTML itself do by copying their code because there is
        // unfortunately no way to delegate to them currently.

        // As usual, reuse the current container if it's empty.
        auto container = m_WParser->GetContainer();
        if (container->GetFirstChild())
            {
            // It isn't, we need to open a new one.
            m_WParser->CloseContainer();
            container = m_WParser->OpenContainer();
            }

...which still has the condition? Is reusing an empty container merely
an optimization? Or is it necessary for correct behavior?