Patch for Bug 163098

[Frank Reininghaus]

Hi Peter, hi everyone,

On Friday 22 August 2008 20:26:44 Peter Penz wrote:
> Am Friday, 22. August 2008 02:11:13 schrieb Frank Reininghaus:
> > I've looked a bit into
> > http://bugs.kde.org/show_bug.cgi?id=163098
> > (crashes Konqueror and Dolphin if you enable "Automatic" text completion
> > in the location bar, enter any 1-character string that prompts a
> > completion and hit backspace).
>
> I'm not familiar with the code of KUrlCompletion and hence I'm not sure
> whether having an empty url_copy string at this part of the code is a valid
> assertion. But from my point of view it is save to commit this patch.
>
> I'd suggest to wait with committing this patch until Tuesday, so that David
> Faure has the chance to comment on this too.

just by looking at the code, I can't really say if having an empty url_copy in 
KUrlCompletionPrivate::MyURL::init () is a valid assertion. Going back in the 
backtrace, one finds that the QString 'url_copy' comes from the 
argument 'txt' in KLineEditPrivate::doCompletion() in 
kdelibs/kdeui/widgets/klineedit.cpp (the frames in between are different for 
the Konqueror and Dolphin case, but that's where it all begins). So the 
alternative to the approach of my patch would be to stop all completion 
efforts for empty strings in this method (or even before).

However, if it is decided to fix the problem in 
KUrlCompletionPrivate::MyURL::init (), I propose to modify my patch slightly. 
I think it might be better to replace "url_copy.at(0) == QLatin1Char('$')" 
by "url_copy.startsWith (QLatin1Char('$'))" and let QString::startsWith () 
take care of the emptiness check, just like it's done at the beginning of 
KUrlCompletionPrivate::MyURL::init () to check for "#" at the beginning in 
the URL (if this had been done using url_copy.at (0) too, the crash would 
occur already there). The attached patch "patch-bug-163098-Version2.diff" 
contains this change, I get no more URL completion crashes with it. 

Note that the file kurlcompletion.cpp contains 5 more lines where a call to 
QString::at(0) combined with a manual emptiness check (either with 
QString::isEmpty () or QString::length ()) could be replaced by 
QString::startsWith (). I could provide a patch for that too if there is 
agreement that this makes sense.

Another note (unrelated to the bug, just an idea to make the code in the 
second half of KUrlCompletionPrivate::MyURL::init () shorter and easier to 
read): One can reduce the complexity by a reordering of the if-statements: 
Right now, the outer if-statement is "if ( cwd.isEmpty() )". In both 
branches, there is an inner if-statement "if 
( !QDir::isRelativePath(url_copy) || ...)". Note that the "true" branch of 
both inner if-statements is the same (if you move the statement "m_kurl = new 
KUrl;" which now occurs before the first "if 
( !QDir::isRelativePath(url_copy) || ...)" into the two branches of this 
if-statement).

Therefore, it might make sense to make "if 
( !QDir::isRelativePath(url_copy) || ...)" the outer if-statement. "if ( 
cwd.isEmpty() )" is then only needed in the else-branch. The attached 
patch "patch-bug-163098-Version2-Extended.diff" does this, it reduces the 
code size by 7 lines (actually 6, one was a comment), and the result looks 
equivalent to me.

There might be room for futher shortening. For example, I wonder whether

KUrl base = cwd;
base.adjustPath(KUrl::AddTrailingSlash);
m_kurl = new KUrl( base );
m_kurl->addPath( url_copy );

is equivalent to

m_kurl = new KUrl( cwd );
m_kurl->addPath( url_copy );

Note that the statement

base.adjustPath(KUrl::AddTrailingSlash);

appears unnecessary because KUrl::addPath () adds slashes automatically if 
needed according to 
http://api.kde.org/4.x-api/kdelibs-apidocs/kdecore/html/classKUrl.html, but 
maybe I'm overlooking something here?

Regards
Frank
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch-bug-163098-Version2.diff
Type: text/x-diff
Size: 1379 bytes
Desc: not available
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20080906/135dd5bd/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch-bug-163098-Version2-Extended.diff
Type: text/x-diff
Size: 1724 bytes
Desc: not available
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20080906/135dd5bd/attachment-0001.diff>