Bug 285028 (qt48_qurl)

Summary: Qt 4.8 QUrl.toLocalFile behavior change, impacts KUrl (and at least knotify)
Product: [Frameworks and Libraries] kdelibs Reporter: Rex Dieter <rdieter>
Component: kdecoreAssignee: kdelibs bugs <kdelibs-bugs>
Status: RESOLVED UPSTREAM    
Severity: normal CC: faure, kevin.kofler, melko, philm, rakuco, scarpino, sebastian.radish, stuffcorpse, sven.burmeister, wstephenson
Priority: NOR    
Version: 4.7   
Target Milestone: ---   
Platform: Fedora RPMs   
OS: Linux   
Latest Commit: Version Fixed In:
Attachments: hack to workaround in kdelibs/knotify
hack to workaround in kde-runtime/knotify
knotify fix

Description Rex Dieter 2011-10-26 18:11:33 UTC
Seems Qt 4.8 QUrl.toLocalFile no longer processes file path strings of the form:
foo.txt
and now returns an empty QString.
(see also upstream bug I filed about this, https://bugreports.qt.nokia.com/browse/QTBUG-22382 ).

In the meantime, it means KUrl behaves the same.  Bad side effects include:
knotification audio events with sources like: 
KDE-Sys-Log-In.ogg
no longer work in knotify4 (kde-runtime) or knotifyconfigactionswidget(kdelibs)

I've got a simple fix for the former, but the latter uses  KUrlRequester class all over, so is less trivial.


grepping kdelibs/4.7 branch for toLocalfile I get 195 hits, which scares the bejesus out of me.
Comment 1 Rex Dieter 2011-10-26 19:38:10 UTC
Created attachment 64904 [details]
hack to workaround in kdelibs/knotify
Comment 2 Rex Dieter 2011-10-26 19:38:55 UTC
Created attachment 64905 [details]
hack to workaround in kde-runtime/knotify
Comment 3 Rex Dieter 2011-10-26 20:03:52 UTC
And, fwiw, I think this is the upstream Qt commit that introduced the change,
http://qt.gitorious.org/qt/qt/commit/a2f797b52c4274a62a7cf1f0939aca1429afe211
Comment 4 Kevin Kofler 2011-10-26 20:55:08 UTC
Easiest fix (reverting the logic change only without reverting the API addition
and the cleanups):

In the (new) QUrl::isLocalFile function, before:

if (d->scheme.compare(QLatin1String("file"), Qt::CaseInsensitive) != 0)

add the following:

// Treat URLs with no scheme as local for backwards compatibility
if (d->scheme.isEmpty())
    return true;

(Not tested yet, but looking at the offending commit, it should fix this.)
Comment 5 Kevin Kofler 2011-10-27 16:22:30 UTC
It turns out that the behavior of the new QUrl::isLocalFile() is consistent with the existing KUrl::isLocalFile() and Q3Url::isLocalFile() functions, so an alternative easy fix:

In the toLocalFile function, change:

if (!isLocalFile())

to:

// Treat URLs with no scheme as local for backwards compatibility
if (!d->scheme.isEmpty() && !isLocalFile())

(Again, not tested yet, but should work based on my proofreading, and fix this regression in the least invasive way possible.)
Comment 6 Kevin Kofler 2011-10-27 19:09:34 UTC
Actually, please use:

// Treat URLs with no scheme as local for backwards compatibility
if (!isLocalFile() && !d->scheme.isEmpty())

in this order, because isLocalFile() also ensures the URL is parsed (as written in the comment above the line we're changing).
Comment 7 Kevin Kofler 2011-10-27 21:50:56 UTC
Actually, please use:

// Treat URLs with no scheme as local for backwards compatibility
if (!isLocalFile() && (!d || !d->scheme.isEmpty()))

Some code (e.g. in Digikam) is calling isLocalFile() on a null QUrl(), without the added !d check, it crashes trying to check !d->scheme.isEmpty().

(I use !d || rather than d && because if d is NULL, we really SHOULD return a QString() (what else?).)
Comment 8 David Faure 2011-12-28 07:52:58 UTC
KUrl was never meant to be used with relative paths (filenames) like this.

If in knotify, soundURL is always just a filename, then it should be a QString rather than a KUrl. If it's a filename or a url, then the code could use some QUrl::isRelative() check.

But it seems it's always just a filename, right?
Comment 9 David Faure 2011-12-28 07:58:18 UTC
Created attachment 67180 [details]
knotify fix

OK in fact the check for isRelative is already there, it's just wrongly handled. How about this patch, then?
Comment 10 Phil Miller 2011-12-29 21:40:00 UTC
David, your fix didn't fix it completely. Seems it only happens if you have QT 4.8.0 or higher installed. On a plain QT 4.7.4 install this bug don't happen at all. You can now play sounds in systemsettings/notifications but you don't hear them when the actual notification comes. Only as text-notification as usual.
Seems we have to dig a little deeper into this situation. As you already posted on kde-release team list (http://mail.kde.org/pipermail/release-team/2011-December/005420.html) patching QT with "qt-everywhere-opensource-src-4.8.0-QUrl_toLocalFile.patch" should be a no-go for us.

What else can we try to fix this issue with knotify and qt 4.8?
Comment 11 Rex Dieter 2011-12-29 21:44:08 UTC
Phil, see comment #2, kde-runtime/knotify needs some love too it seems.
Comment 12 Phil Miller 2011-12-30 10:38:06 UTC
Yes Rex, kde-runtime/knotify is the one which plays the sound actually. So something has to be changed in this code section:

    // get file name
	if ( KUrl::isRelativeUrl(soundFile) )
	{
		QString search = QString("%1/sounds/%2").arg(config->appname).arg(soundFile);
		search = KGlobal::mainComponent().dirs()->findResource("data", search);
		if ( search.isEmpty() )
			soundFile = KStandardDirs::locate( "sound", soundFile );
		else
			soundFile = search;
	}
	if ( soundFile.isEmpty() )
	{
		finish( eventId );
		return;
	}

Adding your hack plays it, but I think it is not the solution David has in mind or?
Comment 13 S. Burmeister 2012-03-31 09:44:42 UTC
So was this fixed (upstream) or did it become a wontfix?
Comment 14 Kevin Kofler 2012-03-31 10:43:25 UTC
The behavior got reverted upstream in Qt 4.8.1. Qt 5 will (re)introduce the change though.