Bug 285028 (qt48_qurl) - Qt 4.8 QUrl.toLocalFile behavior change, impacts KUrl (and at least knotify)
Summary: Qt 4.8 QUrl.toLocalFile behavior change, impacts KUrl (and at least knotify)
Status: RESOLVED UPSTREAM
Alias: qt48_qurl
Product: kdelibs
Classification: Unmaintained
Component: kdecore (show other bugs)
Version: 4.7
Platform: Fedora RPMs Linux
: NOR normal
Target Milestone: ---
Assignee: kdelibs bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-26 18:11 UTC by Rex Dieter
Modified: 2012-03-31 10:43 UTC (History)
10 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
hack to workaround in kdelibs/knotify (901 bytes, patch)
2011-10-26 19:38 UTC, Rex Dieter
Details
hack to workaround in kde-runtime/knotify (680 bytes, patch)
2011-10-26 19:38 UTC, Rex Dieter
Details
knotify fix (726 bytes, application/octet-stream)
2011-12-28 07:58 UTC, David Faure
Details

Note You need to log in before you can comment on or make changes to this bug.
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.