D15743: Fix removal of external scripts

[Francis Herne]

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


  This looks good to me overall, but see a few inline comments.
  
  I've tried various awkward cases with the current patch, and it seems to work fine!

INLINE COMMENTS

> amhndu wrote in externalscriptplugin.cpp:392
> Would this better be a member ?
> Will save the time required to create a new set each call.

We don't need this at all, it's effectively a reimplementation of KConfigGroup::groupList() <https://api.kde.org/frameworks/kconfig/html/classKConfigGroup.html#a1f776ccf3815c32e84c0816403e249b6>.

In principle yours is more efficient for this purpose (doesn't needlessly convert the set to a list), but the number of external-scripts will never be large enough for that to matter.

> externalscriptplugin.cpp:406
> +    int maxSuffix = 0;
> +    QString keyCandidate = item->text() + QString::number( maxSuffix );
> +    for (; keySet.contains( keyCandidate ); ++maxSuffix) {

I'd prefer if this line was just `keyCandidate = item->text()`, and have `maxSuffix` initialised to 2.

Scripts will almost always have unique names, because users don't want to confuse themselves, and it would be tidier not to have a suffix for the normal case.
That would give us:

- [Thing]
- [Thing2]

rather than

- [Thing0]
- [Thing1]

> externalscriptplugin.h:71
>  
> -  void saveItem(const ExternalScriptItem* item);
>  

Why do you rename `save` -> `update` everywhere? To me that implies the opposite action, updating the item from a modified config.

REPOSITORY
  R32 KDevelop

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

To: amhndu, #kdevelop, flherne
Cc: flherne, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20180928/b44a5df5/attachment.html>