<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/119243/">https://git.reviewboard.kde.org/r/119243/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 21st, 2014, 7:16 a.m. CEST, <b>Ian Wadham</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">So where are we leaving this? Any conclusions? Any solutions or further patches?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Should we report the inconsistent handling of Native file dialogs as a bug on bugs.kde.org (without a patch this time)?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If so, against what "product" should we report it (i.e. where a KDE developer will be listening)?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I agree with Thomas that "kfiledialog://" should be evaluated before deciding whether it is a local file.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I notice also, that in the "condition" that keeps occurring ((!startDir.isValid() || startDir.isLocalFile()), the isValid() method seems to be inherited from QUrl class and maybe isLocalFile() is too.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It seems odd that a possible condition for invoking QFileDialog methods should be that the QUrl is invalid. Perhaps there used to be an isValid() in KUrl with a different meaning.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It's all very confusing. Who can sort it out?</p></pre>
</blockquote>
<p>On July 21st, 2014, 1:52 p.m. CEST, <b>Thomas Lübking</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">dfaure (assined to the kio framework)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">kfiledialog.cpp would require a general cleanup, but it's in kde4support now, so the predominant question is whether it makes any sense to spend time on this at all.</p></pre>
</blockquote>
<p>On July 22nd, 2014, 2:52 a.m. CEST, <b>Ian Wadham</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The following change by dfaure (David Faure) says it all. KFileDialog is now deprecated in favour of QFileDialog. <br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
https://projects.kde.org/projects/frameworks/kdelibs4support/repository/diff/src/kio/kfiledialog.h?rev=977151f9747bc3b7db6d68daafcee41e11516e31&rev_to=cb01fc274d4c0e907d27a1cd7fc80bbca9c1dbcd</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">When/if all uses of KFileDialog in KDE apps are replaced by uses of QFileDialog, all their file dialogs should become Native on all platforms.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Unfortunately, the above change has not been entered into https://community.kde.org/Frameworks/Porting_Notes so I do not know how KDE apps authors like me would normally become aware of it. I did check the Porting Notes carefully before entering into this discussion and I have just checked them again. If they had been up-to-date on this issue, that could have saved us all a lot of time on this review!</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">With the Randa sprint coming up soon (and hopefully lots of KDE apps porting being done), I would suggest that the Porting Notes get re-read in their entirety by the authors and updated soon, to catch this and any other missing bits, if that is not already happening.</p></pre>
</blockquote>
<p>On July 27th, 2014, 10:22 a.m. CEST, <b>RJVB Bertin</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I came across another application that could be an interesting testbed; Skrooge. Despite the patches in this RR it opens a KDE style dialog. Additionally, it looses the whole (mac style) menubar, definitely, after opening a dialog but also after alt-tabbing to another application. This is probably unrelated but then again maybe not completely.</p></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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">the predominant question is whether it makes any sense to spend time on this at all.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That would depend on what's in the future for KDE on OS X / under MacPorts, specifically how long we're going to be stuck on 4.12 or 4.1x releases.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As far as I'm concerned, we could try to reach some concensus on how to iron out any remaining kinks in the patches, so I can create a MacPorts ticket to create a kdelibs4 specific "variant".</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In that context, someone wrote</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Other than that it looks ok. Please update the patch though.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">but there are so many comments and remarks by now that I have no idea what should be changed. Remember that I'm not a KDE developer and all I did in the 1st place was update and "package" a series of modifications someone made in 2011 without any form of documentation at all.</p></pre>
<br />
<p>- RJVB</p>
<br />
<p>On July 14th, 2014, 8:15 p.m. CEST, Marko Käning wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Software on Mac OS X, kdelibs, Christoph Feck, Ian Wadham, and RJVB Bertin.</div>
<div>By Marko Käning.</div>
<p style="color: grey;"><i>Updated July 14, 2014, 8:15 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=337356">337356</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This bundles both patches submitted by René J.V. Bertin in the associated issue</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">See issue for more info from René.</p>
<hr style="text-rendering: inherit;margin: 0;padding: 0;white-space: normal;border: 1px solid #ddd;line-height: inherit;" />
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I myself haven't yet tested this. Will report back as soon as I got to it.</p></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>kdeui/widgets/kmainwindow.cpp <span style="color: grey">(85beaccdb6f123d2996b8c2b5e38435265c63ff8)</span></li>
<li>kio/kfile/kfiledialog.h <span style="color: grey">(2b11796897ff095279e7c254a383a9e8e323ea0f)</span></li>
<li>kio/kfile/kfiledialog.cpp <span style="color: grey">(4005ba304a18b59572cc1aece3fcd122ce05fda9)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119243/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>