<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/118194/">https://git.reviewboard.kde.org/r/118194/</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 21st, 2014, 12:07 p.m. MSK, <b>Milian Wolff</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;">Missing the ICompilerProvider specification, I find it hard to review.
This now introduces:
- KDevelop::ICompilerProvider
- BaseProvider
- CompilerProvider
- IADCompilerProvider
- the gcc, clang and msvc providers
This is far too complicated. Also the whole code comparing strings of compiler names like "gcc", "clang", "msvc" is very error-prone and hard to extend.
I suggest instead to make the gcc, clang and msvc plugins implementing the new ICompilerProvider interface. Add a "virtual QString name() const = 0" to the interface (which should return a localized human readable name). Then in the GUI just query the plugins implementing this interface to fill the combobox. Oh and also add a "virtual QString defaultPath() const = 0" and show that in the UI as well to fill the path-lineedit by default. To remember what compiler the user selected, use the plugin ID (not the pretty localized name).
Then the API to actually query includes and defines should take the actual user-selected path i.e.:
virtual QStringList includes(const QString &path) const = 0
virtual QHash<QString, QString> defines(const QString &path) const = 0
This should then be queried from the IADM. Also ensure to cache this data. We don't want to query gcc/clang/msvc for data every time we parse a file.</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;">>Missing the ICompilerProvider specification
Nope, It was here all the time, seems like you read too fast and didn't notice it...
>Then in the GUI just query the plugins implementing this interface to fill the combobox.
Why bother if KConfig framework does it automatically for us?
>To remember what compiler the user selected, use the plugin ID (not the pretty localized name).
What is the plugin ID? Also compiler names not localized and shouldn't be, so I'm not sure if it makes sense at all...
>Also ensure to cache this data.
Yeap, already done that, see e.g. GccLikeProvider::includes. On the first line it checks whether data already parsed, if so it returns it without quering the compiler.
</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 21st, 2014, 12:07 p.m. MSK, <b>Milian Wolff</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/118194/diff/1/?file=273406#file273406line126" style="color: black; font-weight: bold; text-decoration: underline;">languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">126</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span> <span class="s">"none"</span><span class="p">;</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;">return QString() and in the UI replace it with a proper localized error message.</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;">>in the UI replace it with a proper localized error message.
After some thinking I've decided not to show an error message, IMO issuing a warning is more than enough.</pre>
<br />
<p>- Sergey</p>
<br />
<p>On May 18th, 2014, 9:34 p.m. MSK, Sergey Kalinichev 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 KDevelop.</div>
<div>By Sergey Kalinichev.</div>
<p style="color: grey;"><i>Updated May 18, 2014, 9:34 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevelop
</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;">To make it all work I had to turn IADM into global plugin, that way it can be used to retrieve includes/defines for files without project.
Also I made SettingsManager a shared library to ease access to it from different places.
</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;">All tests pass (except cppspecialcompletion, which doesn't work for me anyway).
Also I've tested it with 3 opened projects in one session, works fine for me.</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>languages/cpp/setuphelpers.cpp <span style="color: grey">(57505fc)</span></li>
<li>languages/cpp/setuphelpers.h <span style="color: grey">(012dcf4)</span></li>
<li>languages/cpp/includepathcomputer.cpp <span style="color: grey">(822b0bc)</span></li>
<li>languages/cpp/msvcdefinehelper.cpp <span style="color: grey">(9b131ed)</span></li>
<li>languages/cpp/cpputils.cpp <span style="color: grey">(380dd4e)</span></li>
<li>languages/cpp/cpputils.h <span style="color: grey">(c951bd3)</span></li>
<li>languages/cpp/CMakeLists.txt <span style="color: grey">(377f35e)</span></li>
<li>languages/cpp/cpplanguagesupport.cpp <span style="color: grey">(0f3bd4a)</span></li>
<li>languages/cpp/setuphelpers_gcc_like.cpp <span style="color: grey">(b261589)</span></li>
<li>languages/cpp/setuphelpers_msvc.cpp <span style="color: grey">(5576c01)</span></li>
<li>languages/cpp/tests/CMakeLists.txt <span style="color: grey">(d6eb67a)</span></li>
<li>languages/cpp/tests/test_cppfiles.cpp <span style="color: grey">(76486d0)</span></li>
<li>languages/plugins/custom-definesandincludes/CMakeLists.txt <span style="color: grey">(be7d6e8)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/gcclikeprovider.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/gcclikeprovider.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/icompilerprovider.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/kdevcompilerprovider.desktop.cmake <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/msvcdefinehelper.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/msvcprovider.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/plugins/custom-definesandincludes/compilerprovider/msvcprovider.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>languages/plugins/custom-definesandincludes/definesandincludesmanager.h <span style="color: grey">(12a7763)</span></li>
<li>languages/plugins/custom-definesandincludes/definesandincludesmanager.cpp <span style="color: grey">(bdce956)</span></li>
<li>languages/plugins/custom-definesandincludes/kcm_widget/CMakeLists.txt <span style="color: grey">(0d8c830)</span></li>
<li>languages/plugins/custom-definesandincludes/kcm_widget/customdefinesandincludes.kcfg <span style="color: grey">(3b49940)</span></li>
<li>languages/plugins/custom-definesandincludes/kcm_widget/customdefinesandincludes.kcfgc <span style="color: grey">(720af41)</span></li>
<li>languages/plugins/custom-definesandincludes/kcm_widget/kcm_customdefinesandincludes.cpp <span style="color: grey">(75f5063)</span></li>
<li>languages/plugins/custom-definesandincludes/kcm_widget/kcm_kdevcustomdefinesandincludes.desktop <span style="color: grey">(ef63c57)</span></li>
<li>languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.cpp <span style="color: grey">(1456217)</span></li>
<li>languages/plugins/custom-definesandincludes/kcm_widget/projectpathswidget.ui <span style="color: grey">(4e512a9)</span></li>
<li>languages/plugins/custom-definesandincludes/kdevdefinesandincludesmanager.desktop.cmake <span style="color: grey">(ad08867)</span></li>
<li>languages/plugins/custom-definesandincludes/settingsmanager.h <span style="color: grey">(9ae23d6)</span></li>
<li>languages/plugins/custom-definesandincludes/settingsmanager.cpp <span style="color: grey">(dc7aa3e)</span></li>
<li>languages/plugins/custom-definesandincludes/tests/plugintest.cpp <span style="color: grey">(1cfa2e4)</span></li>
<li>projectmanagers/custom-buildsystem/tests/projects/builddirproject/.kdev4/builddirproject.kdev4 <span style="color: grey">(33c8568)</span></li>
<li>projectmanagers/custom-buildsystem/tests/projects/multipathproject/.kdev4/multipathproject.kdev4 <span style="color: grey">(fd14aca)</span></li>
<li>projectmanagers/custom-buildsystem/tests/projects/simpleproject/.kdev4/simpleproject.kdev4 <span style="color: grey">(2c4bac5)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118194/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<ul>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/05/18/f8aee22e-deac-41f2-999d-7c321a419147__kdevplatform.diff">kdevplatform_changes</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/05/18/e7a99678-843d-4dd1-a5f2-297662956134__selectCompiler_image.png">selectCompiler_image.png</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>