<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/106974/">http://git.reviewboard.kde.org/r/106974/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 21st, 2012, 10:30 a.m., <b>Harald Sitter</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 21st, 2012, 10:30 a.m., <b>Harald Sitter</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/106974/diff/2/?file=91573#file91573line58" style="color: black; font-weight: bold; text-decoration: underline;">phonon/mediasource.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">MediaSource::MediaSource()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">51</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QString</span> <span class="n">path</span><span class="p">(</span><span class="n">filename</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">documentation on why path and url would be useful</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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).</pre>
<br />
<p>- Jon</p>
<br />
<p>On October 21st, 2012, 9:59 a.m., Jon Severinsson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks and Phonon.</div>
<div>By Jon Severinsson.</div>
<p style="color: grey;"><i>Updated Oct. 21, 2012, 9:59 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">QFSFileEngine has been removed from Qt5, so rip it out of phonon/mediasource.cpp. Any pointers to a propper fix would be welcome.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>phonon/mediasource.cpp <span style="color: grey">(9e35094)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/106974/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>