Merge lp:~dcaro/terminator/fix-684340 into lp:terminator/trunk

[David Caro]

Hi,
I've added a delete method for the plugins config to enable 'cleaning' the plugins configuration before saving (maybe it would be better to add a key delete method to avoid deleting the entire config each time).

And added a 'position' field for each command config, so you can keep track of the order when loading the commands (maybe the config should keep track of the order of the properties so this 'position' field is not needed?).

David

[Stephen Boddy (stephen-j-boddy)]

Hi David. A couple of comments:
1) You have unnecessary random line inserts and removals, i.e. lines 136-137, 141, 191, 198, & 201 particularly stand out, but there may be more I missed
2) The chunk of lines 40-72 a significant chuck of that is actually identical, but your changes have unnecessarily altered indentation.
3) It would be preferable if the two items (fix ordering, and fix deletion) were two seperate commits. Makes it easier to see whats going on.

Other than that the patch works and fixes the issue. I'm marking as abstain for now. I won't merge this directly, but when I find time I'll clean it up and place it into trunk.

review: Abstain

=== modified file 'terminatorlib/config.py'
--- terminatorlib/config.py	2011-10-07 00:20:54 +0000
+++ terminatorlib/config.py	2011-12-28 22:24:30 +0000
@@ -393,6 +393,10 @@
         """Set a whole config tree for a given plugin"""
         return(self.base.set_plugin(plugin, tree))
 
+    def plugin_del_config(self, plugin):
+        """Delete a whole config tree for a given plugin"""
+        return(self.base.del_plugin(plugin))
+
     def layout_get_config(self, layout):
         """Return a layout"""
         return(self.base.get_layout(layout))
@@ -670,6 +674,11 @@
         """Set a whole tree for a plugin"""
         self.plugins[plugin] = tree
 
+    def del_plugin(self, plugin):
+        """Delete a whole tree for a plugin"""
+        if plugin in self.plugins:
+            del self.plugins[plugin]
+
     def add_profile(self, profile):
         """Add a new profile"""
         if profile in self.profiles:

=== modified file 'terminatorlib/plugins/custom_commands.py'
--- terminatorlib/plugins/custom_commands.py	2010-06-10 15:56:17 +0000
+++ terminatorlib/plugins/custom_commands.py	2011-12-28 22:24:30 +0000
@@ -23,28 +23,39 @@
 class CustomCommandsMenu(plugin.MenuItem):
     """Add custom commands to the terminal menu"""
     capabilities = ['terminal_menu']
-    cmd_list = []
+    cmd_list = {}
     conf_file = os.path.join(get_config_dir(),"custom_commands")
 
     def __init__( self):
-      config = Config()
-      sections = config.plugin_get_config(self.__class__.__name__)
-      if not isinstance(sections, dict):
-          return
-      for part in sections:
-        s = sections[part]
-        if not (s.has_key("name") and s.has_key("command")):
-          print "CustomCommandsMenu: Ignoring section %s" % s
-          continue
-        name = s["name"]
-        command = s["command"]
-        enabled = s["enabled"] and s["enabled"] or False
-        self.cmd_list.append(
+        config = Config()
+        sections = config.plugin_get_config(self.__class__.__name__)
+        if not isinstance(sections, dict):
+            return
+        noord_cmds = []
+        for part in sections:
+            s = sections[part]
+            if not (s.has_key("name") and s.has_key("command")):
+                print "CustomCommandsMenu: Ignoring section %s" % s
+                continue
+            name = s["name"]
+            command = s["command"]
+            enabled = s["enabled"] and s["enabled"] or False
+            if s.has_key("position"):
+                self.cmd_list[int(s["position"])] = {'enabled' : enabled,
+                                            'name' : name,
+                                            'command' : command
+                                            }
+            else:
+                noord_cmds.append(
                               {'enabled' : enabled,
                                 'name' : name,
                                 'command' : command
                               }
                             )
+        for cmd in noord_cmds:
+            self.cmd_list[len(self.cmd_list)] = cmd
+
+      
     def callback(self, menuitems, menu, terminal):
         """Add our menu items to the menu"""
         item = gtk.MenuItem(_('Custom Commands'))
@@ -61,7 +72,7 @@
         submenu.append(menuitem)
 
         theme = gtk.IconTheme()
-        for command in self.cmd_list:
+        for command in [ self.cmd_list[key] for key in sorted(self.cmd_list.keys()) ] :
           if not command['enabled']:
             continue
           exe = command['command'].split(' ')[0]
@@ -78,21 +89,22 @@
         
     def _save_config(self):
       config = Config()
+      config.plugin_del_config(self.__class__.__name__)
       i = 0
-      length = len(self.cmd_list)
-      while i < length:
-        enabled = self.cmd_list[i]['enabled']
-        name = self.cmd_list[i]['name']
-        command = self.cmd_list[i]['command']
+      for command in [ self.cmd_list[key] for key in sorted(self.cmd_list.keys()) ] :
+        enabled = command['enabled']
+        name = command['name']
+        command = command['command']
        
         item = {}
         item['enabled'] = enabled
         item['name'] = name
         item['command'] = command
+        item['position'] = i
 
         config.plugin_set(self.__class__.__name__, name, item)
-        config.save()
         i = i + 1
+      config.save()
 
     def _execute(self, widget, data):
       command = data['command']
@@ -113,7 +125,7 @@
                     )
       store = gtk.ListStore(bool, str, str)
 
-      for command in self.cmd_list:
+      for command in [ self.cmd_list[key] for key in sorted(self.cmd_list.keys()) ]:
         store.append([command['enabled'], command['name'], command['command']])
  
       treeview = gtk.TreeView(store)
@@ -183,30 +195,32 @@
       button.set_sensitive(False)
       ui['button_delete'] = button
 
-
-
       hbox.pack_start(button_box)
       dbox.show_all()
       res = dbox.run()
+
       if res == gtk.RESPONSE_ACCEPT:
-        #we save the config
+        self.update_cmd_list(store)
+        self._save_config()
+      dbox.destroy()
+      return
+
+
+    def update_cmd_list(self, store):
         iter = store.get_iter_first()
-        self.cmd_list = []
+        self.cmd_list = {}
+        i=0
         while iter:
           (enabled, name, command) = store.get(iter,
                                               CC_COL_ENABLED,
                                               CC_COL_NAME,
                                               CC_COL_COMMAND)
-          self.cmd_list.append(
-                            {'enabled' : enabled,
+          self.cmd_list[i] = {'enabled' : enabled,
                             'name': name,
                             'command' : command}
-                              )
           iter = store.iter_next(iter)
-        self._save_config()
+          i = i + 1
       
-      dbox.destroy()
-      return
 
     def on_toggled(self, widget, path, data):
       treeview = data['treeview']
@@ -218,10 +232,7 @@
                                     CC_COL_COMMAND
                                         )
       store.set_value(iter, CC_COL_ENABLED, not enabled)
-      for cmd in self.cmd_list:
-        if cmd['name'] == name:
-          cmd['enabled'] = not enabled
-          break
+
 
     def on_selection_changed(self,selection, data=None):
       treeview = selection.get_tree_view()
@@ -376,17 +387,14 @@
       (store, iter) = selection.get_selected()
       if iter:
         store.remove(iter)
-      
       return
  
     def on_edit(self, button, data):
       treeview = data['treeview']
       selection = treeview.get_selection()
       (store, iter) = selection.get_selected()
-      
       if not iter:
         return
-       
       (dialog,enabled,name,command) = self._create_command_dialog(
                                                 enabled_var = store.get_value(iter, CC_COL_ENABLED),
                                                 name_var = store.get_value(iter, CC_COL_NAME),