<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/111184/">http://git.reviewboard.kde.org/r/111184/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 25th, 2013, 6:46 a.m. UTC, <b>Kevin Ottens</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/111184/diff/2/?file=165624#file165624line483" style="color: black; font-weight: bold; text-decoration: underline;">kio/kfile/kurlrequester.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; ">void KUrlRequester::KUrlRequesterPrivate::_k_slotUpdateUrl()</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">483</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="n">dropEv</span><span class="o">-></span><span class="n">mimeData</span><span class="p">()</span><span class="o">-></span><span class="n">hasUrls</span><span class="p">())</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;">Why don't you use KUrlMimeData like the original code in KLineEdit? It'll be more robust AFAICT.</pre>
</blockquote>
<p>On June 25th, 2013, 10:46 a.m. UTC, <b>Albert Vaca Cintora</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;">I'm using hasUrls() instead because KUrlMimeData returns a list of urls, which is more expensive than what Qt does (a string comparison).
Also, if our way of checking URLs is better than the way Qt uses, we should contribute it to Qt instead of avoid using their methods (we are going to rely a lot more on Qt since KF5, so we have to trust what Qt does).</pre>
</blockquote>
<p>On June 26th, 2013, 7:36 a.m. UTC, <b>Kevin Ottens</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;">Well, KUrlMimeData has some specific handling for KIO, which is a big no-no for Qt. That's more what I had in mind... we don't need the metadata part in that context though, so it's likely a moot point.
That said we look for the application/x-kde4-urilist mimetype too (again something we can't really push in Qt). David, any reason why we have this extra mimetype? Doesn't really makes sense to me.</pre>
</blockquote>
<p>On June 26th, 2013, 8:16 a.m. UTC, <b>David Faure</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;">KUrlMimeData has some specific handling for KIO, and not just metadata, but also shipping two lists of URLs: KIO urls and most-local urls. For instance desktop:/foo and file://home/dfaure/Desktop/foo, the first one being aimed at KIO-enabled apps and the second one at non-KIO-enabled apps.
The code here simply checks for "there are urls", and lets QLineEdit do the extraction.
This means that the most-local urls are going to be pasted, not the KIO ones.
But that's not really specific to kurlrequester nor the clear-before-insert feature... (e.g. konqueror's location bar would have the same issue)
WAIT ..... who determined that only KUrlRequester used the "url drop" feature of KLineEdit?
That feature is ON by default, so looking for calls to setUrlDropsEnabled(true) is definitely wrong.
Konqueror's location bar definitely uses that feature too. I suspect the location bar in dolphin and the one in kfiledialog do as well, at least.
It seems to me that putting the code into KUrlRequester is just wrong. It should be an event filter that can be plugged onto any QLineEdit. And then it can take care of both: clear before insert, and using KUrlMimeData to do the URL extraction.</pre>
</blockquote>
<p>On June 26th, 2013, 9:36 a.m. UTC, <b>Kevin Ottens</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;">Hmmm... You're right, as I mentionned earlier that was my only concern that url drops were enabled by default... But somehow I assumed that most of our URL bars were KUrlRequester. Dunno what happened in my brain that day as obviously it's not the case. They're either KComboBox or KLineEdit.
OK... so if we want to make the url drops feature more reusable we should make that feature an event filter in its own class as I initially proposed.
But (and that's a strong but), I now realize that we never pushed toward KUrlRequester to use QLineEdit or QComboBox. It can keep using KLineEdit or KComboBox just fine. They're not getting deprecated, they'll be in KCompletion (and the future KioWidgets will be able to use KCompletion just fine IMO).
It makes me think this patch should in fact be dropped.</pre>
</blockquote>
<p>On June 30th, 2013, 10:06 p.m. UTC, <b>Albert Vaca Cintora</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;">I see your point and agree with you. However I don't think that every other class using this feature can keep using KLineEdit as before (some will need to be ported to QLineEdit even if I was wrong and this is not the case of KUrlRequester) and I don't think this feature is generic enough to send a patch to Qt. I could then move it to a separate class (usable from wherever we need) as Ervin suggested initially, but... do you feel it necessary or can we just drop this feature from the apps we migrate to QLineEdit?
Since you have a more comprehensive perspective of the project as I have, if you think we should keep it I will write the patch to move it to an independent class.</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;">Right, it's kind of case by case basis indeed... Some might want just a QLineEdit with no completion and still get this drop behavior for URLs. So yes, please make a new patch to put that as a separate class usable on top of anything having a text property. Thanks in advance!</pre>
<br />
<p>- Kevin</p>
<br />
<p>On June 25th, 2013, 10:46 a.m. UTC, Albert Vaca Cintora wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By Albert Vaca Cintora.</div>
<p style="color: grey;"><i>Updated June 25, 2013, 10:46 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;">KLineEdit and KComboBox had the property enableUrlDrops that was added to use it in KUrlRequester. Since KUrlRequester is part of KIO, it can not use Kde4Support and should be ported to use QLineEdit and QComboBox. As I said on my email to the frameworks list, it was easier to implement this directly in KUrlRequester than patching Qt, even though it was originally planned as a Qt task. It was my intention to also remove that property from KLineEdit and KComboBox, but finally I didn't include it in the patch (see Ervin's comment in the mailing list).
In the patch I have also added a TODO because I think it would be good to detect any url without needing the mimetype to be set, but I have not done it yet because it should be discussed first.
(Reviewed by aacid and apol)</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>KDE5PORTING.html <span style="color: grey">(ba67bdc)</span></li>
<li>kdeui/widgets/kcombobox.h <span style="color: grey">(ccb019d)</span></li>
<li>kdeui/widgets/klineedit.h <span style="color: grey">(7ac22f6)</span></li>
<li>kio/kfile/kurlrequester.cpp <span style="color: grey">(fa6b234)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111184/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>