Review Request 110960: FramestackWidget: Make keyboard navigation useful

[Vlas Puhov]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110960/#review35694
-----------------------------------------------------------



debugger/framestack/framestackwidget.cpp
<http://git.reviewboard.kde.org/r/110960/#comment26166>

    Here..



debugger/framestack/framestackwidget.cpp
<http://git.reviewboard.kde.org/r/110960/#comment26167>

    .. and here. Why do you connect twice to frameSelectionChanged, wouldn't it be better to use selectionChanged signal instead??


Btw, I really love this DoNotFocus feature!!

- Vlas Puhov


On July 5, 2013, 7:23 p.m., Nicolai Hähnle wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110960/
> -----------------------------------------------------------
> 
> (Updated July 5, 2013, 7:23 p.m.)
> 
> 
> Review request for KDevelop and Niko Sams.
> 
> 
> Description
> -------
> 
> 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!
> 
> 
> This addresses bug 316873.
>     http://bugs.kde.org/show_bug.cgi?id=316873
> 
> 
> Diffs
> -----
> 
>   debugger/framestack/framestackwidget.h 9794353 
>   debugger/framestack/framestackwidget.cpp dcc7a64 
>   interfaces/idocumentcontroller.h 434cb4d 
>   shell/debugcontroller.cpp d1924fb 
>   shell/documentcontroller.cpp 660637f 
>   sublime/mainwindow.h 421ee57 
>   sublime/mainwindow.cpp dce4bf7 
> 
> Diff: http://git.reviewboard.kde.org/r/110960/diff/
> 
> 
> Testing
> -------
> 
> 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.
> 
> 
> Thanks,
> 
> Nicolai Hähnle
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20130707/32f30a43/attachment-0001.html>