<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="http://git.reviewboard.kde.org/r/110960/">http://git.reviewboard.kde.org/r/110960/</a>
</td>
</tr>
</table>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/110960/diff/5/?file=169142#file169142line103" style="color: black; font-weight: bold; text-decoration: underline;">debugger/framestack/framestackwidget.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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; ">FramestackWidget::FramestackWidget(IDebugController* controller, QWidget* parent)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">103</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">connect</span><span class="p">(</span><span class="n">m_frames</span><span class="p">,</span> <span class="n">SIGNAL</span><span class="p">(</span><span class="n">clicked</span><span class="p">(</span><span class="n">QModelIndex</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">103</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">connect</span><span class="p">(</span><span class="n">m_frames</span><span class="p">,</span> <span class="n">SIGNAL</span><span class="p">(</span><span class="n">clicked</span><span class="p">(</span><span class="n">QModelIndex</span><span class="p">)),</span><span class="hl"> </span><span class="n"><span class="hl">SLOT</span></span><span class="p"><span class="hl">(</span></span><span class="n"><span class="hl">frameSelectionChanged</span></span><span class="p"><span class="hl">(</span></span><span class="n"><span class="hl">QModelIndex</span></span><span class="p"><span class="hl">)));</span></span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Here..</pre>
</div>
<br />
<div>
<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="http://git.reviewboard.kde.org/r/110960/diff/5/?file=169142#file169142line133" style="color: black; font-weight: bold; text-decoration: underline;">debugger/framestack/framestackwidget.cpp</a>
<span style="font-weight: normal;">
(Diff revision 5)
</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; ">FramestackWidget::~FramestackWidget() {}</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">132</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">this</span><span class="p">,</span> <span class="n">SLOT</span><span class="p">(</span><span class="n">frameSelectionChanged</span><span class="p">(</span><span class="n">QModelIndex</span><span class="p">)));</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">.. and here. Why do you connect twice to frameSelectionChanged, wouldn't it be better to use selectionChanged signal instead??</pre>
</div>
<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;">Btw, I really love this DoNotFocus feature!!</pre>
<p>- Vlas</p>
<br />
<p>On July 5th, 2013, 7:23 p.m. MSK, Nicolai Hähnle wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://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 and Niko Sams.</div>
<div>By Nicolai Hähnle.</div>
<p style="color: grey;"><i>Updated July 5, 2013, 7:23 p.m.</i></p>
<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;">Allow users to navigate the Frame Stack Toolview using the arrow keys
and tab/shift-tab without distraction to locate the thread(s) and
frame(s) of interest.
This required the following changes:
1) Add IDocumentController::DoNotFocus; assorted changes in
DocumentController and MainWindow.
The intention is clear: Allow opening and showing a document without
yanking the focus away from where it currently is. This could also be
interesting for other toolviews (e.g., keyboard navigation in the Code
Browser).
Focusing the new view is hidden inside MainWindow::activateView, so
I added an overloaded variant of this method which allows the caller
to choose whether the view should get the focus.
2) Use DoNotFocus in the relevant parts of the debugger.
3) Change FramestackWidget signal wiring to react to all types of input
Previously, FramestackWidget connected its slots to the item views'
clicked() signal, which only reacts on mouse input. Use the selection
models' currentChanged() signal instead, which reacts to other types of
input, in particular the keyboard.
As a consequence, the connections have to be set up in
currentSessionChanged(), because that's where the model is fully set up.
Relatedly, switch only m_frames->rootIndex in currentThreadChanged()
rather than the entire model, because the entire model doesn't actually
change.
4) There is one unrelated change, which is that I moved a connection
attempt to checkFetchMoreFrames down into currentSessionChanged;
otherwise, the connection does not succeed (and QObject complains
about it on the console).
I noticed that there is a related bug report, bug #316873, about this
fetch-more-frames feature; the bug report ends rather vaguely without
resolution, and I only noticed it after starting this Review Request.
However, fetching more frames automatically, triggered by scrolling,
works for me.
That's pretty much it. I hope you consider this a useful new feature,
and I hope the patch looks good to you. If you see any problems, or if
I have misstepped outside the coding guidelines at some point, please
let me know!</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;">Only manual testing - I don't know if/how this could be tested
automatically:
Within {my own project, the threads test case from kdevelop,
kdevelop itself} I have interrupted the debug run and done a lot
of mouse- and keyboard-based back and forth between threads and
frames. I also explicitly checked that loading additional frames
still works. That's pretty much it.</pre>
</td>
</tr>
</table>
<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=316873">316873</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>debugger/framestack/framestackwidget.h <span style="color: grey">(9794353)</span></li>
<li>debugger/framestack/framestackwidget.cpp <span style="color: grey">(dcc7a64)</span></li>
<li>interfaces/idocumentcontroller.h <span style="color: grey">(434cb4d)</span></li>
<li>shell/debugcontroller.cpp <span style="color: grey">(d1924fb)</span></li>
<li>shell/documentcontroller.cpp <span style="color: grey">(660637f)</span></li>
<li>sublime/mainwindow.h <span style="color: grey">(421ee57)</span></li>
<li>sublime/mainwindow.cpp <span style="color: grey">(dce4bf7)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/110960/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>