<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/121484/">https://git.reviewboard.kde.org/r/121484/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Dezember 14th, 2014, 12:22 nachm. UTC, <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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">good stuff, I guess :) but I wonder about other users of this API, such as the python debugger - do you think they'll continue to work or are there API/behavior changes in here which will require changes to the other debuggers?</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;">Thank you for telling me about kdev-python. I'll look into that one.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This particular changeset is very much binary incompatible, but only trivial source changes (potentially none at all) are necessary to keep other debuggers working. However, as I wrote I do intend to go further in removing the "old style API" in IBreakpointController in later steps of this refactoring. It would be feasible to create a class CompatBreakpointController which would allow old-style debuggers to be ported with relatively few and mechanical source-level changes.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">How I will proceed depends a bit on how many more other debuggers are hidden behind the "such as". If it's just kdev-python, I might just evaluate porting that at the same time. If it's more, then I'd be happy to punt that work by introducing such a compat class...</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Dezember 14th, 2014, 12:22 nachm. UTC, <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/121484/diff/1/?file=333004#file333004line52" style="color: black; font-weight: bold; text-decoration: underline;">debugger/breakpoint/breakpointmodel.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="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">BreakpointModel::BreakpointModel(QObject* parent)</pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">49</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">BreakpointModel</span><span class="o">::</span><span class="n">BreakpointModel</span><span class="p">(</span><span class="n"><span class="hl">QObject</span></span><span class="o">*</span> <span class="n">parent</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">52</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">BreakpointModel</span><span class="o">::</span><span class="n">BreakpointModel</span><span class="p">(</span><span class="n"><span class="hl">IDebugController</span></span><span class="o">*</span> <span class="n">parent</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'd rather this take two arguments, the debug controller and a QObject parent. This is more safe, api-wise (one cannot accidentally breaks stuff by calling setParent), and allows the model to be constructed on the stack, if desired, by setting parent = 0.</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Good point about setParent(), I hadn't thought about that. I now go via KDevelop::ICore::self(), because passing a second argument to the constructor seems a bit cumbersome.</p></pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On Dezember 14th, 2014, 12:22 nachm. UTC, <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/121484/diff/1/?file=333008#file333008line208" style="color: black; font-weight: bold; text-decoration: underline;">debugger/interfaces/ibreakpointcontroller.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; ">IBreakpointController::IBreakpointController(KDevelop::IDebugSession* parent)</pre></td>
</tr>
</tbody>
<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">172</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">BreakpointModel</span> <span class="o">*</span> <span class="n">breakpointModel</span> <span class="o">=</span> <span class="k">this</span><span class="o">-></span><span class="n">breakpointModel</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">style: * next to typename (you can configure KDevelop to use this style by default, btw.), or just auto. I'd prefer it this way:</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: #008000; font-weight: bold">auto</span> model <span style="color: #666666">=</span> breakpointModel();
...
</pre></div>
</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As long as KDevelop's C++ support isn't perfect, I actually prefer explicit types when they're not too cumbersome.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Which configuration do you refer to? I just enter the * and & manually...</p></pre>
<br />
<p>- Nicolai</p>
<br />
<p>On Dezember 14th, 2014, 6:59 nachm. UTC, Nicolai Hähnle 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.</div>
<div>By Nicolai Hähnle.</div>
<p style="color: grey;"><i>Updated Dez. 14, 2014, 6:59 nachm.</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;">Overall, I want to move the code into a shape where BreakpointModel stores all information about breakpoints as presented to the user, and IBreakpointController is a genuine thin interface with no code (or only shim code) in kdevplatform.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The changes are necessarily binary incompatible, but AFAIU this is okay in current kdevplatform and kdevelop master. There are also minor source incompatible changes; I have already updated kdevelop master so that it is compatible with them.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The next step after this set of changes will be to change the gdb plugin in kdevelop so that it no longer uses what is now marked as the "old API". This will reduce the interactions between kdevelop and kdevplatform. Once those changes have landed and are stable, the old API in kdevplatform can then be purged.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Besides the usual reviewing feedback and feedback on the general direction of these changes, I am particularly interested in:</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Are there other users of the debuggers/breakpoint API, and source/binary compatibility policies that I should be aware of?</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I introduced the Column and ColumnFlags enum, where the latter is intended to replace various sets by more compact bit fields. Is there a nice type-safe Qt way of having such a pair of enums, where one is for bitfield corresponding to sets of the other?</p>
</li>
</ol></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;">kdevelop gdb unit tests work</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>debugger/breakpoint/breakpoint.h <span style="color: grey">(23c690f243ec3a46dfb66fd220c620125fd07327)</span></li>
<li>debugger/breakpoint/breakpoint.cpp <span style="color: grey">(7e6de84208050192b6af3242c0cabd5f5515b567)</span></li>
<li>debugger/breakpoint/breakpointdetails.cpp <span style="color: grey">(40d90bc863b10df24adf7189e12265ceb434a690)</span></li>
<li>debugger/breakpoint/breakpointmodel.h <span style="color: grey">(025e78e6d270f870a6ad7d4c526640a3a404f59c)</span></li>
<li>debugger/breakpoint/breakpointmodel.cpp <span style="color: grey">(0810db2f4f844beee13861ce4ba7d91464956fb0)</span></li>
<li>debugger/breakpoint/breakpointwidget.h <span style="color: grey">(66567869e6d1fe8bdb89669c99a8fd1286568630)</span></li>
<li>debugger/breakpoint/breakpointwidget.cpp <span style="color: grey">(48e3a7e6c11f4953098b24121c66a32e934a6acb)</span></li>
<li>debugger/interfaces/ibreakpointcontroller.h <span style="color: grey">(5da4f3efafe7c6e97fbfafe9b639e05d2e0478ea)</span></li>
<li>debugger/interfaces/ibreakpointcontroller.cpp <span style="color: grey">(03a7546166a61a8be9c9b6def2dcb3596c17e82a)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121484/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>