<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="https://git.reviewboard.kde.org/r/114988/">https://git.reviewboard.kde.org/r/114988/</a>
</td>
</tr>
</table>
<br />
<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, then it breaks lowercase headers, which have been <knewstuff3/entry.h> at least since 2009.
Looking at kdelibs4:
install_manifest.txt:/d/kde/inst/kde4.12/include/knewstuff3/knewstuffaction.h
install_manifest.txt:/d/kde/inst/kde4.12/include/knewstuff3/knewstuffbutton.h
install_manifest.txt:/d/kde/inst/kde4.12/include/KDE/KNS3/KNewStuffAction
install_manifest.txt:/d/kde/inst/kde4.12/include/KDE/KNS3/KNewStuffButton
(and no forwarding headers for DownloadDialog, DownloadWidget, DownloadManager, UploadDialog or Entry....)
This is inconsistent indeed. ecm_generate_prefix assumes that the prefix for camelcase and for lowercase headers is the same, apart from the case.
Oh! For sure there's a bug, I meant PREFIX KNewStuff3, rather than knewstuff3. This needs to be fixed.
I think this is more consistent than this patch, which will lead to kns3/entry.h etc. This would be a much bigger SIC than changing the prefix for camelcase headers, since KNewStuffButton is now Button anyway, and KNewStuffAction... hmm I didn't generate a forwarding header for it since it doesn't actually define such a class, but we can add that for SC reasons.
If we fix the PREFIX to KNewStuff3 then we get
-- Up-to-date: /d/kde/inst/kde_frameworks/include/KF5/KNewStuff3/KNewStuff3/DownloadDialog
-- Up-to-date: /d/kde/inst/kde_frameworks/include/KF5/KNewStuff3/KNewStuff3/DownloadWidget
-- Up-to-date: /d/kde/inst/kde_frameworks/include/KF5/KNewStuff3/KNewStuff3/UploadDialog
-- Up-to-date: /d/kde/inst/kde_frameworks/include/KF5/KNewStuff3/KNewStuff3/Button
-- Up-to-date: /d/kde/inst/kde_frameworks/include/KF5/KNewStuff3/KNewStuff3/DownloadManager
-- Up-to-date: /d/kde/inst/kde_frameworks/include/KF5/KNewStuff3/KNewStuff3/Entry
-- Up-to-date: /d/kde/inst/kde_frameworks/include/KF5/KNewStuff3/knewstuff3/knewstuff_export.h
-- Up-to-date: /d/kde/inst/kde_frameworks/include/KF5/KNewStuff3/knewstuff3/knewstuffaction.h
-- Up-to-date: /d/kde/inst/kde_frameworks/include/KF5/KNewStuff3/knewstuff3/knewstuffbutton.h
-- Up-to-date: /d/kde/inst/kde_frameworks/include/KF5/KNewStuff3/knewstuff3/downloaddialog.h
-- Up-to-date: /d/kde/inst/kde_frameworks/include/KF5/KNewStuff3/knewstuff3/downloadwidget.h
-- Up-to-date: /d/kde/inst/kde_frameworks/include/KF5/KNewStuff3/knewstuff3/downloadmanager.h
-- Up-to-date: /d/kde/inst/kde_frameworks/include/KF5/KNewStuff3/knewstuff3/uploaddialog.h
-- Up-to-date: /d/kde/inst/kde_frameworks/include/KF5/KNewStuff3/knewstuff3/entry.h
-- Up-to-date: /d/kde/inst/kde_frameworks/include/KF5/KNewStuff3/knewstuff3/button.h
Looks nice and consistent to me (including consistent with the other frameworks).</pre>
<br />
<p>- David Faure</p>
<br />
<p>On January 12th, 2014, 8:15 p.m. UTC, Friedrich W. H. Kossebau wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://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, Aleix Pol Gonzalez and David Faure.</div>
<div>By Friedrich W. H. Kossebau.</div>
<p style="color: grey;"><i>Updated Jan. 12, 2014, 8:15 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
knewstuff
</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;">Currently the KNewStuff CamelCase forwarding headers are installed in [...]/include/KF5/KNewStuff3/knewstuff3
Which seems wrong, at least does not follow the pattern seen with the other namespaced modules. And breaks all existing #includes if the build does not use KDE4Support with its variants of the CamelCase forwarding headers.
Attached patch changes the PREFIX parameter to ecm_generate_headers from knewstuff3 to KNS3, so that the prefix matches the namespace of the classes in the module.</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;">Applied and run make install: CamelCase includes are installed in [...]/include/KF5/KNewStuff3/KNS3 directory, code which #includes KNS3/* without KDE4Support builds.</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>src/CMakeLists.txt <span style="color: grey">(05cd500)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/114988/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>