Keyboard shortcuts for Uses in Code Browser
ClosedPublic

Authored by dporobic on Feb 7 2017, 6:29 PM.

Details

Summary

https://bugs.kde.org/show_bug.cgi?id=245902

Fix for bug/wish 245902. I've used the itoolactionviewer to implement this feature, when the Code Browser is open and shows uses you can jump through all Uses by pressing F4 and Shift+F4, the Use that is selected will also be opened, just like clicking on it.

The allUses() function is maybe heavy due to three iterations Any suggestion for code improvement is welcome.

Test Plan

Manuel Testing

Diff Detail

Repository
R33 KDevPlatform
Lint
Lint Skipped
Unit
Unit Tests Skipped
dporobic updated this revision to Diff 11021.Feb 7 2017, 6:29 PM
dporobic retitled this revision from to Keyboard shortcuts for Uses in Code Browser.
dporobic updated this object.
dporobic edited the test plan for this revision. (Show Details)
dporobic added a reviewer: mwolff.
dporobic set the repository for this revision to R33 KDevPlatform.
dporobic added subscribers: KDevelop, kdevelop-devel.
mwolff requested changes to this revision.Feb 9 2017, 3:56 PM
mwolff edited edge metadata.
mwolff added inline comments.
language/duchain/navigation/useswidget.cpp
160

this is pretty brittle, please introduce free functions in an anonymous namespace that return a QColor for the active and inactive color, then use their names here. I simply guess that the hashes you use here are those of the QColors used in highlightAndEscapeUseText?

162

style: join with next line

language/duchain/navigation/useswidget.h
52

const

plugins/contextbrowser/contextbrowserview.cpp
252

this is conceptually bad, esp. performance wise

  • you build a list of all uses
  • then you iterate that list to find the selected one
  • if there is no selected one you select the first one
  • otherwise you unselect the current one and select the next one

you essentially don't need the list of all uses for this at all.

rather, what you want is a free function taking in an enum that decides whether to select the next or previous item and then do it in the loop. no temporary list required, only the first, previous, current item needs to be remembered, and then depending on

if this is too complicated for you, at least find the index of the "current" one already in allUses to remove the need for selectedUseIndex altogether (return a struct of the list of OneUseWidget* and the pointer to the current one)

304

style:

auto abstractNaviWidget = dynamic_cast<AbstractNavigationWidget*>(navigationWidget());

don't put everything into QPointer, there's no need for it in this patch as far as I can see

314

please don't use foreach in new code, use C++11 range-based-for

for (auto item : usesWidget->items()) {
316

reduce indent width by instead doing

if (!topContext) {
    continue;
}
317

dito, rnage-based-for

319

dito, reduce indent width

335

replace manual loop:

auto it = std::find_if(uses.begin(), uses.end(), [](QWidget* widget) -> bool {
                           if (auto use = qobject_cast<OneUseWidget*>(widget)) {
                               return use->isHighlighted();
                           }
                           return false;
                       });
return (it == uses.end()) ? -1 : std::distance(uses.begin(), it);
plugins/contextbrowser/contextbrowserview.h
97

const

98

const& the arg and move out of the class into a free function in an anon namespace in the .cpp file

This revision now requires changes to proceed.Feb 9 2017, 3:56 PM
dporobic updated this revision to Diff 11238.Feb 11 2017, 7:28 PM
dporobic edited edge metadata.

Moved must of the functionality to single free function in separate namespace. Some additional cleanup as suggested by mwolff

dporobic marked 8 inline comments as done.Feb 11 2017, 7:32 PM
dporobic added inline comments.
language/duchain/navigation/useswidget.h
52

Added const only to the one function that returned a value, is that what you meant?

plugins/contextbrowser/contextbrowserview.cpp
304

Check out if this looks better, I've tried to follow your instruction hope it's what you had in mind.

335

Removed this functionality, not longer required.

mwolff requested changes to this revision.Feb 12 2017, 11:38 AM
mwolff edited edge metadata.

minor nitpicks, otherwise lgtm now - thanks a lot!

language/duchain/navigation/useswidget.cpp
52

space after if

170

maybe add this early on:

if (highlight == m_isHighlighted) {
    return;
}
language/duchain/navigation/useswidget.h
52

yes, perfect

plugins/contextbrowser/contextbrowserview.cpp
60

join next lines, so that it reads

namespace {
...
63

expand to multiple lines

enum Direction
{
    NextUse,
    PreviousUse
};
65

place * next to the typename

78

place * next to the typename, use nullptr (C++11) instead of the C NULL macro

typo: previouse -> previous

81

space after if

This revision now requires changes to proceed.Feb 12 2017, 11:38 AM
dporobic updated this revision to Diff 11249.Feb 12 2017, 1:13 PM
dporobic edited edge metadata.
dporobic marked 2 inline comments as done.

More formatting and style cleanup as suggested by mwolff

dporobic marked 7 inline comments as done.Feb 12 2017, 1:15 PM

Changed formatting as suggested. If it's ok now could you please commit the patch, I don't have commit rights?

Changed formatting as suggested. If it's ok now could you please commit the patch, I don't have commit rights?

And here my contact details for the commit:
Damir Porobic
damir_porobic (at) live (dot) com

mwolff accepted this revision.Feb 15 2017, 10:28 PM
mwolff edited edge metadata.

I'll land this now

This revision is now accepted and ready to land.Feb 15 2017, 10:28 PM
mwolff added a project: KDevelop.
This revision was automatically updated to reflect the committed changes.