Differential D4483 Diff 11387 plugins/contextbrowser/contextbrowserview.cpp

Changeset View

Standalone View

View Options

plugins/contextbrowser/contextbrowserview.cpp

Show First 20 Lines • Show All 51 Lines • ▼ Show 20 Line(s)
52#include <language/interfaces/codecontext.h> 52​#include <language/interfaces/codecontext.h>
53#include <language/duchain/navigation/abstractdeclarationnavigationcontext.h> 53​#include <language/duchain/navigation/abstractdeclarationnavigationcontext.h>
54#include <language/duchain/navigation/useswidget.h> 54​#include <language/duchain/navigation/useswidget.h>
55#include <interfaces/contextmenuextension.h> 55​#include <interfaces/contextmenuextension.h>
56#include <interfaces/iplugincontroller.h> 56​#include <interfaces/iplugincontroller.h>
57 57​
58using namespace KDevelop; 58​using namespace KDevelop;
59 59​
60​namespace {
mwolffUnsubmitted
Done

join next lines, so that it reads

namespace {
...
mwolff: join next lines, so that it reads ``` namespace { ... ```
61​
62​enum Direction
63​{
mwolffUnsubmitted
Done

expand to multiple lines

enum Direction
{
    NextUse,
    PreviousUse
};
mwolff: expand to multiple lines ``` enum Direction { NextUse, PreviousUse }; ```
64​ NextUse,
65​ PreviousUse
mwolffUnsubmitted
Done

place * next to the typename

mwolff: place `*` next to the typename
66​};
67​
68​void selectUse(ContextBrowserView* view, Direction direction)
69​{
70​ auto abstractNaviWidget = dynamic_cast<AbstractNavigationWidget*>(view->navigationWidget());
71​
72​ if (!abstractNaviWidget) {
73​ return;
74​ }
75​
76​ auto usesWidget = dynamic_cast<UsesWidget*>(abstractNaviWidget->context()->widget());
77​ if (!usesWidget) {
78​ return;
mwolffUnsubmitted
Done

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

typo: previouse -> previous

mwolff: place `*` next to the typename, use `nullptr` (C++11) instead of the C `NULL` macro typo…
79​ }
80​
81​ OneUseWidget* first = nullptr, *previous = nullptr, *current = nullptr;
mwolffUnsubmitted
Done

space after if

mwolff: space after `if`
82​ for (auto item : usesWidget->items()) {
83​ auto topContext = dynamic_cast<TopContextUsesWidget*>(item);
84​ if (!topContext) {
85​ continue;
86​ }
87​ for (auto item : topContext->items()) {
88​ auto navigationList = dynamic_cast<NavigatableWidgetList*>(item);
89​ if (!navigationList) {
90​ continue;
91​ }
92​ for (auto item : navigationList->items()) {
93​ auto use = dynamic_cast<OneUseWidget*>(item);
94​ if (!use) {
95​ continue;
96​ }
97​ if (!first) {
98​ first = use;
99​ }
100​ current = use;
101​ if (direction == PreviousUse && current->isHighlighted() && previous) {
102​ previous->setHighlighted(true);
103​ previous->activateLink();
104​ current->setHighlighted(false);
105​ return;
106​ }
107​ if (direction == NextUse && previous && previous->isHighlighted()) {
108​ current->setHighlighted(true);
109​ current->activateLink();
110​ previous->setHighlighted(false);
111​ return;
112​ }
113​ previous = current;
114​ }
115​ }
116​ }
117​ if (direction == NextUse && first) {
118​ first->setHighlighted(true);
119​ first->activateLink();
120​ if (current && current->isHighlighted())
121​ current->setHighlighted(false);
122​ return;
123​ }
124​ if (direction == PreviousUse && current) {
125​ current->setHighlighted(true);
126​ current->activateLink();
127​ if (first && first->isHighlighted()) {
128​ first->setHighlighted(false);
129​ }
130​ }
131​}
132​
133​}
134​
60QWidget* ContextBrowserView::createWidget(KDevelop::DUContext* context) { 135​QWidget* ContextBrowserView::createWidget(KDevelop::DUContext* context) {
61 m_context = IndexedDUContext(context); 136​ m_context = IndexedDUContext(context);
62 if(m_context.data()) { 137​ if(m_context.data()) {
63 return m_context.data()->createNavigationWidget(nullptr, nullptr, {}, {}, AbstractNavigationWidget::EmbeddableWidget); 138​ return m_context.data()->createNavigationWidget(nullptr, nullptr, {}, {}, AbstractNavigationWidget::EmbeddableWidget);
64 } 139​ }
65 return nullptr; 140​ return nullptr;
66} 141​}
67 142​
▲ Show 20 Lines • Show All 101 Lines • ▼ Show 20 Line(s)238 if(navigationWidget && event->type() == QEvent::KeyPress) {
169 if(key == Qt::Key_Up) 244​ if(key == Qt::Key_Up)
170 navigationWidget->up(); 245​ navigationWidget->up();
171 if(key == Qt::Key_Down) 246​ if(key == Qt::Key_Down)
172 navigationWidget->down(); 247​ navigationWidget->down();
173 if(key == Qt::Key_Return || key == Qt::Key_Enter) 248​ if(key == Qt::Key_Return || key == Qt::Key_Enter)
174 navigationWidget->accept(); 249​ navigationWidget->accept();
175 250​
176 251​
177 if(key == Qt::Key_L) 252​ if(key == Qt::Key_L)
mwolffUnsubmitted
Not Done

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)

mwolff: this is conceptually bad, esp. performance wise - you build a list of all uses - then you…
178 m_lockAction->toggle(); 253​ m_lockAction->toggle();
179 } 254​ }
180 } 255​ }
181 return QWidget::event(event); 256​ return QWidget::event(event);
182} 257​}
183 258​
184void ContextBrowserView::showEvent(QShowEvent* event) 259​void ContextBrowserView::showEvent(QShowEvent* event)
185{ 260​{
Show All 35 Lines290 if (widget) {
221 m_allowLockedUpdate = false; 296​ m_allowLockedUpdate = false;
222 setUpdatesEnabled(true); 297​ setUpdatesEnabled(true);
223 if (widget->metaObject()->indexOfSignal(QMetaObject::normalizedSignature("contextChanged(bool,bool)")) != -1) { 298​ if (widget->metaObject()->indexOfSignal(QMetaObject::normalizedSignature("contextChanged(bool,bool)")) != -1) {
224 connect(widget, SIGNAL(contextChanged(bool,bool)), this, SLOT(navigationContextChanged(bool,bool))); 299​ connect(widget, SIGNAL(contextChanged(bool,bool)), this, SLOT(navigationContextChanged(bool,bool)));
225 } 300​ }
226 } 301​ }
227} 302​}
228 303​
229void ContextBrowserView::navigationContextChanged(bool wasInitial, bool isInitial) 304​void ContextBrowserView::navigationContextChanged(bool wasInitial, bool isInitial)
mwolffUnsubmitted
Done

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

mwolff: style: ``` auto abstractNaviWidget = dynamic_cast<AbstractNavigationWidget*>(navigationWidget…
dporobicAuthorUnsubmitted
Not Done

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

dporobic: Check out if this looks better, I've tried to follow your instruction hope it's what you had in…
230{ 305​{
231 if(wasInitial && !isInitial && !m_lockAction->isChecked()) 306​ if(wasInitial && !isInitial && !m_lockAction->isChecked())
232 { 307​ {
233 m_autoLocked = true; 308​ m_autoLocked = true;
234 m_lockAction->setChecked(true); 309​ m_lockAction->setChecked(true);
235 }else if(!wasInitial && isInitial && m_autoLocked) 310​ }else if(!wasInitial && isInitial && m_autoLocked)
236 { 311​ {
237 m_autoLocked = false; 312​ m_autoLocked = false;
238 m_lockAction->setChecked(false); 313​ m_lockAction->setChecked(false);
239 }else if(isInitial) { 314​ }else if(isInitial) {
mwolffUnsubmitted
Done

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

for (auto item : usesWidget->items()) {
mwolff: please don't use foreach in new code, use C++11 range-based-for ``` for (auto item…
240 m_autoLocked = false; 315​ m_autoLocked = false;
241 } 316​ }
mwolffUnsubmitted
Done

reduce indent width by instead doing

if (!topContext) {
    continue;
}
mwolff: reduce indent width by instead doing ``` if (!topContext) { continue; } ```
242} 317​}
mwolffUnsubmitted
Done

dito, rnage-based-for

mwolff: dito, rnage-based-for
243 318​
319​void ContextBrowserView::selectNextItem()
mwolffUnsubmitted
Done

dito, reduce indent width

mwolff: dito, reduce indent width
320​{
321​ selectUse(this, NextUse);
322​}
323​
324​void ContextBrowserView::selectPreviousItem()
325​{
326​ selectUse(this, PreviousUse);
327​}
328​
244void ContextBrowserView::setDeclaration(KDevelop::Declaration* decl, KDevelop::TopDUContext* topContext, bool force) { 329​void ContextBrowserView::setDeclaration(KDevelop::Declaration* decl, KDevelop::TopDUContext* topContext, bool force) {
245 m_lastUsedTopContext = IndexedTopDUContext(topContext); 330​ m_lastUsedTopContext = IndexedTopDUContext(topContext);
246 331​
247 if(isLocked() && (!m_navigationWidget.data() || !isVisible())) 332​ if(isLocked() && (!m_navigationWidget.data() || !isVisible()))
248 { 333​ {
249 // Automatically remove the locked state if the view is not visible or the widget was deleted, 334​ // Automatically remove the locked state if the view is not visible or the widget was deleted,
250 // because the locked state has side-effects on other navigation functionality. 335​ // because the locked state has side-effects on other navigation functionality.
mwolffUnsubmitted
Not Done

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);
mwolff: replace manual loop: ``` auto it = std::find_if(uses.begin(), uses.end(), [](QWidget* widget)…
dporobicAuthorUnsubmitted
Not Done

Removed this functionality, not longer required.

dporobic: Removed this functionality, not longer required.
251 m_autoLocked = false; 336​ m_autoLocked = false;
252 m_lockAction->setChecked(false); 337​ m_lockAction->setChecked(false);
253 } 338​ }
254 339​
255 if(m_navigationWidgetDeclaration == decl->id() && !force) 340​ if(m_navigationWidgetDeclaration == decl->id() && !force)
256 return; 341​ return;
257 342​
258 m_navigationWidgetDeclaration = decl->id(); 343​ m_navigationWidgetDeclaration = decl->id();
▲ Show 20 Lines • Show All 53 Lines • Show Last 20 Lines