<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/127827/">https://git.reviewboard.kde.org/r/127827/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 11th, 2016, 12:24 a.m. UTC, <b>Aleix Pol Gonzalez</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;">Doesn't that change the behavior for the regular non-native use-case?</p></pre>
</blockquote>
<p>On May 11th, 2016, 7:06 a.m. UTC, <b>Sven Brauch</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;">What is the "regular non native-use case"?
It changes behaviour, but right now the behaviour is wrong, so that is a good thing ;)</p></pre>
</blockquote>
<p>On May 11th, 2016, 9:45 a.m. UTC, <b>René J.V. 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;">Maybe Aleix meant non-native = not pure QFileDialog, referring to the fact that under X11 (and Wayland?) KDevelop uses KDE file dialogs rather than QFileDialogs?</p></pre>
</blockquote>
<p>On May 11th, 2016, 8:27 p.m. UTC, <b>Sven Brauch</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;">No, instantiating QFileDialog under Plasma uses the KDE file dialog because of the platform plugin which is being loaded. The code before doing this differentitation again made no sense whatsoever in the first place.</p></pre>
</blockquote>
<p>On May 11th, 2016, 11:22 p.m. UTC, <b>Aleix Pol Gonzalez</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;">No, we use the KFileWidget directly.</p></pre>
</blockquote>
<p>On May 12th, 2016, 6:54 a.m. UTC, <b>Sven Brauch</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;">But is that worth keeping two code paths?</p></pre>
</blockquote>
<p>On May 12th, 2016, 8:08 a.m. UTC, <b>René J.V. 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;">FWIW, one thing I've always appreciated is the fact that the directory selection dialog you get when configuring a cmake project (selecting the build dir) is native on OS X. I vastly prefer that one over KDE's version which is (pardon, seems) inspired by the one from MS Windows.</p></pre>
</blockquote>
<p>On May 12th, 2016, 8:19 a.m. UTC, <b>Kevin Funk</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;">@scummos: I'm with Aleix here. Please restore the behavior we had prior to the change (use KFileWidget) for regular KDE users (or those who want to have KDE-like file dialogs). Worth having the two code paths here, IMO, yes.</p></pre>
</blockquote>
<p>On May 12th, 2016, 8:41 a.m. UTC, <b>René J.V. 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;">You mean the behaviour which prompted me to send a message to kdevelop-devel?</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;">I just noticed that KDevPlatform now falls back to using QFileDialog when KDE_FULL_SESSION isn't set.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Not that this is completely devoid of justification, but isn't that what happens automatically anyway when the framework integration plugin isn't installed, or the KDE platform theme isn't being used (for instance because KDE_FULL_SESSION isn't set, on Linux)?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Having to set <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KDE_FULL_SESSION</code> to get KDE file dialogs is a bit problematic on non-Linux platforms, IMHO. It's semantically wrong (as it implies a Plasma session) and can have unwanted side-effects.
I'd vote for a behaviour where <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KDE_FULL_SESSION</code> isn't checked on OS X, MSWin etc, but assumed to be true ... and then let it depend on how KF5 is installed and configured what file dialog is used.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Actually seems a good principle to me. Applications should only rarely be concerned about the kind of dialog they present but rather leave that to the session or OS configuration (or else provide an option, like OpenOffice/LibreOffice does).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is the patch I've had to add to my port for KDevPlatform, to get back the "old" behaviour:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #000080; font-weight: bold">diff --git shell/openprojectdialog.cpp shell/openprojectdialog.cpp</span>
<span style="color: #000080; font-weight: bold">index 9ccca43..34eefad 100644</span>
<span style="color: #A00000">--- shell/openprojectdialog.cpp</span>
<span style="color: #00A000">+++ shell/openprojectdialog.cpp</span>
<span style="color: #800080; font-weight: bold">@@ -75,7 +75,7 @@ OpenProjectDialog::OpenProjectDialog( bool fetch, const QUrl& startUrl, QWidget*</span>
{
resize(QSize(700, 500));
<span style="color: #A00000">- const bool useKdeFileDialog = qEnvironmentVariableIsSet("KDE_FULL_SESSION");</span>
<span style="color: #00A000">+ const bool useKdeFileDialog = true; /*qEnvironmentVariableIsSet("KDE_FULL_SESSION")*/</span>
QStringList filters, allEntry;
QString filterFormat = useKdeFileDialog
? QStringLiteral("%1|%2 (%1)")
<span style="color: #000080; font-weight: bold">diff --git shell/documentcontroller.cpp shell/documentcontroller.cpp</span>
<span style="color: #000080; font-weight: bold">index 0176c2e..5b44837 100644</span>
<span style="color: #A00000">--- shell/documentcontroller.cpp</span>
<span style="color: #00A000">+++ shell/documentcontroller.cpp</span>
<span style="color: #800080; font-weight: bold">@@ -138,7 +138,7 @@ struct DocumentControllerPrivate</span>
auto parent = Core::self()->uiControllerInternal()->defaultMainWindow();
// use special dialogs in a KDE session, native dialogs elsewhere
<span style="color: #A00000">- if (qEnvironmentVariableIsSet("KDE_FULL_SESSION")) {</span>
<span style="color: #00A000">+ /*if (qEnvironmentVariableIsSet("KDE_FULL_SESSION"))*/ {</span>
const auto result = KEncodingFileDialog::getOpenUrlsAndEncoding(QString(), dir,
filter, parent, caption);
return {result.URLs, result.encoding};
</pre></div>
</p></pre>
</blockquote>
<p>On May 12th, 2016, 8:57 a.m. UTC, <b>Kevin Funk</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;">Well, either way: We need to have <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">some</em> input to know whether we should show KDE file dialogs or not. Currently we use the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">KDE_FULL_SESSION</code> env var (which is suboptimal, I agree), but it could be <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">anything</em>. It should be a runtime option nevertheless. Not a compile-time one (as you seem to want). User preferences vary.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Note: This is somewhat unrelated to this patch, please discuss that on the mailing list. Let's keep this one focussed on the issue we're trying to fix.</p></pre>
</blockquote>
<p>On May 12th, 2016, 8:58 p.m. UTC, <b>Sven Brauch</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;">But this is a problem common to all applications. It is the same issue in every other KDE application I run outside plasma. Yes, we could introduce our own workaround, but is that really a good plan?</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">New suggestion: I could replace the file dialog wizard page by a wizard page with a KUrlRequester. I think that's the closest we can get to the current behaviour while keeping functionality ...
Opinions?</p></pre>
<br />
<p>- Sven</p>
<br />
<p>On May 3rd, 2016, 10:32 p.m. UTC, Sven Brauch 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 KDevelop and Kevin Funk.</div>
<div>By Sven Brauch.</div>
<p style="color: grey;"><i>Updated May 3, 2016, 10:32 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevplatform
</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;">The native dialog has two significant restrictions: it cannot be embedded, and it cannot be told to accept both a directory <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">or</em> a file at once. The previous change to the open project dialog broke the (important) option to open a directory as a project. This fixes that through introduction of an extra step: you select the method you want to open your project with first. This has the advantage of making it more clear to the user what the options are; many users are still not aware you can simply tell KDevelop to open a folder as a project.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm well aware this is far from an optimal solution, but right now it's just broken and this is certainly an improvement over the current situation. Better ideas welcome.</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;">can open from CMakeLists.txt, from foo.kdev5 or from a folder; also fetch works again (before simply random stuff happened)</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>shell/CMakeLists.txt <span style="color: grey">(83d4db0)</span></li>
<li>shell/openprojectdialog.h <span style="color: grey">(d39ff8e)</span></li>
<li>shell/openprojectdialog.cpp <span style="color: grey">(9ccca43)</span></li>
<li>shell/openprojectdialog.ui <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/openprojectpage.h <span style="color: grey">(1e0ff60)</span></li>
<li>shell/openprojectpage.cpp <span style="color: grey">(42d836f)</span></li>
<li>shell/projectsourcepage.h <span style="color: grey">(a45ee19)</span></li>
<li>shell/projectsourcepage.cpp <span style="color: grey">(43ab6e9)</span></li>
<li>shell/projectsourcepage.ui <span style="color: grey">(79699aa)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127827/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/05/03/7b6cdfc2-c4d7-4394-9a39-2ccc923f28fa__Screenshot_20160504_001810.png">the added dialog</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>