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

Proposed by David Caro on 2011-12-28
Status: Merged
Merged at revision: 1430
Proposed branch: lp:~dcaro/terminator/fix-684340
Merge into: lp:terminator
Diff against target: 204 lines (+56/-39) 2 files modified
To merge this branch: bzr merge lp:~dcaro/terminator/fix-684340
Reviewer Review Type Date Requested Status
Stephen Boddy 2011-12-28 Abstain on 2013-07-16
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.
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

1=== modified file 'terminatorlib/config.py'
2--- terminatorlib/config.py 2011-10-07 00:20:54 +0000
3+++ terminatorlib/config.py 2011-12-28 22:24:30 +0000
4@@ -393,6 +393,10 @@
5 """Set a whole config tree for a given plugin"""
6 return(self.base.set_plugin(plugin, tree))
7
8+ def plugin_del_config(self, plugin):
9+ """Delete a whole config tree for a given plugin"""
10+ return(self.base.del_plugin(plugin))
11+
12 def layout_get_config(self, layout):
13 """Return a layout"""
14 return(self.base.get_layout(layout))
15@@ -670,6 +674,11 @@
16 """Set a whole tree for a plugin"""
17 self.plugins[plugin] = tree
18
19+ def del_plugin(self, plugin):
20+ """Delete a whole tree for a plugin"""
21+ if plugin in self.plugins:
22+ del self.plugins[plugin]
23+
24 def add_profile(self, profile):
25 """Add a new profile"""
26 if profile in self.profiles:
27
28=== modified file 'terminatorlib/plugins/custom_commands.py'
29--- terminatorlib/plugins/custom_commands.py 2010-06-10 15:56:17 +0000
30+++ terminatorlib/plugins/custom_commands.py 2011-12-28 22:24:30 +0000
31@@ -23,28 +23,39 @@
32 class CustomCommandsMenu(plugin.MenuItem):
33 """Add custom commands to the terminal menu"""
34 capabilities = ['terminal_menu']
35- cmd_list = []
36+ cmd_list = {}
37 conf_file = os.path.join(get_config_dir(),"custom_commands")
38
39 def __init__( self):
40- config = Config()
41- sections = config.plugin_get_config(self.__class__.__name__)
42- if not isinstance(sections, dict):
43- return
44- for part in sections:
45- s = sections[part]
46- if not (s.has_key("name") and s.has_key("command")):
47- print "CustomCommandsMenu: Ignoring section %s" % s
48- continue
49- name = s["name"]
50- command = s["command"]
51- enabled = s["enabled"] and s["enabled"] or False
52- self.cmd_list.append(
53+ config = Config()
54+ sections = config.plugin_get_config(self.__class__.__name__)
55+ if not isinstance(sections, dict):
56+ return
57+ noord_cmds = []
58+ for part in sections:
59+ s = sections[part]
60+ if not (s.has_key("name") and s.has_key("command")):
61+ print "CustomCommandsMenu: Ignoring section %s" % s
62+ continue
63+ name = s["name"]
64+ command = s["command"]
65+ enabled = s["enabled"] and s["enabled"] or False
66+ if s.has_key("position"):
67+ self.cmd_list[int(s["position"])] = {'enabled' : enabled,
68+ 'name' : name,
69+ 'command' : command
70+ }
71+ else:
72+ noord_cmds.append(
73 {'enabled' : enabled,
74 'name' : name,
75 'command' : command
76 }
77 )
78+ for cmd in noord_cmds:
79+ self.cmd_list[len(self.cmd_list)] = cmd
80+
81+
82 def callback(self, menuitems, menu, terminal):
83 """Add our menu items to the menu"""
84 item = gtk.MenuItem(_('Custom Commands'))
85@@ -61,7 +72,7 @@
86 submenu.append(menuitem)
87
88 theme = gtk.IconTheme()
89- for command in self.cmd_list:
90+ for command in [ self.cmd_list[key] for key in sorted(self.cmd_list.keys()) ] :
91 if not command['enabled']:
92 continue
93 exe = command['command'].split(' ')[0]
94@@ -78,21 +89,22 @@
95
96 def _save_config(self):
97 config = Config()
98+ config.plugin_del_config(self.__class__.__name__)
99 i = 0
100- length = len(self.cmd_list)
101- while i < length:
102- enabled = self.cmd_list[i]['enabled']
103- name = self.cmd_list[i]['name']
104- command = self.cmd_list[i]['command']
105+ for command in [ self.cmd_list[key] for key in sorted(self.cmd_list.keys()) ] :
106+ enabled = command['enabled']
107+ name = command['name']
108+ command = command['command']
109
110 item = {}
111 item['enabled'] = enabled
112 item['name'] = name
113 item['command'] = command
114+ item['position'] = i
115
116 config.plugin_set(self.__class__.__name__, name, item)
117- config.save()
118 i = i + 1
119+ config.save()
120
121 def _execute(self, widget, data):
122 command = data['command']
123@@ -113,7 +125,7 @@
124 )
125 store = gtk.ListStore(bool, str, str)
126
127- for command in self.cmd_list:
128+ for command in [ self.cmd_list[key] for key in sorted(self.cmd_list.keys()) ]:
129 store.append([command['enabled'], command['name'], command['command']])
130
131 treeview = gtk.TreeView(store)
132@@ -183,30 +195,32 @@
133 button.set_sensitive(False)
134 ui['button_delete'] = button
135
136-
137-
138 hbox.pack_start(button_box)
139 dbox.show_all()
140 res = dbox.run()
141+
142 if res == gtk.RESPONSE_ACCEPT:
143- #we save the config
144+ self.update_cmd_list(store)
145+ self._save_config()
146+ dbox.destroy()
147+ return
148+
149+
150+ def update_cmd_list(self, store):
151 iter = store.get_iter_first()
152- self.cmd_list = []
153+ self.cmd_list = {}
154+ i=0
155 while iter:
156 (enabled, name, command) = store.get(iter,
157 CC_COL_ENABLED,
158 CC_COL_NAME,
159 CC_COL_COMMAND)
160- self.cmd_list.append(
161- {'enabled' : enabled,
162+ self.cmd_list[i] = {'enabled' : enabled,
163 'name': name,
164 'command' : command}
165- )
166 iter = store.iter_next(iter)
167- self._save_config()
168+ i = i + 1
169
170- dbox.destroy()
171- return
172
173 def on_toggled(self, widget, path, data):
174 treeview = data['treeview']
175@@ -218,10 +232,7 @@
176 CC_COL_COMMAND
177 )
178 store.set_value(iter, CC_COL_ENABLED, not enabled)
179- for cmd in self.cmd_list:
180- if cmd['name'] == name:
181- cmd['enabled'] = not enabled
182- break
183+
184
185 def on_selection_changed(self,selection, data=None):
186 treeview = selection.get_tree_view()
187@@ -376,17 +387,14 @@
188 (store, iter) = selection.get_selected()
189 if iter:
190 store.remove(iter)
191-
192 return
193
194 def on_edit(self, button, data):
195 treeview = data['treeview']
196 selection = treeview.get_selection()
197 (store, iter) = selection.get_selected()
198-
199 if not iter:
200 return
201-
202 (dialog,enabled,name,command) = self._create_command_dialog(
203 enabled_var = store.get_value(iter, CC_COL_ENABLED),
204 name_var = store.get_value(iter, CC_COL_NAME),

Subscribers

People subscribed via source and target branches

to status/vote changes: