D27791: BUG: 408312

[Juraj Oravec]

SGOrava added a comment.


  Thank you for your contribution.
  
  Our Gitlab instance was moved just recently (few days ago).

INLINE COMMENTS

> pluginsmanager.cpp:55
>      connect(mApp->plugins(), &Plugins::availablePluginsChanged, this, &PluginsManager::refresh);
> -
> +    connect(ui->search,&QLineEdit::textChanged,this,&PluginsManager::addFilter);
>      ui->list->setItemDelegate(new PluginListDelegate(ui->list));

same here, I would keep a blank line after connect commands.

> pluginsmanager.cpp:173
>  }
> -
>  void PluginsManager::itemChanged(QListWidgetItem* item)

Please do not delete the blank line, or lines which you do not WANT to delete.

It is good to keep unrelated code intact.

> pluginsmanager.cpp:246
> +       const QString& pluginName = ui->list->item(i)->text();
> +        if (pluginName.startsWith(filter,Qt::CaseInsensitive) || !filter.size())
> +            ui->list->item(i)->setHidden(false);

This checks only if the plugin name starts with given string which is insufficient for proper search.

Using QString::contains() fits the criteria of "search" much more.

Plus Always use {} for commands after the conditions.

> pluginsmanager.h:47
>      void itemChanged(QListWidgetItem* item);
> -
> +    void addFilter(const QString& newText);
>      void refresh();

In headers methods are sometimes divided by blank lines to indicate that they do different kind of job than the previous "group" of methods, it is good to keep those blanks and find a better place for the new entry (this is sometimes a challenge but it is worth it because it makes code more readable (at least for me))

REPOSITORY
  R875 Falkon

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

To: malibushko, SGOrava
Cc: falkon, grune, akilgus, siraj_qazi, saishm, anmolgautam, SGOrava, iodelay, spoorun, ptabis, navarromorales, clivej, mparillo
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/falkon/attachments/20200303/b7288362/attachment.html>