<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 Juli 12th, 2014, 2:11 nachm. UTC, <b>Aleix Pol Gonzalez</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="https://git.reviewboard.kde.org/r/119243/diff/1/?file=289740#file289740line316" style="color: black; font-weight: bold; text-decoration: underline;">kio/kfile/kfiledialog.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">KFileDialog::KFileDialog( const KUrl& startDir, const QString& filter,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">316</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">d</span><span class="o">-></span><span class="n">w</span><span class="o">-></span><span class="n">setCustomWidget</span><span class="p">(</span><span class="n">QString</span><span class="p">(),</span> <span class="n">customWidget</span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">316</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1"><span class="hl">//</span>d->w->setCustomWidget(QString(), customWidget);</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't know why you did that, but it doesn't look good.</p></pre>
</blockquote>
<p>On Juli 12th, 2014, 4:56 nachm. UTC, <b>Marko Käning</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;">Actually, when submitting this RR I was also stumbling over this commented out line and wondered why...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I am afraid we've to wait for René to figure this out.</p></pre>
</blockquote>
<p>On Juli 14th, 2014, 3:06 vorm. UTC, <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;">If we are going to use Review Board, can we get the kde-mac list added as a group? How do you do that?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That would let René, Boudewijn and Nicolas listen in on this stuff, FWIW.</p></pre>
</blockquote>
<p>On Juli 14th, 2014, 3:17 vorm. UTC, <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;">Actually I do not think Review Board is worth much. It is extra work, hard to use and, in my experience, rarely results in anything much more than white-space adjustments - as you can see from the unanswered questions, issues and comments above, by Mark Gaiser, Marko and me. Actually I answered Marko's issue, but I would far rather be doing so, more quickly and directly, on kde-mac or BKO, if none of the kdelibs maintainers have anything useful to add. Nevertheless I will have one more try at reviewing this patch, now that I have tested it on Apple OS X with a few different file-open and save situations.</p></pre>
</blockquote>
<p>On Juli 14th, 2014, 6:13 vorm. UTC, <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;">You'd need to request addition of the group to groups (just a formality to add the entry, ask sysadmin æt kde döt org), otherwise you've to add rene (rjvbb) directly.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The file dialog apparently requires the native fallback in case the start dir is not local.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
The string UnifiedTitleAndToolBarOnMac is only used here: http://lxr.kde.org/search?_filestring=&_string=UnifiedTitleAndToolBarOnMac <br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
The key is "Native" in kdeglobals (~/.kde/share/config/kdeglobals), [KFileDialog Settings] group (just looked up the code on the static flag, see http://api.kde.org/4.x-api/kdelibs-apidocs/kio/html/kfiledialog_8cpp_source.html from line 206)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">A very specific problem about this review is that the submitter is not the author.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The worth of reviewboard is (at least) to elimintate bad commits (see "commented d->w->setCustomWidget(QString(), customWidget);" - whatever the problem is, this solution not only adds cruft and lacks a comment but most certainly introduces a bug, since it basically kills the API feature)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">... oh, and this review was opened on the FIFA cup finals weekend - eg. i still won't be near a usable workstation before tonight. And I think it's amazing™ that i can already use a keyboard again =)</p></pre>
</blockquote>
<p>On Juli 14th, 2014, 9:30 vorm. UTC, <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;">Thanks, I have applied to add kde-mac to the groups.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">On your advice, I used the Native key (=true) to do some tests. It was not appearing in kdeglobals anywhere on Apple OS X because the default (false?) was always set and KConfig entries with default values are not shown. I wish KConfig would not do that.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Actuallly the submitter to the submitter is not the author either. René acquired this patch from somewhere on KDE Forums IIRC and originally floated it on BKO. I appreciate your point about bad commits, but we (on KDE-Mac) are not complete newbies. Our usual practice would be to test such a change "downstream", individually at first and then, if it looks good, by releasing it in MacPorts. Then arises the question of how and where it should appear as a change in KDE portability code - in KDE 4, in KF 5, etc. My solution has been to set up https://projects.kde.org/projects/playground/base/osx-patches/repository/revisions/master/entry/README so that KDE and MacPorts people can decide for themselves when and in what version to incorporate such patches.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As to the World Cup, well Marko submitted this review request and he is German too... :-) Never mind. Well done Germany!!! I am very happy to see you win!!!</p></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;"><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 wish KConfig would not do that.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I assume it's rather because the key has no GUI item and is (therefore) not considered in kdeglobals.kcfg - it's a "secret" to the library code.</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;">we (on KDE-Mac) are not complete newbies</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That's actually not the point. By formalizing the process you more or less guarantee that the patch had at least a surface level cross-check. (That's not foolproof either, but better than nothing)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">"We just somehow do it, we're pros" works great for small scale groups, but not for things like KDE with hundreds of modules and thousands of occasional commiters.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You do not need the policy to cover the individual idiot, but to handle the crowd (and the idiots inside ;-)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Eg. I guess there're speed limits down under?<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Ever thought why they are there? Because <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">you</em> would be too stupid to pick an appropriate speed?<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Nahhh... It's to coordinate traffic. I guess there're no explicit limits in the outback, where there is about one car every once a week?</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;">Then arises the question of how and where it should appear as a change in KDE portability code</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Bugfixes can go into kdelibs/workspace 4, new features into KF5 / "Plasma Next". Other modules/applications are not frozen.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">However looking randomly at some patches, they seem to cover actual bugs which just emerge on OSX.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Eg. KMyMoney should not crash on the raster graphicssystem, period. It's the only available in Qt5, so this will have to be fixed for KDE5 anyway. <br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Other things like the libdispatch issue on kcrash could require a "proper" fix (isolate libdispatch FDs and exclude them from closing. Actually, unconditionally closing random FDs might be a more gereral issue and just worked by luck so far)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">-> Are there open bug reports for those issues?</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Juli 12th, 2014, 2:11 nachm. 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;">Other than that it looks ok. Please update the patch though.</p></pre>
</blockquote>
<p>On Juli 14th, 2014, 8:21 vorm. UTC, <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;">I have tested the patch on Apple OS X in my kdesrc-build environment for KDE 4.13 branch. Before I did so, I removed the comment from line 316 and also the pair of braces from the line above it. Here are my results, using 3 apps for which I know the source code (I am the curent maintainer).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">MAC - Palapeli, Create Puzzle - m_imageSelector(new KUrlRequester) [1]<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
KDE - Palapeli, Import Puzzle(s) - KFileDialog::getOpenFileNames(KUrl("kfiledialog:///palapeli-import"), filter);<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
MAC - Palapeli, Export Puzzle(s) - KFileDialog::getSaveFileName(KUrl(startLoc), filter) [2]</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">MAC - Kubrick, Load Puzzle - KFileDialog::getOpenFileName (KUrl(), "<em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">.kbk", myParent, i18n("Load Puzzle"))<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
MAC - Kubrick, Save Puzzle - KFileDialog::getSaveFileName (KUrl(), "</em>.kbk", myParent, i18n("Save Puzzle"))</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">KDE - KSudoku, Load - KFileDialog::getOpenUrl(KUrl("kfiledialog:///ksudoku"), QString(), this, i18n("Open Location"))<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
KDE - KSudoku, Save - KFileDialog::getSaveUrl(KUrl("kfiledialog:///ksudoku"))</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">[1] KUrlRequester is a mini-dialog with a text-edit box and a button. The button brings up a dynamic KFileDialog window in native Mac style.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">[2] QString startLoc = QString::fromLatin1("kfiledialog:///palapeli-export/%1.puzzle").arg(cmp->metadata.name); but startLoc does not appear in the dialog's location box as a default name.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In each case, you have the dialog style that appeared (MAC or KDE), followed by the program and action, the invocation of KFileDialog used and maybe a note.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Without the patch, using my MacPorts installation as at KDE 4.12.5, <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">all</em> the above cases gave the KDE dialog.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">When I inserted Native=true in ~/Library/Preferences/KDE/share/config/kdeglobals (which is where app preferences are kept in Apple OS X and MacPorts), still without any patch, there were some changes in "Palapeli, Create", "Kubrick, Load" and "Kubrick, Save", but all the others stayed at KDE style. "Palapeli, Create" with just Native=true (no patch) displayed a dialog that was empty except for buttons Cancel and OK. Kubrick displayed a MAC dialog in both cases.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This post by Boudewijn Rempt is also relevant to this somewhat confusing situation and shows some more test results on KFileDialog on various platforms --- http://mail.kde.org/pipermail/kde-mac/2014-July/001211.html --- that thread is where René originally raised this topic.</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;">The "problem" with the code is (likely) that "kfiledialog://" is not considered as "local" protocol, so actually the only thing that confuses me is that the patch turns "Palapeli, Export Puzzle" into a native dialog while the "Native" key does not.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I assume this could be fixed (for KF5) by resolving "kfiledialog:///foobar" first and then passing the result to the native dialog (if a local protocol) - as is done for ::getSaveFileName, but not ::getSaveUrl or others.<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
The code in question is really messy in this context ;-)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Defaulting "Native" as true on OSX (and windows?) should then be sufficient - I assume the conflicting behavior on ::getSaveFileName is due to the static flag, polluted by ::getOpenFileNames().</p></pre>
<br />
<p>- Thomas</p>
<br />
<p>On Juli 14th, 2014, 6:15 nachm. UTC, 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 Juli 14, 2014, 6:15 nachm.</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>