Oddity in e²->e³ conversion

[DiapDealer]

Originally Posted by KevinH

Really not sure how the opf is read (ie by recreating it from parsed form; or by sneaking around the wrapper to directly access the opf file) but epubcheck and epubveri plugins could have made different choices here.

According to the plugin's thread, there have been some changes to epubveri in that regard. Looking at the code, I can see that epubveri doesn't have an option to validate an unzipped epub, so a temporary epub is created. In earlier versions, the copied opf was being overwritten using get_opf (which I believe is Sigil's parsed rebuild of the opf). That was changed to using readotherfile. This was in an attempt to verify unsaved edits. But it was resulting in line numbers that didn't match what the user was seeing in Sigil when reporting errors.

https://www.mobileread.com/forums/sh...7&postcount=20


I may be putting too much emphasis on the silent opf rebuild, since it's been that way for a long, long time. Any save is going to correct a missing closing tag in the opf afterall. It just seemed odd to me that a problematic epub could be opened, validated, and closed (with the pre-existing error still intact) with no warning messages whatsoever

[KevinH]

It should not happen as SaveAllToDisk is used in prep for any plugin and Save by Sigil. I can not see how using bk.readotherfile in a plugin gets something different. Even if it is just missing the xml header line if SaveAllToDisk actually is working properly. So I am with you on this. Something is strange. I will explore this with some debug output in ResourceObjects/TextResource.cpp to make sure its SaveToDisk routine is properly being hit (for the opf's bookpath). The OPFResource inherits from XMLResource which in turn inherits from TextResource, so that is the routine that should be triggered unless we override it in OPFResource itself.

[KevinH]

Actually, OPFResource overrides SaveToDisk so I will take a look at it.

[DiapDealer]

For what it's worth: The latest version of the epubveri is now returning the same two errors that epubcheck did in the original post. The missing closing spine tag is not being reported by epubcheck or epubveri. So that leads me to believe that plugins are getting the same opf (corrected to add the closing spine tag) that Sigil is seeing. The corrected closing tag has not been saved to the epub yet, but both validators are ignoring it. Which leads me to believe that the opf has indeed been saved to disk (to Sigil's scratch folder) before being handed off to the plugin.

[KevinH]

Whew! That is good to hear!

Staring at tiny code in my phone was killing me!

[DiapDealer]

I'm still a little concerned about how an epub with a bad opf can potentially be opened, validated with no errors, and then closed (without saving) with an opf error still intact. With no warnings.

But as I said, I think that particular situation has been a possibility for a long time. But the epub would usually be saved at some point, or something else would be flushed to disk to change the modified status of the epub.

And if the Epub3-itizer plugin gets its fix, we should be in a good place.

[Kayadelenium]

Originally Posted by DiapDealer

why the epubveri plugin can see the missing spine closing tag while Sigil and epubcheck cannot

Author of epubveri here. It is not a parsing difference, and I would rather kill that idea than let it stand. I do not have DNSB's book, so I built the smallest one with the same fault — an OPF whose </spine> is simply absent — and gave it to epubcheck 5.3.0:

FATAL(RSC-016): content.opf(15,3): The element type "spine" must be terminated by the matching end-tag "</spine>".

The difference is which bytes each tool was given. The plugin asks Sigil for the container with copy_book_contents_to, and for the OPF that routes through readotherfile, which has this in wrapper.py:

if id == self.opfbookpath:
    if id in self.modified:
        return self.build_opf()

So Sigil hands back its live rebuild only when the OPF is marked modified, and the file from the ebook root otherwise. In this case it was the second — which is how I know, without having seen the book, that the OPF was not marked modified after the conversion plugin wrote it. The plugin validated what was actually in the container, and the container was malformed. Saving fixed it because saving is what flushes the repair, exactly as KevinH describes.

So epubveri was not cleverer; it was looking at the file rather than at the model. Which of the two epubcheck was given in DNSB's run I cannot tell from here — run over the container it reports this fault, as above, so if it stayed quiet it was reading something else.

Now the part I would like your opinion on, because this thread has found a hole in my reasoning rather than in my code.

Reading the file instead of the model was a deliberate choice, and I made it for one reason: line numbers. Version 0.1.0 substituted get_opf() unconditionally, and build_opf re-serialises from Sigil's model — it sorts the manifest by id and rewrites each item — so the plugin was reporting line numbers into a document nobody could see. On one book a manifest entry sat at line 91 in the file, 95 in the rebuild and 96 in what Code View displayed. Taking the file from the ebook root put the numbers back on the text in front of the user.

DNSB's book breaks that reasoning. Here Code View shows a </spine> that is not on disk, so the file and the display disagree, which is the one thing my choice was supposed to guarantee. The line numbers are then right about the file and wrong about the screen, by one line, from the spine onwards.

So, two questions for the three of you, since you know Sigil far better than I do:

1. Which artefact should a validator inside Sigil report on? The container as it stands on disk is what ships if the user never saves — and it is what caught this — but it is not what they are looking at. The model is what they see and what a save will write, but it will never show them a fault Sigil has already quietly repaired. I currently do the first, and after this thread I am no longer sure that is right for an editor plugin as opposed to a command line.

2. Is there any way for a plugin to know that Sigil repaired something while loading? If there is, the honest thing is to say so in the results — "this was repaired in memory, save to write it" — rather than to report a fault the user cannot find in Code View, or to hide one that is really there. Comparing the file against build_opf() is not it: the re-serialisation differs from the original even when nothing is wrong.

Either way, thank you for chasing this one down to the line in opf_newparser.py. That is a better outcome than the validator noise that started it.

[KevinH]

To clarify, Validators inherit from the output container so they should be using only the copy_book_contents and that in turn uses readotherfile but since a validator plugin and not the edit plugin, the opf should never be marked as modified by the plugin. So you should get the opf file itself not, the rebuild. Rebuild is used by edit plugins that can make changes to its copies and therefore the only valid opf if modified is the rebuilt one.

So in a validator plugin if the save all to disk is indeed working as expected for the opf it should result in any Sigil internal user made modifications or fixes to the opf being flushed to disk first before any plugin is launched and it should result in readotherfile returning a file that exactly matches what Sigil is showing and using internally. If that is not happening, it is a bug that I will track down and fix upon my return next week.

As for changes to notify for opf fixes, they are not needed by design as Sigil requires a 100% machine readable opf at all times and so auto repairs common xml issues with each read, to prevent Sigil from losing data. That is not changing.

So somewhere the built-in flush to disk the happens before any plugin is not happening as expected as it seems to forget to flush auto-repairs for some reason that I will look into.

Hope this clarifies.

[Kayadelenium]

Originally Posted by KevinH

the built-in flush to disk that happens before any plugin is not happening as expected

Thank you — that answers both questions, and the first one settles a choice I had made for a reason that turns out to be shakier than I thought. Since you are away, here is what I could work out from this end, in case any of it saves you time when you get back. All of it is from your published source — line numbers are from master today, so they may be a little off against a release — and none of it is a diagnosis; I have marked the one place I am guessing.

1. How the plugin sees the file at all — it has no other option

Worth stating plainly, because it means no plugin can be doing anything clever here. wrapper.py:213 initialises self.modified to an empty OrderedDict, and every later entry comes from the running plugin's own calls. Sigil does not seed it. So the readotherfile special case can only return build_opf() for an OPF that this plugin has already changed, and a validation plugin — which changes nothing — always receives the file from ebook_root. That is not a design decision of mine; there is no other outcome available.

2. The flush looks right at every step I could follow

I went looking for a place where the OPF might be skipped and could not find one:

• PluginRunner.cpp:283-286 — SaveTabData() then m_book->SaveAllResourcesToDisk(), under your own comment about flushing before the plugin. Called.
• Book.cpp:1446 maps SaveOneResourceToDisk over m_Resources.values(), and FolderKeeper.cpp:448 puts m_OPF into m_Resources. The OPF is in the list.
• Resource.h:230 declares SaveToDisk virtual and OPFResource.h:86 overrides it, so the dispatch reaches OPFResource::SaveToDisk rather than the base.
• OPFResource.cpp:342-346 runs CleanSource::ProcessXML — XMLPrettyPrintBS4 — then SetText, then TextResource::SaveToDisk. The early return at TextResource.cpp:97 (!m_CacheInUse && !m_IsLoaded) cannot fire on this path, because SetText has just set m_CacheInUse at TextResource.cpp:77.

So on that reading every plugin launch should rewrite the OPF, pretty-printed through BS4 — which would also have closed the spine tag on its way past.

A cheap test of that, using what is already in this thread: DNSB attached the converted OPF in post 8. If it is neither pretty-printed in Sigil's style nor closed, then no OPFResource::SaveToDisk ever wrote it, and the question is why the call did not reach there rather than what it did when it arrived. That is a two-minute check and it would split the search in half.

The one place I would instrument, and here I am guessing: SaveAllResourcesToDisk uses QtConcurrent::blockingMap, so SaveOneResourceToDisk runs off the GUI thread, and TextResource::SetText carries your own comment about delaying the QTextDocument update when it is not called from the main thread. GetText() on that path reads m_TextDocument->toText(). Printing what GetText() returns for the OPF at that moment would say whether the text being processed is the one you expect.

3. The other door, which is the part I would fix regardless

Separately from the flush: launcher.py:157-158 calls container._w.write_opf() at the end of an edit plugin, and wrapper.py:413-422 writes build_opf() straight to disk as bytes — no parse, no mend, no check. Since writeotherfile refuses the OPF as a protected file (wrapper.py:929), that write-back is the only way a plugin's OPF text reaches the book, and nothing on it asks whether the text is well-formed.

You have fixed Epub3-itizer, so this particular book will not happen again. But the next plugin that builds an OPF by hand walks through the same door, and the fault lands silently in the user's book rather than in the plugin author's lap. A single ElementTree parse there — refuse the write and report it — would have turned DNSB's afternoon into an error message at the moment the bad text was produced. Happy to open a PR if that is welcome, or to leave it entirely alone if you would rather decide the shape yourself.

4. A detector, if it is useful

Fifteen lines, no dependencies, works on any book. Drop it in as a validation plugin and it answers "did the flush happen" in one click:

import os
from xml.etree import ElementTree as ET


def run(bk):
    bookpath = bk.get_opfbookpath()
    raw = bk.readotherfile(bookpath)
    if isinstance(raw, bytes):
        raw = raw.decode("utf-8", "replace")
    try:
        ET.fromstring(raw)
    except ET.ParseError as failure:
        bk.add_result("error", bookpath, "-1",
                      "the OPF a plugin receives is not well-formed: %s"
                      % failure)
        return 0
    bk.add_result("info", bookpath, "-1",
                  "the OPF a plugin receives is well-formed (%d bytes)"
                  % len(raw))
    return 0

I ran it both ways rather than writing it from memory: against a sound book it reports well-formed, and against a book whose OPF I stripped the closing spine tag from on disk it reports mismatched tag: line 15, column 2. Worth knowing from the same run: the launcher's own Opf_Parser reads that malformed OPF without complaint and the plugin starts normally, which is presumably why nothing upstream of the plugin noticed either.

One thing that made all of this cheap, in case it is useful to anyone else here: launcher.py can be driven directly, with no GUI — hand it a book directory, an output directory, "validation" and a plugin path, run it under Sigil's bundled Python, and you get the real Wrapper and the real result XML. I had been testing my plugin against a container of my own making because I had written down that Sigil could not be scripted. The window cannot; the launcher can, and every Sigil behaviour my plugin got wrong so far had been learned by installing a zip and clicking.

Not chasing this — it is your bug to fix in your own time, and none of it is urgent from my side. It just seemed better to hand over what I had found than to sit on it.

[KevinH]

I am well familiar with the code as I designed it close to 10 years ago. And I have been programming for over 50 years and do understand how to debug. Note: ePub3-itizer is an *output* plugin not an edit plugin so it is the Sigil Importer for epub which allowed it in with a bad form, which it was designed to do to handle poor epubs. And PluginRunner does do an OPF Well-formed check before accepting the results of an edit plugin and does so for every xml and xhtml file so that is not the problem.

Either the flush to disk is not working in some way (no rtti so not clear if more explicit qobject cast is sometimes needed or that somehow a broken opf was loaded by Sigil's ImportEpub and no further call to load the opf for any Sigil tool was ever made meaning no auto repair was done. Either way we will take care of it in a few days when I am back and have access to more than my phone. Thank you but your help is not actually helping in this case. The problem is most likely in the Sigil ImportEpub code that auto repairs before any initial read but purposely does not update the opf immediately after unpacking it to prevent comments in the opf from being initially lost. Normally this does not hurt anything as whenever the opf must be read by Sigil it is auto-repaired first then saved upon a flush. So this is an odd case as no further reading of the opf seems to have been done.

We will handle it from here.

[Kayadelenium]

Originally Posted by KevinH

The problem is most likely in the Sigil ImportEpub code that auto repairs before any initial read but purposely does not update the opf immediately after unpacking it.

Thank you — that is upstream of anywhere I got to, and it fits what we saw.

I will leave it with you and watch the thread. Happy to test anything on my side once you have had a look.