Fix removal of external scripts
ClosedPublic

Authored by amhndu on Sep 25 2018, 10:20 AM.

Details

Summary

Previously, deleting a script which is in the middle of the config would invalidate indices
of the scripts that come after it in the config, this would mean deleting
anything later deletes the wrong script.

The config labels are now stored in the model itself and is no longer based on just the index,
but on the name of the script. Thus, deleting/updating one script does not interfere with the
other scripts.

BUG: 385298

Test Plan
  • Before applying the patch:
    • Delete "Google Selection"
    • Restart
    • Delete "Quick Compile"
    • Restart and check list of external scripts
  • Apply the patch and repeat.

Diff Detail

Repository
R32 KDevelop
Branch
script-remove-bug
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 3289
Build 3307: arc lint + arc unit
amhndu created this revision.Sep 25 2018, 10:20 AM
Restricted Application added a project: KDevelop. · View Herald TranscriptSep 25 2018, 10:20 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
amhndu requested review of this revision.Sep 25 2018, 10:20 AM

I couldn't find anything to move a group or rename a group, so I'm copying and deleting.
Is there a better way ?

amhndu retitled this revision from Fix removal of external scripts to [WIP] Fix removal of external scripts.Sep 25 2018, 1:46 PM
amhndu updated this revision to Diff 42320.Sep 25 2018, 7:14 PM
amhndu added a subscriber: flherne.

Refactored external script to store the config label inside the model
as discussed with @flherne on irc
Inserting/Deleting/Updating scripts now works as expected.

amhndu retitled this revision from [WIP] Fix removal of external scripts to Fix removal of external scripts.Sep 25 2018, 7:18 PM
amhndu edited the summary of this revision. (Show Details)
amhndu added inline comments.Sep 26 2018, 10:39 AM
plugins/externalscript/externalscriptplugin.cpp
392 ↗(On Diff #42320)

Would this better be a member ?
Will save the time required to create a new set each call.

Patch to move the key set as a member:

diff --git a/plugins/externalscript/externalscriptplugin.cpp b/plugins/externalscript/externalscriptplugin.cpp
index e1009457c8..3f2bddeeff 100644
--- a/plugins/externalscript/externalscriptplugin.cpp
+++ b/plugins/externalscript/externalscriptplugin.cpp
@@ -120,6 +120,7 @@ ExternalScriptPlugin::ExternalScriptPlugin( QObject* parent, const QVariantList&
       item->action()->setShortcut( QKeySequence( script.readEntry( "shortcuts" ) ) );
       item->setShowOutput( script.readEntry( "showOutput", true ) );
       m_model->appendRow( item );
+      m_keySet.insert( script.name() );
     }
   }
   //END load config
@@ -350,6 +351,7 @@ void ExternalScriptPlugin::rowsAboutToBeRemoved( const QModelIndex& /*parent*/,
     const ExternalScriptItem* const item = static_cast<ExternalScriptItem*>( m_model->item( row ) );
     KConfigGroup child = config.group( item->key() );
     qCDebug(PLUGIN_EXTERNALSCRIPT) << "removing config group:" << child.name();
+    m_keySet.remove( item->key() );
     child.deleteGroup();
   }
   config.sync();
@@ -361,7 +363,10 @@ void ExternalScriptPlugin::updateItem( const ExternalScriptItem* item )
   Q_ASSERT( index.isValid() );
 
   getConfig().group( item->key() ).deleteGroup(); // delete the previous group
+  m_keySet.remove( item->key() );
+
   setupKeys( index.row(), index.row() );
+
   saveItemForRow( index.row() ); // save the new group
 }
 
@@ -389,28 +394,18 @@ void ExternalScriptPlugin::saveItemForRow( int row )
 
 void ExternalScriptPlugin::setupKeys(int start, int end)
 {
-  QSet<QString> keySet;
-  for ( int row = 0; row < m_model->rowCount(); ++row ) {
-    if (row == start) {
-      row = end;
-    } else {
-      const ExternalScriptItem* const item = static_cast<ExternalScriptItem*>( m_model->item( row ) );
-      keySet.insert( item->key() );
-    }
-  }
-
   for ( int row = start; row <= end; ++row ) {
     ExternalScriptItem* const item = static_cast<ExternalScriptItem*>( m_model->item( row ) );
 
-    int maxSuffix = 0;
-    QString keyCandidate = item->text() + QString::number( maxSuffix );
-    for (; keySet.contains( keyCandidate ); ++maxSuffix) {
-      keyCandidate = item->text() + QString::number( maxSuffix);
+    int nextSuffix = 0;
+    QString keyCandidate = keyCandidate = item->text() + QString::number( nextSuffix );
+    for ( ++nextSuffix; m_keySet.contains( keyCandidate ); ++nextSuffix ) {
+      keyCandidate = item->text() + QString::number( nextSuffix );
     }
 
     qCDebug(PLUGIN_EXTERNALSCRIPT) << "set key" << keyCandidate << "for" << item << item->command();
     item->setKey(keyCandidate);
-    keySet.insert(keyCandidate);
+    m_keySet.insert(keyCandidate);
   }
 }
 
diff --git a/plugins/externalscript/externalscriptplugin.h b/plugins/externalscript/externalscriptplugin.h
index 068efc2de7..4c7ed73d12 100644
--- a/plugins/externalscript/externalscriptplugin.h
+++ b/plugins/externalscript/externalscriptplugin.h
@@ -26,6 +26,7 @@
 #include <QVariantList>
 #include <KConfigGroup>
 #include <QUrl>
+#include <QSet>
 
 class ExternalScriptItem;
 
@@ -101,6 +102,7 @@ private:
 
   QStandardItemModel* m_model;
   QList<QUrl> m_urls;
+  QSet<QString> m_keySet;
   static ExternalScriptPlugin* m_self;
 
   class ExternalScriptViewFactory *m_factory;
flherne requested changes to this revision.Sep 28 2018, 11:34 AM

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!

plugins/externalscript/externalscriptplugin.cpp
392 ↗(On Diff #42320)

We don't need this at all, it's effectively a reimplementation of KConfigGroup::groupList().

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.

406 ↗(On Diff #42320)

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]
plugins/externalscript/externalscriptplugin.h
71

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

This revision now requires changes to proceed.Sep 28 2018, 11:34 AM
amhndu updated this revision to Diff 42481.Sep 28 2018, 2:03 PM
amhndu marked 3 inline comments as done.

Updated with requested changes.

amhndu marked an inline comment as done.Sep 28 2018, 2:04 PM
amhndu added inline comments.
plugins/externalscript/externalscriptplugin.cpp
392 ↗(On Diff #42320)

Thanks, wasn't aware of that function.

flherne accepted this revision.Sep 28 2018, 2:42 PM

The QSet include is redundant now.

Otherwise, looks good and still seems to work fine; with that tiny fix I think you can go ahead and push it.

plugins/externalscript/externalscriptplugin.cpp
52

This isn't needed anymore.

This revision is now accepted and ready to land.Sep 28 2018, 2:42 PM
This revision was automatically updated to reflect the committed changes.