epubcheck plugin for Sigil

[DNSB]

Originally Posted by davidfor

Did you just remove the path change, or also change it so that Sigil was opened in the same way as other Windows apps? The other differences were probably about making the PATH removal work, so it will probably work that way.

I'll do a little testing and talk to kiwidude about updating the plugin.

All I did was to change "sigil" to "ligis" in action.py so the test to see if Sigil was being opened failed and Sigil opened in the same way as other Windows apps. Quick and dirty.

Original:
# However we need a special case for Sigil which has issues with C runtime paths
  DETACHED_PROCESS = 0x00000008
  if external_app_path.lower().endswith('sigil.exe'):

Modification:
# However we need a special case for Sigil which has issues with C runtime paths
   DETACHED_PROCESS = 0x00000008
   if external_app_path.lower().endswith('ligis.exe'):

[DNSB]

Originally Posted by kovidgoyal

You can also access the builtin open with functionality by right clicking the view button and choosing view specific format (or right clicking a book entry and choosing that).

Yet another option. Thanks for the information.

[DiapDealer]

Hey Doitsu,

I've tracked (with some help) my double mimetype issue down to this plugin. The problem stems from you writing a new mimetype file to bk._w.ebook_root (which is the actual Sigil scratch directory). It's fine (while still usually discouraged) to use bk._w.ebook_root for read-only operations, but since you need to create a mimetype file for Epubcheck, you should probably create your own temp directory, copy the book contents there -- bk.copy_book_contents_to(temp_dir) -- and then create your mimetype file.

Let me know if there's anything else you need to correct the problem.

[Doitsu]

Hi DiapDealer,

The first version of this plugin actually copied the book to a newly created temp folder using .copy_book_contents_to(temp_dir). When I became aware of bk._w.ebook_root, I changed the code to use the actual temp folder, because I assumed that this would slightly speed up the validation process.

I'll change it back to use a separate temp folder.

[DiapDealer]

Originally Posted by Doitsu

The first version of this plugin actually copied the book to a newly created temp folder using .copy_book_contents_to(temp_dir). When I became aware of bk._w.ebook_root, I changed the code to use the actual temp folder, because I assumed that this would slightly speed up the validation process.

So I'm not crazy, then. I thought I remembered your plugin doing the copy_book_contents bit before.

You're right though, it probably did speed things up. It's just that that folder shouldn't have anything written to it by a plugin. That's Sigil's working temp directory. All changes to it should be handled by Sigil once it gets control back from the plugin.

Thanks for fixing it! I'll see if there's anything we can do on our end to detect/eliminate extra mimetype files and/or protect the contents of bk._w.ebook_root.

[KevinH]

yes, ever directly using any of the bk._w. type routines is really frowned upon. We can live with it for reading if there is a really good reason, but it should never be used for writing. If at all possible, please stick to using the official bookcontainer interface routines as documented in the Sigil Plugin Framework docs. The entire reason for creating the interface was to protect Sigil itself.

Thanks,

KevinH

[Doitsu]

Originally Posted by KevinH

yes, ever directly using any of the bk._w. type routines is really frowned upon.

I've uploaded a fixed version that'll use its own temp folder.

@DiapDealer: I'm sorry that I caused you extra work tracking down the double mimetype issue. From here on onward I shall no longer write to bk._w.

[DiapDealer]

Originally Posted by Doitsu

@DiapDealer: [/B] I'm sorry that I caused you extra work tracking down the double mimetype issue. From here on onward I shall no longer write to bk._w.

No problem. Tracking down issues is an obsession of mine that I take perverse pleasure in (no matter how much I might complain about it).

Regardless of whether or not this was a plugin's "fault," I think there's probably some tweaking that could be done to Sigil's zip routines. In my mind, it shouldn't really be possible to create a zip archive that has two files with the same name (but I could be wrong about that). It just seems to me that there should be some alarm bells going off, somewhere, long before an epub with two mimetype files makes it to ADE/RMSDK. Sigil lets it slide (after it already exists); and neither Flightcrew nor Epubcheck (local or online) seems to care about it. It's kind of weird.

Oh, well. It least we know what's going on now..

[PeterT]

I also saw from the screen shot that one of the mimetype's was stored uncompressed, and one was compressed.

[Toxaris]

It might be a bug or something strange on my side, but the plugin disappeared from the list in Sigil. I installed it again and it was there again. I did nothing that might have caused this. Perhaps a side effect from the update checker?

[Doitsu]

Originally Posted by Toxaris

Perhaps a side effect from the update checker?

The update checker code segment is only executed once the plugin runs. I.e. it has nothing to do with Sigil itself. However, if a plugin corrupts/deletes its own plugin.xml file, it might no longer be listed.
The ePubCheck plugin checks its version number using xml.etree.ElementTree, but doesn't write to plugin.xml. I.e., it shouldn't have the capability to modify its plugin.xml file.

@KevinH & DiapDealer: Could you please do a "mini code review" of the following version checking segment?

import xml.etree.ElementTree as ET

    # run plugin version check
    plugin_xml_path = os.path.abspath(os.path.join(bk._w.plugin_dir, 'EpubCheck', 'plugin.xml'))
    plugin_version = ET.parse(plugin_xml_path).find('.//version').text

This is the only code that accesses plugin.xml and the ElementTree information isn't used elsewhere in the plugin.

In addition to the version check code I also added the following line to plugin.xml:

<autostart>true</autostart>

However, this line should be ignored by Sigil 0.9.2 and older versions.

[DiapDealer]

Originally Posted by Doitsu

@KevinH & DiapDealer: Could you please do a "mini code review" of the following version checking segment?

import xml.etree.ElementTree as ET

    # run plugin version check
    plugin_xml_path = os.path.abspath(os.path.join(bk._w.plugin_dir, 'EpubCheck', 'plugin.xml'))
    plugin_version = ET.parse(plugin_xml_path).find('.//version').text

This is the only code that accesses plugin.xml and the ElementTree information isn't used elsewhere in the plugin.

In addition to the version check code I also added the following line to plugin.xml:

<autostart>true</autostart>

However, this line should be ignored by Sigil 0.9.2 and older versions.

I don't see anything wrong with it at first glance. It's just parsing/searching the element tree and not modifying it, so there should be no issue.

I'll try to download the newest version of the plugin and check it out sometime today, just to be sure.

[Toxaris]

Perhaps it was just a fluke. I will keep an eye on it though. Thanks for checking anyway.

[KevinH]

IMHO ..using element tree to parse the simple plugin.xml just to get the version is probably overkill in the extreme. You can more easily use regular expressions as I did here:

https://www.mobileread.com/forums/sho...2&postcount=19

See the _version_pattern and related code.

KevinH

[Doitsu]

Originally Posted by KevinH

IMHO ..using element tree to parse the simple plugin.xml just to get the version is probably overkill in the extreme. You can more easily use regular expressions as I did here:

I totally agree with you on that using an element tree to parse plugin.xml is most definitely overkill in the extreme. However, that's what I came up with before you posted your code example. (Using an element tree also gave me an opportunity to learn more about XPath.)
BTW, if you look at my source code, you'll notice that I implemented pretty much anything else that you suggested.