Edgewall Software
Modify

Opened 13 years ago

Closed 13 years ago

#10199 closed defect (fixed)

Image macro does not render links anymore

Reported by: mitar Owned by: Remy Blank
Priority: normal Milestone: 0.12.3
Component: wiki system Version: 0.12.2
Severity: normal Keywords:
Cc: mmitar@… Branch:
Release Notes:
API Changes:
Internal Changes:

Description

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.

Attachments (2)

image.patch (2.5 KB ) - added by mitar 13 years ago.
image-all.patch (5.8 KB ) - added by mitar 13 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 by Remy Blank, 13 years ago

Milestone: 0.12.3
Owner: set to 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?

comment:2 by mitar, 13 years ago

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?)

comment:3 by Remy Blank, 13 years ago

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?

comment:4 by mitar, 13 years ago

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>

comment:5 by Christian Boos, 13 years ago

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)]]
source:trunk/trac/htdocs/file.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)]]
source:trunk/trac/htdocs/file.png (no download link here: source:trunk/trac/htdocs)

Last edited 13 years ago by Christian Boos (previous) (diff)

comment:6 by Remy Blank, 13 years ago

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.

comment:7 by mitar, 13 years ago

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?

comment:8 by Remy Blank, 13 years ago

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).

by mitar, 13 years ago

Attachment: image.patch added

comment:9 by mitar, 13 years ago

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.

in reply to:  9 comment:10 by Remy Blank, 13 years ago

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().

comment:11 by mitar, 13 years ago

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

in reply to:  11 comment:12 by Remy Blank, 13 years ago

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 :)

by mitar, 13 years ago

Attachment: image-all.patch added

comment:13 by Mitar, 13 years ago

OK. Fixed also elsewhere where href was wanted.

comment:14 by Remy Blank, 13 years ago

Resolution: fixed
Status: newclosed

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).

comment:15 by Mitar, 13 years ago

Resolution: fixed
Status: closedreopened

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.

comment:16 by Remy Blank, 13 years ago

Resolution: fixed
Status: reopenedclosed

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!

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.