[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kfm-devel
Subject:    Re: PATCH: KonqRun::askSave and URLs containing '%'
From:       David Faure <david () mandrakesoft ! com>
Date:       2001-08-25 11:43:18
[Download RAW message or body]

On Saturday 25 August 2001 16:45, Maarten ter Huurne wrote:
> On Friday 24 August 2001 18:58, David Faure wrote:
> > On Friday 24 August 2001 20:52, Maarten ter Huurne wrote:
> 
> > > Will the translations still be valid when %1 and %2 are swapped?
> >
> > No, this is obviously a change in the to-be-translated string. But in the
> > HEAD branch, this is no problem. The poor translators will update their
> > translations once more :}
> 
> I was actually hoping the patch could make it into 2.2.1, which is why I 
> didn't want to break translations.
> 
> 
> I noticed two problems in the patch:
> 
> 1. The behaviour differs from QString::arg: the Qt function reads a digit 
> after the '%', the patch reads a number (as many digits as possible) after 
> the '%'. I changed the patch to read just one digit.
> 
> 2. If a '%' character occurs without digits after it, the patch complained 
> about an index being out-of-range, producing an erroneous error message.
> 
> A fixed patch is attached. I used diff -u this time, as you suggested. I 
> attached it rather than copied it into the body because it contains lines 
> longer than 80 characters.

I think it's a bit dangerous to have an untested patch (I mean, other than your
tests) in a branch that developers don't test, especially if it's not the solution
we choose for HEAD. We won't find the other problems in safeArg, so the patch
itself isn't "safe" :)

However, this bug can happen in many places where we use .arg(), and moving the
potentially dangerous argument to the last position isn't the best solution (in case
two arguments could have '%' in them).
So maybe we should have something like your safeArg in kdelibs-head, publically
available, and to be used whereever strings can contain '%' (and the message
has more than just %1). I don't see how to do this with the .arg() syntax, so 
another syntax is indeed needed. Maybe a "..." (va_args etc.) solution would 
be nicer to the developer though, instead of the QStringList.

-- 
David FAURE, david@mandrakesoft.com, faure@kde.org
http://perso.mandrakesoft.com/~david/, http://www.konqueror.org/
KDE, Making The Future of Computing Available Today

[prev in list] [next in list] [prev in thread] [next in thread]