Review Request: Remove QFSFileEngine useage from Phonon five.

[Jon Severinsson]

> On Oct. 21, 2012, 10:30 a.m., Harald Sitter wrote:
> > Changing the behavior of a function to that extent seems unwise to me, plus it allows even further misuse of the function. One should only ever have local files thrown at that function, hence why I think Phonon 5 should not have that ctor at all but force QUrls. Allowing people to throw more non-local files at it is a bit counter productive.
> > 
> > To address your worries though... :)
> > file:// URLs are valid everywhere in fact I personally would argue that it is the only way to specify a valid local file path.
> > On unix:
> > file:///home/foo/bar.mp3 (note the second /)
> > On windows
> > file://C:/Users/foo/bar.mp3 (possibly \, but I am reasonable certain / works ;))
> > 
> > This is true to such an extent that what we actually do in the backends is check if the url() has a defined protocol and if it does not we simply throw a file:// in front of it.

The constructor already supports non-local file paths and URLs (to local files, to remote files, and to non-file:// protocols), so removing that feature would probably result in more breakage than my changes (which amounts to already broken code being broken in a different way).

Removing the function would certainly be simlpler, but then all calls to it would suddenly turn into calls to MediaSource::MediaSource(QUrl(QString)), silently breaking existing code.
Additionally we would have to add support for qrc:// urls in the MediaSource::MediaSource(const QUrl &url) constructor, or we'd lose one major feature without a replacement.

As for your examples, your third URL is invalid. As written it would refer to the file "foo/bar.mp3" on the smb share "Users" on the host "C:", but "C:" is an invalid hostname. I think you meant "file:///C:/User/foo/bar.mp3" (note the third /), which reffers to the Users/foo/bar.mp3 file on the C: drive on localhost.

Think of your typo as further proof that the "file" URL scheme is easy to get wrong, so if we force it's use, we are just inviting bugs in user code.


> On Oct. 21, 2012, 10:30 a.m., Harald Sitter wrote:
> > phonon/mediasource.cpp, line 58
> > <http://git.reviewboard.kde.org/r/106974/diff/2/?file=91573#file91573line58>
> >
> >     documentation on why path and url would be useful

I thought it self explanatory, we need the ":/" syntax for QFile, and the "qrc://" syntax for QUrl.

The old code would not work for "qrc://" URLs, and would create an invalid "file::/foo" QUrl for a ":/foo" file path (wich don't stop it from working, but provides garbage output to users).


- Jon


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106974/#review20616
-----------------------------------------------------------


On Oct. 21, 2012, 9:59 a.m., Jon Severinsson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106974/
> -----------------------------------------------------------
> 
> (Updated Oct. 21, 2012, 9:59 a.m.)
> 
> 
> Review request for KDE Frameworks and Phonon.
> 
> 
> Description
> -------
> 
> QFSFileEngine has been removed from Qt5, so rip it out of phonon/mediasource.cpp. Any pointers to a propper fix would be welcome.
> 
> 
> Diffs
> -----
> 
>   phonon/mediasource.cpp 9e35094 
> 
> Diff: http://git.reviewboard.kde.org/r/106974/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Jon Severinsson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20121021/3e023fce/attachment.html>