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

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added a comment.
This revision now requires changes to proceed.


  minor nitpicks, otherwise lgtm now - thanks a lot!

INLINE COMMENTS

> useswidget.cpp:52
> +{
> +    if(isHighlighted) {
> +      return QColor(251, 150, 242).name();

space after `if`

> useswidget.cpp:171
> +{
> +    if (highlight) {
> +        m_label->setText(m_label->text().replace("background-color:" + backgroundColor(false), "background-color:" + backgroundColor(true)));

maybe add this early on:

  if (highlight == m_isHighlighted) {
      return;
  }

> dporobic wrote in useswidget.h:52
> Added const only to the one function that returned a value, is that what you meant?

yes, perfect

> contextbrowserview.cpp:60
>  
> +namespace
> +

join next lines, so that it reads

  namespace {
  ...

> contextbrowserview.cpp:63
> +{
> +enum Direction { NextUse, PreviousUse };
> +

expand to multiple lines

  enum Direction
  {
      NextUse,
      PreviousUse
  };

> contextbrowserview.cpp:65
> +
> +void selectUse(ContextBrowserView *view, Direction direction)
> +{

place `*` next to the typename

> contextbrowserview.cpp:78
> +
> +    OneUseWidget *first = NULL, *previouse = NULL, *current = NULL;
> +    for (auto item : usesWidget->items()) {

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

typo: `previouse` -> `previous`

> contextbrowserview.cpp:81
> +        auto topContext = dynamic_cast<TopContextUsesWidget*>(item);
> +        if(!topContext) {
> +            continue;

space after `if`

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/20170212/9913d948/attachment-0001.html>