Image macro does not render links anymore

[mitar]

In 0.12 it seems Image macro does not render links anymore (link= argument). The same content which worked in 0.11 now does not work anymore.

I have traced the problem to this line​. elt is not an instance of Element but Fragment.

[Remy Blank]

Good detective work, thanks! I wonder how come the value returned by extract_link() has changed?

Would you like to provide a patch?

[mitar]

I tried to make it but was not successful. I am not sure how to get Elements our of the Fragment. I tried to iterate over the Fragment hoping that children will be Elements, but they aren't. So if you help me with how to get href out of it, I can make a patch. (And also a patch for test?)

[Remy Blank]

It will probably be interesting to find out first why you get a Fragment and not an Element. What kind of link is this? External or internal? To what type of resource?

[mitar]

It is a link to the attachment. So I have something like:

[[Image(thumbnail.jpg, link=attachment:bigone.jpg, border=1, alt=1)]]

and printing the elt produces:

<a class="attachment" href="/attachment/wiki/FooBar/bigone.jpg" title="Attachment 'bigone.jpg' in FooBar">attachment:bigone.jpg</a><span class="noprint"> <a class="trac-rawlink" href="/raw-attachment/wiki/FooBar/bigone.jpg" title="Download"><img src="/chrome/common/download.png" alt="Download"/></a></span>

[Christian Boos]

Right, it's because of the presence of the additional download link. I came up with the example of source: links:

[[Image(source:trunk/trac/htdocs/file.png,link=source:trunk/trac/htdocs/folder.png)]]
as the target is rendered as source:trunk/trac/htdocs/folder.png​ (← download icon and link)

But the following works:

[[Image(source:trunk/trac/htdocs/file.png,link=source:trunk/trac/htdocs)]]
(no download link here: source:trunk/trac/htdocs)

[Remy Blank]

Right, that was my fault :) Previously, links to attachments were a single Element, and now they are a Fragment containing multiple Elements.

In this case, you could get the first child Element. Or we could make extract_link() more intelligent. Or create an extract_href() that encapsulates the call to extract_link() and extracts the URL from the result. My preference goes to the last idea.

[mitar]

extract_href would be great idea. But I am not sure how to get the first child Element? If I am traversing the Fragment it recurses into the children. Should I just traverse children attribute? Is this external API of the Fragment?

[Remy Blank]

Yes, extract_href() should probably recurse into the Fragment or Element (preorder depth-first?) and return the content of the first href= attribute. Traversing the Fragment is done by iterating over the .children list (and Element is a subclass of Fragment).

[mitar]

Attached a patch. I found out that in wiki/intertrac.py this is probably also needed. Somebody did there the same deep-first search by himself. And in search/web_ui.py there is the same assumption that the link can be only an instance of Element.

[Remy Blank]

Replying to mitar:

Somebody did there the same deep-first search by himself.

Yes, that was me ;) I have a very bad memory for these things, I'm afraid.

And in search/web_ui.py there is the same assumption that the link can be only an instance of Element.

Don't hesitate to replace all these instances with calls to extract_href().

[mitar]

So I should make another patch? OK. I really hate patches. I like pull requests much more.

[Remy Blank]

Replying to mitar:

I like pull requests much more.

That's fine, too. If you're using Mercurial, I can easily pull. If you're using git, it will give me a good excuse to learn using it :)

[Mitar]

OK. Fixed also elsewhere where href was wanted.

[Remy Blank]

Reworked patch applied in [10709]. Instead of creating extract_*() variants, I have extracted the search for the right element into find_element() and placed it in trac.util.html. It also allows searching for an element having a given (CSS) class, which allowed using it in the last location where extract_link() was used (trac.mimeview.rst).

[Mitar]

We broke something. With this patch intertrac links do not work anymore for me. For example, for /intertrac/ticket:42 link I get:

TypeError: tuple indices must be integers, not str

Stack trace:

File "/usr/lib/python2.6/dist-packages/trac/web/main.py", line 511, in _dispatch_request
  dispatcher.dispatch(req)
File "/usr/lib/python2.6/dist-packages/trac/web/main.py", line 237, in dispatch
  resp = chosen_handler.process_request(req)
File "/usr/lib/python2.6/dist-packages/trac/wiki/intertrac.py", line 57, in process_request
  href = elt.attrib['href']
File "/usr/lib/pymodules/python2.6/genshi/core.py", line 366, in __getitem__
  items = tuple.__getitem__(self, i)

Line 57 in intertrac.py is among the lines we changed with this ticket.

[Remy Blank]

Right, my bad. I had incorrectly assumed that the Genshi object containing element attributes had the same interface as a dict. Fixed in [10712]. Thanks for the report!