Docutils 0.13 is out

[Guenter Milde]

Deas Sphinx developers,

the new Docutils release 0.13 if finally out. It includes many bugfixes
and the new HTML5 writer (no need to ship a copy).
See http://docutils.sourceforge.net/RELEASE-NOTES.html

Unfortunately, it also leads to an error with the "image" directive, due to

* dropping the use of the "heterogenous stack" ``self.context``
in the visit_image() / depart_image() functions

* Sphinx unsafe "partial overwriting" of this function pair:

While most overwriting in ``sphinx/writers/html.py`` is pairwise
(also in cases where only one function is changed:
# overwritten (but not changed) to keep pair of visit/depart_term
# overwritten (but not changed) to keep pair of visit/depart_definition
), this is not the case for visit_image(), where the matching parent function
`depart_image()` is not overwritten.


The following patch will make Sphinx fit for both, Docutils 0.13 and earlier
versions:

--- /usr/lib/python2.7/dist-packages/sphinx/writers/html.py 2016-10-01 17:14:37.000000000 +0200
+++ /tmp/html28485.1990.py 2016-12-12 08:44:52.914722357 +0100
@@ -498,6 +498,10 @@
node['height'] = str(size[1])
BaseTranslator.visit_image(self, node)

+ # overwritten (but not changed) to keep pair of visit/depart_image
+ def depart_image(self, node):
+ self.body.append(self.context.pop())
+
def visit_toctree(self, node):
# this only happens when formatting a toc from env.tocs -- in this
# case we don't want to include the subtree


This patch should replace the restriction on the Docutils version
https://github.com/sphinx-doc/sphinx/pull/3217
and related.

Sorry for the trouble,

Günter Milde

[Komiya Takeshi]

Hi Guenter,

Thank you for letting us know, and congratulations!

This patch should replace the restriction on the Docutils version 
https://github.com/sphinx-doc/sphinx/pull/3217 
and related. 

The visit_image() handler of Sphinx' HTML writer pushes context only if svg images.

For non SVG images (like JPEG, PNG and so on), it calls visit_image() of docutils' HTML writer.

But, in docutils-0.13, the visit_image() does not push any context.

So we should pop out the context with conditionally.

Thanks,

Takeshi KOMIYA

2016年12月12日月曜日 16時55分45秒 UTC+9 Guenter Milde:

[Guenter Milde]

Dear Komiya,

On 2016-12-12, Komiya Takeshi wrote:

> Thank you for letting us know, and congratulations!

> This patch should replace the restriction on the Docutils version
>> https://github.com/sphinx-doc/sphinx/pull/3217
>> and related.

> The visit_image() handler of Sphinx' HTML writer pushes context only if svg
> images.

Are you sure?

Docutils' 0.12 visit_image() always pushed an element to "context" - but
it could be an empty string.

And it always popped one element from the "context" stack.

If this worked well with Sphinx, just adding the "old" depart_image()"
should work, too.

> For non SVG images (like JPEG, PNG and so on), it calls visit_image() of
> docutils' HTML writer.
> But, in docutils-0.13, the visit_image() does not push any context.

> So we should pop out the context with conditionally.

As long as push/pop is always levelled and you overwrite both, visit and
depart functions, it should be safe.


Günter Milde

[Komiya Takeshi]

Are you sure? 

Yes, the following is a summary of visit_image() of Sphinx.

    # overwritten

    def visit_image(self, node):

        ...

        if uri.lower().endswith(('svg', 'svgz')):

            ...

            if 'align' in node:

                self.context.append('</div>\n')

            else:

                self.context.append('')

            return

        ...
        BaseTranslator.visit_image(self, node)

As you see at last line, the handler calls visit_image() of base class.

So status of context has been changed with both uri and version of docutils:

* docutils-0.12 + SVG => pushed

* docutils-0.12 + PNG => pushed

* docutils-0.13 + SVG => pushed

* docutils-0.13 + PNG => NOT pushed

So my patch [1] pops out context conditionally.

[1] https://github.com/sphinx-doc/sphinx/pull/3217 

Thanks,

Takeshi KOMIYA

2016年12月12日月曜日 22時34分22秒 UTC+9 Guenter Milde:

[Guenter Milde]

On 2016-12-12, Komiya Takeshi wrote:

>> Are you sure?

> Yes, the following is a summary of visit_image() of Sphinx.

> # overwritten
> def visit_image(self, node):
> ...
> if uri.lower().endswith(('svg', 'svgz')):
> ...
> if 'align' in node:
> self.context.append('</div>\n')
> else:
> self.context.append('')
> return

> ...
> BaseTranslator.visit_image(self, node)

> As you see at last line, the handler calls visit_image() of base class.
> So status of context has been changed with both uri and version of docutils:

> * docutils-0.12 + SVG => pushed
> * docutils-0.12 + PNG => pushed
> * docutils-0.13 + SVG => pushed
> * docutils-0.13 + PNG => NOT pushed

> So my patch [1] pops out context conditionally.

Thank you for the clarification. I overlooked the call to the base class.

How about a "balanced" approach:

Both visit_ and depart_ call the base class under the same conditions::

> def visit_image(self, node):
> ...
> if uri.lower().endswith(('svg', 'svgz')):
> ...
> if 'align' in node:
> self.context.append('</div>\n')
> else:
> self.context.append('')
> return

> ...
> BaseTranslator.visit_image(self, node)

def depart_image(self, node):
uri = node['uri']
if uri.lower().endswith('svg') or uri.lower().endswith('svgz'):
self.body.append(self.context.pop())
else:
BaseTranslator.visit_image(self, node)

No need to check Docutils versions. Save also if we decide to change again ;-)

Günter

[Komiya Takeshi]

Good point! it looks better.

I will merge it later :-)

Thanks,

Takeshi KOMIYA

2016年12月13日火曜日 1時12分12秒 UTC+9 Guenter Milde: