Bug 285028 (qt48_qurl)

Created attachment 64904 [details]
hack to workaround in kdelibs/knotify

Created attachment 64905 [details]
hack to workaround in kde-runtime/knotify

And, fwiw, I think this is the upstream Qt commit that introduced the change,
http://qt.gitorious.org/qt/qt/commit/a2f797b52c4274a62a7cf1f0939aca1429afe211

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.)

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.)

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).

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?).)

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?

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?

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?

Phil, see comment #2, kde-runtime/knotify needs some love too it seems.

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?

So was this fixed (upstream) or did it become a wontfix?

The behavior got reverted upstream in Qt 4.8.1. Qt 5 will (re)introduce the change though.