Redmine

Thanks for the patch.
But it would be nice to fix RedCloth#inline_textile_link as well.

This patch doesn't work well with links inside parenthesis. Eg. (http://www.foo.bar/Test_foobar).
Trailing parenthesis is included in the url.

Paul Rivier wrote:

This is not an easy problem. Should we consider that parenthesis inside url should alway be balanced ? I don't think we should.
Then should we treat the particular case of a url with a preceding ( and a trailing ) ?

here is a naive patch for the later.

   def inline_auto_link(text)
        text.gsub!(AUTO_LINK_RE) do
          all, a, b, c, d = $&, $1, $2, $3, $4
          if a =~ /<a\s/i || a =~ /![<>=]?/
          else
################## PATCH BELOW
            if ( a[-1] == ?( and c[-1]==?) ) # looks like url is put inside parenthesis
              c=c[0..-2]
              d=")"+d
            end
################## PATCH ABOVE
            text = b + c
            %(#{a}<a class="external" href="#{b=="www."?"http://www.":b}#{c}">#{text}</a>#{d})
          end
        end
      end

Jean-Philippe Lang wrote:

Thanks for the patch.
But it would be nice to fix RedCloth#inline_textile_link as well.

What's the policy for this kind of "bundled-librairy" related problem ? Should we patch against bundled files ? Should we try to monkey-patch from outside ?

Should we consider that parenthesis inside url should alway be balanced ? I don't think we should.

I don't know which is the best solution but I'd really like that something like this works: (see: http://example.net).
I think we see this more often than a link with a single closing parenthesis at the end.
WDYT ?

Should we patch against bundled files ?

Yes, you can.
redcloth.rb is already patched: http://www.redmine.org/repositories/changes/redmine/trunk/lib/redcloth.rb

Jean-Philippe Lang wrote:

I don't know which is the best solution but I'd really like that something like this works: (see: http://example.net).
I think we see this more often than a link with a single closing parenthesis at the end.
WDYT ?

For sure I'd like this as well, but we first have to formulate clearly what we want. For the moment, it's like :

 "If the url ends a sentence in parenthesis, don't include the closing parenthesis in the URL" 

That would involve matching the whole sentence, before url to see if it starts with '(' and after to see if there isn't an other closing ')' later, like in "(see: http://fr.example.net/wiki/clavier_(musique) to read more)". I think we will have hard time doing so :/

Maybe we can do that :

  "If the url last char is closing ')' that is unbalanced in the url itself, take the ')' as external to the url

• (see: http://example.net)
• (see: http://fr.example.net/wiki/clavier_(musique) to read more)
• (see: http://fr.example.net/wiki/clavier_(musique))

WDYT ?

"If the url last char is closing ')' that is unbalanced in the url itself, take the ')' as external to the url

This is fine for me.

jean philippe, are you reasonably happy with this patch ? If not, could you explain how you think it should be improved please.