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

Proposed by David Caro
Status: Merged
Merged at revision: 1430
Proposed branch: lp:~dcaro/terminator/fix-684340
Merge into: lp:terminator/trunk
Diff against target: 204 lines (+56/-39)
2 files modified
terminatorlib/config.py (+9/-0)
terminatorlib/plugins/custom_commands.py (+47/-39)
To merge this branch: bzr merge lp:~dcaro/terminator/fix-684340
Reviewer Review Type Date Requested Status
Stephen Boddy Abstain
Review via email: mp+87045@code.launchpad.net

Description of the change

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

To post a comment you must log in.
Revision history for this message
Stephen Boddy (stephen-j-boddy) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
393 """Set a whole config tree for a given plugin"""393 """Set a whole config tree for a given plugin"""
394 return(self.base.set_plugin(plugin, tree))394 return(self.base.set_plugin(plugin, tree))
395395
396 def plugin_del_config(self, plugin):
397 """Delete a whole config tree for a given plugin"""
398 return(self.base.del_plugin(plugin))
399
396 def layout_get_config(self, layout):400 def layout_get_config(self, layout):
397 """Return a layout"""401 """Return a layout"""
398 return(self.base.get_layout(layout))402 return(self.base.get_layout(layout))
@@ -670,6 +674,11 @@
670 """Set a whole tree for a plugin"""674 """Set a whole tree for a plugin"""
671 self.plugins[plugin] = tree675 self.plugins[plugin] = tree
672676
677 def del_plugin(self, plugin):
678 """Delete a whole tree for a plugin"""
679 if plugin in self.plugins:
680 del self.plugins[plugin]
681
673 def add_profile(self, profile):682 def add_profile(self, profile):
674 """Add a new profile"""683 """Add a new profile"""
675 if profile in self.profiles:684 if profile in self.profiles:
676685
=== 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 @@
23class CustomCommandsMenu(plugin.MenuItem):23class CustomCommandsMenu(plugin.MenuItem):
24 """Add custom commands to the terminal menu"""24 """Add custom commands to the terminal menu"""
25 capabilities = ['terminal_menu']25 capabilities = ['terminal_menu']
26 cmd_list = []26 cmd_list = {}
27 conf_file = os.path.join(get_config_dir(),"custom_commands")27 conf_file = os.path.join(get_config_dir(),"custom_commands")
2828
29 def __init__( self):29 def __init__( self):
30 config = Config()30 config = Config()
31 sections = config.plugin_get_config(self.__class__.__name__)31 sections = config.plugin_get_config(self.__class__.__name__)
32 if not isinstance(sections, dict):32 if not isinstance(sections, dict):
33 return33 return
34 for part in sections:34 noord_cmds = []
35 s = sections[part]35 for part in sections:
36 if not (s.has_key("name") and s.has_key("command")):36 s = sections[part]
37 print "CustomCommandsMenu: Ignoring section %s" % s37 if not (s.has_key("name") and s.has_key("command")):
38 continue38 print "CustomCommandsMenu: Ignoring section %s" % s
39 name = s["name"]39 continue
40 command = s["command"]40 name = s["name"]
41 enabled = s["enabled"] and s["enabled"] or False41 command = s["command"]
42 self.cmd_list.append(42 enabled = s["enabled"] and s["enabled"] or False
43 if s.has_key("position"):
44 self.cmd_list[int(s["position"])] = {'enabled' : enabled,
45 'name' : name,
46 'command' : command
47 }
48 else:
49 noord_cmds.append(
43 {'enabled' : enabled,50 {'enabled' : enabled,
44 'name' : name,51 'name' : name,
45 'command' : command52 'command' : command
46 }53 }
47 )54 )
55 for cmd in noord_cmds:
56 self.cmd_list[len(self.cmd_list)] = cmd
57
58
48 def callback(self, menuitems, menu, terminal):59 def callback(self, menuitems, menu, terminal):
49 """Add our menu items to the menu"""60 """Add our menu items to the menu"""
50 item = gtk.MenuItem(_('Custom Commands'))61 item = gtk.MenuItem(_('Custom Commands'))
@@ -61,7 +72,7 @@
61 submenu.append(menuitem)72 submenu.append(menuitem)
6273
63 theme = gtk.IconTheme()74 theme = gtk.IconTheme()
64 for command in self.cmd_list:75 for command in [ self.cmd_list[key] for key in sorted(self.cmd_list.keys()) ] :
65 if not command['enabled']:76 if not command['enabled']:
66 continue77 continue
67 exe = command['command'].split(' ')[0]78 exe = command['command'].split(' ')[0]
@@ -78,21 +89,22 @@
78 89
79 def _save_config(self):90 def _save_config(self):
80 config = Config()91 config = Config()
92 config.plugin_del_config(self.__class__.__name__)
81 i = 093 i = 0
82 length = len(self.cmd_list)94 for command in [ self.cmd_list[key] for key in sorted(self.cmd_list.keys()) ] :
83 while i < length:95 enabled = command['enabled']
84 enabled = self.cmd_list[i]['enabled']96 name = command['name']
85 name = self.cmd_list[i]['name']97 command = command['command']
86 command = self.cmd_list[i]['command']
87 98
88 item = {}99 item = {}
89 item['enabled'] = enabled100 item['enabled'] = enabled
90 item['name'] = name101 item['name'] = name
91 item['command'] = command102 item['command'] = command
103 item['position'] = i
92104
93 config.plugin_set(self.__class__.__name__, name, item)105 config.plugin_set(self.__class__.__name__, name, item)
94 config.save()
95 i = i + 1106 i = i + 1
107 config.save()
96108
97 def _execute(self, widget, data):109 def _execute(self, widget, data):
98 command = data['command']110 command = data['command']
@@ -113,7 +125,7 @@
113 )125 )
114 store = gtk.ListStore(bool, str, str)126 store = gtk.ListStore(bool, str, str)
115127
116 for command in self.cmd_list:128 for command in [ self.cmd_list[key] for key in sorted(self.cmd_list.keys()) ]:
117 store.append([command['enabled'], command['name'], command['command']])129 store.append([command['enabled'], command['name'], command['command']])
118 130
119 treeview = gtk.TreeView(store)131 treeview = gtk.TreeView(store)
@@ -183,30 +195,32 @@
183 button.set_sensitive(False)195 button.set_sensitive(False)
184 ui['button_delete'] = button196 ui['button_delete'] = button
185197
186
187
188 hbox.pack_start(button_box)198 hbox.pack_start(button_box)
189 dbox.show_all()199 dbox.show_all()
190 res = dbox.run()200 res = dbox.run()
201
191 if res == gtk.RESPONSE_ACCEPT:202 if res == gtk.RESPONSE_ACCEPT:
192 #we save the config203 self.update_cmd_list(store)
204 self._save_config()
205 dbox.destroy()
206 return
207
208
209 def update_cmd_list(self, store):
193 iter = store.get_iter_first()210 iter = store.get_iter_first()
194 self.cmd_list = []211 self.cmd_list = {}
212 i=0
195 while iter:213 while iter:
196 (enabled, name, command) = store.get(iter,214 (enabled, name, command) = store.get(iter,
197 CC_COL_ENABLED,215 CC_COL_ENABLED,
198 CC_COL_NAME,216 CC_COL_NAME,
199 CC_COL_COMMAND)217 CC_COL_COMMAND)
200 self.cmd_list.append(218 self.cmd_list[i] = {'enabled' : enabled,
201 {'enabled' : enabled,
202 'name': name,219 'name': name,
203 'command' : command}220 'command' : command}
204 )
205 iter = store.iter_next(iter)221 iter = store.iter_next(iter)
206 self._save_config()222 i = i + 1
207 223
208 dbox.destroy()
209 return
210224
211 def on_toggled(self, widget, path, data):225 def on_toggled(self, widget, path, data):
212 treeview = data['treeview']226 treeview = data['treeview']
@@ -218,10 +232,7 @@
218 CC_COL_COMMAND232 CC_COL_COMMAND
219 )233 )
220 store.set_value(iter, CC_COL_ENABLED, not enabled)234 store.set_value(iter, CC_COL_ENABLED, not enabled)
221 for cmd in self.cmd_list:235
222 if cmd['name'] == name:
223 cmd['enabled'] = not enabled
224 break
225236
226 def on_selection_changed(self,selection, data=None):237 def on_selection_changed(self,selection, data=None):
227 treeview = selection.get_tree_view()238 treeview = selection.get_tree_view()
@@ -376,17 +387,14 @@
376 (store, iter) = selection.get_selected()387 (store, iter) = selection.get_selected()
377 if iter:388 if iter:
378 store.remove(iter)389 store.remove(iter)
379
380 return390 return
381 391
382 def on_edit(self, button, data):392 def on_edit(self, button, data):
383 treeview = data['treeview']393 treeview = data['treeview']
384 selection = treeview.get_selection()394 selection = treeview.get_selection()
385 (store, iter) = selection.get_selected()395 (store, iter) = selection.get_selected()
386
387 if not iter:396 if not iter:
388 return397 return
389
390 (dialog,enabled,name,command) = self._create_command_dialog(398 (dialog,enabled,name,command) = self._create_command_dialog(
391 enabled_var = store.get_value(iter, CC_COL_ENABLED),399 enabled_var = store.get_value(iter, CC_COL_ENABLED),
392 name_var = store.get_value(iter, CC_COL_NAME),400 name_var = store.get_value(iter, CC_COL_NAME),