D8492: Make SourceFormatter plugins enablable, hide actions with no plugins

[Milian Wolff]

mwolff added inline comments.

INLINE COMMENTS

> kossebau wrote in sourceformattercontroller.cpp:192
> `== 1` only. Because for > 1 the actions should have been already enabled before, no state change when there are more than one plugin. Unless things are flawed elsewhere,, which they should not :)
> would you prefer better safe-then-sorry logic instead?

add a comment on the `== 1` case to explain why this is OK.

Also, personal pet-peeve: please try to use `size()` everywhere in preference over `count()`, to make this more in align with C++/STL.

> sourceformattercontroller.cpp:318
>  
>      QStringList formatterinfo = formatter.split( QStringLiteral("||"), QString::SkipEmptyParts );
>  

future optimization opportunity: use `splitRef`

> sourceformattercontroller.cpp:326
> +    foreach (ISourceFormatter* iformatter, d->sourceFormatters) {
> +        if (iformatter->name() == formatterinfo.at(0)) {
> +            return iformatter;

use `.first()` instead of `.at(0)`

> sourceformatterselectionedit.cpp:121
> +            this, &SourceFormatterSelectionEdit::removeSourceFormatter);
> +    for (auto formatter : controller->formatters()) {
> +        addSourceFormatter(formatter);

`auto* `, to make clear this is a pointer. Otherwise it should be `const &` or `const &&`

> sourceformatterselectionedit.cpp:181
>  
> -    foreach (const QString& name, d->languages.keys()) {
> -        if( !sortedLanguages.contains( name ) )
> -            sortedLanguages.push_back( name );
> +    auto languageIter = d->languages.begin();
> +    while (languageIter != d->languages.end()) {

use remove_if + erase

> sourceformatterselectionedit.cpp:204
> +{
> +    for (auto languageIter = d->languages.begin(); languageIter != d->languages.end(); ++languageIter) {
>          // Pick the first appropriate mimetype for this language

this cleanup could be done in a separate patch and committed directly

> sourceformatterselectionedit.cpp:244
> +
> +    foreach(const auto language,
> +                KDevelop::ICore::self()->languageController()->activeLanguages() +

I realize this is old code, but could you do me a favor and clean it up (potentially in a follow-up commit?)

It makes my eyes bleed. Instead, do something like

  // Sort the languages, preferring firstly active, then loaded languages, then any others
  for (const auto& languages : {ICore::self()->languageController()->activeLanguages(),
                                ICore::self()->languageController()->loadedLanguages()})
  {
      for (const auto* language : languages) {
          if (d->languages.contains(language->name()) {
              auto it = std::find(languagesToAdd.begin(), languagesToAdd.end(), language);
              if (it == languagesToAdd.end()) {
                  return;
              }
              languagesToAdd.erase(it);
              d->ui.cbLanguages->addItem(name);
          }
      }
  }
  
  for (const auto &language : languagesToAdd) {
      d->ui.cbLanguages->addItem(language);
  }

REPOSITORY
  R32 KDevelop

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

To: kossebau, #kdevelop, kfunk
Cc: mwolff, kfunk, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20171119/d393e201/attachment.html>