[Differential] [Requested Changes To] D4483: Keyboard shortcuts for Uses in Code Browser

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> useswidget.cpp:160
> +    if (highlight) {
> +        m_label->setText(m_label->text().replace("background-color:#fbfa96", "background-color:#fb96f2"));
> +        m_isHighlighted = true;

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?

> useswidget.cpp:162
> +        m_isHighlighted = true;
> +    }
> +    else {

style: join with next line

> useswidget.h:52
> +        void setHighlighted(bool highlight);
> +        bool isHighlighted();
> +        void activateLink();

const

> contextbrowserview.cpp:252
> +
> +    int index = selectedUseIndex(uses);
> +    QPointer<OneUseWidget> use;

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)

> contextbrowserview.cpp:304
> +
> +    QPointer<AbstractNavigationWidget> abstractNaviWidget = dynamic_cast<AbstractNavigationWidget*>(this->navigationWidget());
> +    if (!abstractNaviWidget) {

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

> contextbrowserview.cpp:314
> +
> +    foreach (QWidget* uw, usesWidget->items()) {
> +        QPointer<TopContextUsesWidget> topContext = dynamic_cast<TopContextUsesWidget*>(uw);

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

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

> contextbrowserview.cpp:316
> +        QPointer<TopContextUsesWidget> topContext = dynamic_cast<TopContextUsesWidget*>(uw);
> +        if (topContext) {
> +            foreach (QWidget* nl, topContext->items()) {

reduce indent width by instead doing

  if (!topContext) {
      continue;
  }

> contextbrowserview.cpp:317
> +        if (topContext) {
> +            foreach (QWidget* nl, topContext->items()) {
> +                QPointer<NavigatableWidgetList> list = dynamic_cast<NavigatableWidgetList*>(nl);

dito, rnage-based-for

> contextbrowserview.cpp:319
> +                QPointer<NavigatableWidgetList> list = dynamic_cast<NavigatableWidgetList*>(nl);
> +                if (list) {
> +                    foreach (QWidget* ou, list->items()) {

dito, reduce indent width

> contextbrowserview.cpp:335
> +{
> +    for(int i = 0; i < uses.count(); i++) {
> +        QPointer<OneUseWidget> use = dynamic_cast<OneUseWidget*>(uses.at(i));

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);

> contextbrowserview.h:97
>          void resetWidget();
> +        QList<QWidget*> allUses();
> +        int selectedUseIndex(QList<QWidget*> uses);

const

> contextbrowserview.h:98
> +        QList<QWidget*> allUses();
> +        int selectedUseIndex(QList<QWidget*> uses);
>  

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

REPOSITORY
  R33 KDevPlatform

REVISION DETAIL
  https://phabricator.kde.org/D4483

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: dporobic, mwolff
Cc: kdevelop-devel, #kdevelop
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170209/6a4cb3cb/attachment-0001.html>