<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/113302/">http://git.reviewboard.kde.org/r/113302/</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 20th, 2013, 9:32 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;">OK, I'm just wondering if it still makes sense to make it static-methods-only.
It can still very well work the other way, which can allow extra flexibility if needed (e.g. for kate to add another widget into it, or use it non-modal, etc.). What's the reason for making it less flexible? Just the fact that nobody uses the constructor right now? (I assume you checked that?).
Kévin?</pre>
</blockquote>
<p>On October 21st, 2013, 10:52 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;">Initially I think I proposed even a namespace. :-)
But seeing the current solution I think you're right, we could indeed allow instances to be created.</pre>
</blockquote>
<p>On October 21st, 2013, 11:24 a.m. UTC, <b>Teo Mrnjavac</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;">Ok, but that would add some more duplication over what KFileDialog already does. Afaict, the only non-static usage of KEncodingFileDialog was in KMail's messagecomposer. I had checked with kdepim people a while ago, and they have no problem with KEncodingFileDialog becoming static only in KF5.</pre>
</blockquote>
<p>On October 21st, 2013, 11:52 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;">I'm not sure which kind of duplication you have in mind, but surely just making the ctor/dtor pair public and adding a result() public method would be enough (said method you could use in your static methods BTW reducing some of the code duplication between them).</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;">OK, discussing with Teo on IRC I think he's right. We could make it instanciable with what I propose... but then if you want a proper API you need accessors for a few properties, and signals when the user interact with the dialog etc. We'd basically duplicate quite some KFileDialog code... which was API not used in the case of KEncodingFileDialog. So he's right, let's keep it based on static alone.</pre>
<br />
<p>- Kevin</p>
<br />
<p>On October 19th, 2013, 11:30 a.m. UTC, Teo Mrnjavac 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, David Faure and Kevin Ottens.</div>
<div>By Teo Mrnjavac.</div>
<p style="color: grey;"><i>Updated Oct. 19, 2013, 11:30 a.m.</i></p>
<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;">This makes KEncodingFileDialog static, moves it to staging/kio and moves some stuff from KFileDialog to KFileWidget to reduce duplication.</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;">Seems to work fine in a test app.</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>kio/CMakeLists.txt <span style="color: grey">(ac80e393c877280dd8033ec13e8e772afea6d2f9)</span></li>
<li>kio/kfile/kencodingfiledialog.h <span style="color: grey">(abb939abeb000126005f1a1a9c6fd50b8bd322bd)</span></li>
<li>kio/kfile/kencodingfiledialog.cpp <span style="color: grey">(d55d908473aae2859838d88fd776cc65cecf3317)</span></li>
<li>kio/kfile/kfiledialog.cpp <span style="color: grey">(32eb98a4490a31c5ed51150ca3cb7af5375dc67e)</span></li>
<li>staging/kio/src/filewidgets/CMakeLists.txt <span style="color: grey">(e8d8c2edf11ad6352e13eb6e7436f828a4a55e3a)</span></li>
<li>staging/kio/src/filewidgets/kencodingfiledialog.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kio/src/filewidgets/kfilewidget.h <span style="color: grey">(f7b162ab39b997274b21f9eff0c6374545e0a9ad)</span></li>
<li>staging/kio/src/filewidgets/kfilewidget.cpp <span style="color: grey">(824ac563f3f8c463426f0a45e792f107ac402a40)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/113302/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>