Edgewall Software
Modify

Opened 9 years ago

Closed 8 years ago

Last modified 5 years ago

#10562 closed defect (fixed)

[patch] Linking images not possible for pages with ':' in name

Reported by: Dirk Stöcker Owned by: Christian Boos
Priority: normal Milestone: 1.0.2
Component: wiki system Version: 1.0
Severity: normal Keywords: wikipagename
Cc: Branch:
Release Notes:

Image macro can now reach attachments on Wiki pages having a : in their name (e.g [[Image("page:fr":image.png)]]).

API Changes:
Internal Changes:

Description

With Image(wiki:page:name) it is possible to link to attachements from other pages.

This is not possible when the page name contains a ':'. Encoding the : With %3A does not work as well as a lot of other workarounds I tried.

This is especially disturbing, as the default localization used the ':' to separate translations (like used for a lot of wikis).

Version 0.12.3 (there is no entry for 0.12.3 ATM :-)

Attachments (0)

Change History (19)

in reply to:  description comment:1 by osimons, 9 years ago

Version: 0.12.3

Replying to dstoecker:

Version 0.12.3 (there is no entry for 0.12.3 ATM :-)

There is now… Thanks. ;-)

comment:2 by Dirk Stöcker, 8 years ago

Version: 0.12.31.0

Any chance this gets fixed?

comment:3 by Christian Boos, 8 years ago

Keywords: wikipagename added
Milestone: next-stable-1.0.x

Let's keep it on the radar, at least.

comment:4 by Christian Boos, 8 years ago

Note that this has now to play well with #10850, i.e. ":" in attachment filenames.

comment:5 by Christian Boos, 8 years ago

While at it, another Image macro enhancement would be to allow for "../" prefix (get the attachment from the parent page, useful for quickly fixing links after moving content to a subpage).

comment:6 by Dirk Stöcker, 8 years ago

I think a minimum fix, which could do no harm would be to allow %3A for encoding. although not really fine, it probably could also fix a bunch of other possible issues.

And probably it only needs a single call to an entity decoder at the right place.

comment:7 by Dirk Stöcker, 8 years ago

Summary: Linking images not possible for pages with ':' in name[patch] Linking images not possible for pages with ':' in name

Following allows to use character encoding for me:

--- wiki/macros.py_  2012-10-04 23:39:59.557103777 +0200
+++ wiki/macros.py   2012-10-04 23:55:18.907440181 +0200
@@ -32,7 +32,7 @@
 from trac.util.datefmt import format_date, from_utimestamp, user_time
 from trac.util.html import escape, find_element
 from trac.util.presentation import separated
-from trac.util.text import unicode_quote, to_unicode, stripws
+from trac.util.text import unicode_quote, unicode_unquote, to_unicode, stripws
 from trac.util.translation import _, dgettext, cleandoc_
 from trac.wiki.api import IWikiMacroProvider, WikiSystem, parse_args
 from trac.wiki.formatter import (
@@ -589,6 +589,7 @@
             desc = url.rsplit('?')[0]
         elif filespec.startswith('/'):        # project-relative
             params = ''
+            filespec = unicode_unquote(filespec)
             if '?' in filespec:
                 filespec, params = filespec.rsplit('?', 1)
             url = formatter.href(filespec)
@@ -599,6 +600,8 @@
             #                                 # or intertrac:realm:id
             realm, id, filename = parts
             intertrac_target = "%s:%s" % (id, filename)
+            id = unicode_unquote(id)
+            filename = unicode_unquote(filename)
             it = formatter.get_intertrac_url(realm, intertrac_target)
             if it:
                 url, desc = it
@@ -607,6 +610,7 @@
                 attachment = Resource(realm, id).child('attachment', filename)
         elif len(parts) == 2:
             realm, filename = parts
+            filename = unicode_unquote(filename)
             if realm in browser_links:   # source:path
                 # TODO: use context here as well
                 rev = None

comment:8 by Christian Boos, 8 years ago

As the TracLinks syntax wiki:"page:name" works just fine, I'd rather support that in the Image macro.

I highly suspect that if we start to URL decode the links, we'll get some other surprising behavior.

comment:9 by Dirk Stöcker, 8 years ago

Your wish is my command

  • macros.

    old new  
    1818from itertools import groupby
    1919import inspect
    2020import os
     21import csv
    2122import re
    2223from StringIO import StringIO
    2324
     
    578579                        attr[str(key)] = val # will be used as a __call__ kwd
    579580
    580581        # parse filespec argument to get realm and id if contained.
    581         parts = filespec.split(':')
     582        parts = csv.reader([filespec], delimiter=':').next()
    582583        url = raw_url = desc = None
    583584        attachment = None
    584585        if (parts and parts[0] in ('http', 'https', 'ftp')): # absolute

Hmm, I start to like some parts of python :-)

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:10 by Christian Boos, 8 years ago

The "battery included" part, I suppose? ;-)

Going for the csv module seems a bit heavy handed here… Furthermore, it does work for the use case I've given, but not for the other valid syntax wiki:'page:name'. OTOH, using a regexp here would make things more complicated.

comment:11 by Dirk Stöcker, 8 years ago

Can be fixed as well:

parts = csv.reader([filespec.replace('\'', '"')], delimiter=':').next()

As page names should not have ' inside that should be correct. I was convinced CSV takes care of both quotes itself.

With RE:

parts = re.compile(r'''((?:[^:"']|"[^"]*"|'[^']*')+)''').split(filespec)

comment:12 by Christian Boos, 8 years ago

wiki:"Scarlett_O'Hara"?

I'll take the regexp version ;-)

Plus the following:

def recompose_string_list(segments, sep=':'):
    """Recompose a string list by concatenating all consecutive elements
    stopping at the separator ones.
    """
    parts = ['']
    for s in segments:
        if s == sep:
            parts.append('')
        else:
            parts[-1] += s
    return parts

comment:13 by Dirk Stöcker, 8 years ago

There is an bug in the RE. This RE actually works correct:

parts = [i.strip('''['"]''') for i in re.compile(r''':(?!(?:[^"':]|[^"']:[^'"])+["'])''').split(filespec)]

It does not care that quotes are actually paired correctly, but I don't want to make it more complex.

comment:14 by Dirk Stöcker, 8 years ago

Could this last one please be applied? It is tested for 4 month on JOSM page now any has no drawbacks.

comment:15 by Christian Boos, 8 years ago

Milestone: next-stable-1.0.x1.0.2
Owner: set to Christian Boos
Status: newassigned

Too late for 1.0.1, but for 1.0.2, promised ;-)

comment:16 by Dirk Stöcker, 8 years ago

Well, now?

comment:17 by Christian Boos, 8 years ago

I'll go through all pending 1.0.2 tickets this week.

comment:18 by Christian Boos, 8 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Suggested fix implemented in r11732, together with a test.

Thanks for your patience!

comment:19 by Christian Boos, 8 years ago

Release Notes: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Christian Boos 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.