Status: | Rejected |
---|---|
Rejected by: | Alex Launi |
Proposed branch: | lp:~cszikszoy/do/config-MA-ext |
Merge into: | lp:do |
Diff against target: | None lines |
To merge this branch: | bzr merge lp:~cszikszoy/do/config-MA-ext |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Alex Launi (community) | Approve | ||
Robert Dyer (community) | Needs Fixing | ||
Review via email: mp+7700@code.launchpad.net |
Commit message
Description of the change
Chris S. (cszikszoy) wrote : | # |
Chris Halse Rogers (raof) wrote : | # |
What are the benefits here? I presume this makes it easier for plugins to provide environment-
Robert Dyer (psybers) wrote : | # |
One good use case for this: Docklets. Right now, if a docklet wants to have a config page it needs to also provide an ItemSource (as Weather is doing). We could extend to look for IConfigurable on docklets, but this solution is much more robust.
Chris S. (cszikszoy) wrote : | # |
Aside from what Robert already stated, there is a benefit here for plugins that want more than one config page, but only have one ItemSource or Action. A good example of this is the Flickr plugin. This plugin consists of only a single Action, yet an entirely empty ItemSource was also made for the sole purpose of implementing IConfigurable in order to have two config pages.
Also, as you stated, Do.Platform.* can now provide an extension point for configurables. Say we decide to move away from GTK and do everything in an environment-
Alex Launi (alexlauni) wrote : | # |
Yeah, this happens in a few plugins as well. This is a patch I've been
meaning to write for a year. thanks chris
--
--Alex Launi
Robert Dyer (psybers) wrote : | # |
All line #s refer to the merge diff.
1. Line 95, too many tabs I believe
2. Line 92, you should lift 'Addin.GetIdName (id)' out of the LINQ statement as it only needs evaluated once (the compiler probably is smart enough to figure this out on its own, but it also makes the code easier to read)
3. Also, can we not just match on the ID? Why are we matching on names?
4. Why are we calling notebook.
Chris S. (cszikszoy) wrote : | # |
> 1. Line 95, too many tabs I believe
- fixed
> 2. Line 92, you should lift 'Addin.GetIdName (id)' out of the LINQ statement
> as it only needs evaluated once (the compiler probably is smart enough to
> figure this out on its own, but it also makes the code easier to read)
- See next comment
> 3. Also, can we not just match on the ID? Why are we matching on names?
- This might not make sense, but here goes: M.A. is strange. Let's start with this. This method (ConfigurablesF
So, that LINQ query returns a set of TypeExtensionNodes, but n.addin.id will return "Do.File".
Long story short, maybe id is a bad variable name, because the "id" coming from another place, isn't what the M.A. "id" really is. Either way, I just copied that comparrison code from somewhere else in that file (line 281 of PluginManager.cs). After experimenting a bit, I found that you can actually compare with ".Where (n => n.Addin.Id == Addin.GetIdName (id))", so I've reduced it to that. And, I've also updated the old code (line 281) to reflect this change as well.
> 4. Why are we calling notebook.
> window re-used somewhere (that's the only reason I can see for removing the
> pages, since the Build() will just create a new notebook each time a new
> Window is created).
As for this... I honestly have no idea what's going on here. All I know is that if you remove these lines Do blows up hard (in native code) when you open a plugin's preferences window twice. The only thing I found relevant was that the notebook.Add () method is actually taking a reference to the widgets or notebook pages. Something about how I'm grabbing the IConfigurable classes must be creating those classes in memory, and then they get added fine the first time, but on the 2nd time there's a nasty GTK error saying you can't set the parent of a widget that already has a parent. My thinking is that notebook.Add the 2nd time is still referring to those original widgets that got created the first time but still exist somewhere in managed memory. When you try to add those widgets to the notebook, GTK tries to set the parent for those widgets a 2nd time and Do blows up here. I don't know if I'm just rambling at this point or if that actually made any sense. But again, I have no idea why that's happening, but that's my best guess. Feel free to remove those lines and see for yourself, or if you have any ideas or suggestions that would be helpful too.
Alex Launi (alexlauni) wrote : | # |
Looks pretty good to me, just a couple of things I noticed
line 91 (of the diff) Gets an IEnumerable and then loops over it, you can probably do this whole thing with a single linq statement and then return the IEnumerable LINQ gives you. I think it'd be cleaner and possibly a small bit faster.
Also, this means we need to update the plugins, I'd like an approved plugins merge before we merge this one.
Alex Launi (alexlauni) wrote : | # |
PluginManager.cs: ObjectsForAddin<T>, you made a string idName var; put it back the way it was, you only use it once there's no point in assigning it way ahead of time. Also, maybe we should define ExtensionPaths as a Dictionary, so that in the class we can refer to the paths with human names instead of hardcoding the paths in a bunch of places?
Other than that, looks good. +1
Robert Dyer (psybers) wrote : | # |
I still won't approve until we have a merge request fixing do-plugins.
Alex Launi (alexlauni) wrote : | # |
I will approve, I just won't merge.
--
-- Alex Launi
Chris Halse Rogers (raof) wrote : | # |
Is there any particular reason why this hasn't got merged? It still seems like something that's reasonable to have, even if it might get somewhat reworked by 0.9.
Alex Launi (alexlauni) wrote : | # |
I think plugins were why this never got merged, but I dont remember
On Fri, Apr 2, 2010 at 3:35 AM, Chris Halse Rogers <email address hidden> wrote:
> Is there any particular reason why this hasn't got merged? It still seems
> like something that's reasonable to have, even if it might get somewhat
> reworked by 0.9.
> --
> https:/
> You are reviewing the proposed merge of lp:~cszikszoy/do/config-MA-ext into
> lp:do.
>
--
--Alex Launi
Unmerged revisions
- 1242. By Chris S.
-
merge w/ trunk
- 1241. By Chris S.
-
lift statement out of loops
- 1240. By Chris S.
-
add FIXME comment
- 1239. By Chris S.
-
change extension point to /Do/Configuration
- 1238. By Chris S.
-
LINQ query & indentation fixes as per review
- 1237. By Chris S.
-
remove MA requirement for Do.Universe
- 1236. By Chris S.
-
merge trunk
- 1235. By Chris S.
-
more fixes
- 1234. By Chris S.
-
remove property
- 1233. By Chris S.
-
fix crash when opening preference window twice
Preview Diff
1 | === modified file 'Do.Platform.Linux/Resources/Do.Platform.Linux.addin.xml' |
2 | --- Do.Platform.Linux/Resources/Do.Platform.Linux.addin.xml 2009-06-10 05:30:30 +0000 |
3 | +++ Do.Platform.Linux/Resources/Do.Platform.Linux.addin.xml 2009-06-20 02:35:45 +0000 |
4 | @@ -32,4 +32,8 @@ |
5 | <Service type="Do.Platform.Linux.GnomeKeyringSecurePreferencesService" /> |
6 | </Extension> |
7 | |
8 | + <ExtensionPoint path="/Do/Config"> |
9 | + <ExtensionNode name="Config" objectType="Do.Platform.Linux.IConfigurable"/> |
10 | + </ExtensionPoint> |
11 | + |
12 | </Addin> |
13 | |
14 | === modified file 'Do.Platform.Linux/gtk-gui/gui.stetic' |
15 | --- Do.Platform.Linux/gtk-gui/gui.stetic 2009-05-29 10:26:49 +0000 |
16 | +++ Do.Platform.Linux/gtk-gui/gui.stetic 2009-06-19 20:22:55 +0000 |
17 | @@ -7,7 +7,6 @@ |
18 | <import> |
19 | <widget-library name="gnomedesktop-sharp, Version=2.20.0.0, Culture=neutral, PublicKeyToken=35e10195dab3c99f" /> |
20 | <widget-library name="notify-sharp, Version=0.4.0.0, Culture=neutral, PublicKeyToken=2df29c54e245917a" /> |
21 | - <widget-library name="../../Do.Interface.Linux/bin/Debug/Do.Interface.Linux.dll" /> |
22 | <widget-library name="gnome-sharp, Version=2.24.0.0, Culture=neutral, PublicKeyToken=35e10195dab3c99f" /> |
23 | <widget-library name="../bin/Debug/Do.Platform.Linux.dll" internal="true" /> |
24 | </import> |
25 | |
26 | === modified file 'Do.Platform.Linux/src/Do.Platform/Do.Platform.Linux/IConfigurable.cs' |
27 | --- Do.Platform.Linux/src/Do.Platform/Do.Platform.Linux/IConfigurable.cs 2008-12-21 23:10:16 +0000 |
28 | +++ Do.Platform.Linux/src/Do.Platform/Do.Platform.Linux/IConfigurable.cs 2009-06-19 02:19:46 +0000 |
29 | @@ -25,10 +25,7 @@ |
30 | { |
31 | public interface IConfigurable |
32 | { |
33 | - string Name { get; } |
34 | - string Description { get; } |
35 | - string Icon { get; } |
36 | - |
37 | - Bin GetConfiguration (); |
38 | + string Title { get; } |
39 | + Bin GetConfiguration (); |
40 | } |
41 | } |
42 | |
43 | === modified file 'Do/gtk-gui/objects.xml' |
44 | --- Do/gtk-gui/objects.xml 2009-06-06 18:05:52 +0000 |
45 | +++ Do/gtk-gui/objects.xml 2009-06-19 02:19:46 +0000 |
46 | @@ -1,12 +1,13 @@ |
47 | <objects attr-sync="on"> |
48 | + <object type="Do.UI.GeneralPreferencesWidget" palette-category="Do" allow-children="false" base-type="Gtk.Bin"> |
49 | + <itemgroups> |
50 | + </itemgroups> |
51 | + <signals /> |
52 | + </object> |
53 | <object type="Do.UI.KeybindingsPreferencesWidget" palette-category="Do" allow-children="false" base-type="Gtk.Bin"> |
54 | <itemgroups /> |
55 | <signals /> |
56 | </object> |
57 | - <object type="Do.UI.GeneralPreferencesWidget" palette-category="Do" allow-children="false" base-type="Gtk.Bin"> |
58 | - <itemgroups /> |
59 | - <signals /> |
60 | - </object> |
61 | <object type="Do.UI.ManagePluginsPreferencesWidget" palette-category="Do" allow-children="false" base-type="Gtk.Bin"> |
62 | <itemgroups /> |
63 | <signals /> |
64 | |
65 | === modified file 'Do/src/Do.Core/PluginManager.cs' |
66 | --- Do/src/Do.Core/PluginManager.cs 2009-06-05 19:45:45 +0000 |
67 | +++ Do/src/Do.Core/PluginManager.cs 2009-06-20 02:35:37 +0000 |
68 | @@ -46,7 +46,7 @@ |
69 | { |
70 | const string DefaultPluginIcon = "folder_tar"; |
71 | |
72 | - static IEnumerable<string> ExtensionPaths = new [] { "/Do/ItemSource", "/Do/Action" }; |
73 | + static IEnumerable<string> ExtensionPaths = new [] { "/Do/ItemSource", "/Do/Action", "/Do/Config" }; |
74 | |
75 | public static readonly IEnumerable<AddinClassifier> Classifiers = |
76 | new AddinClassifier [] { |
77 | @@ -80,7 +80,7 @@ |
78 | // Initialize services before addins that may use them are loaded. |
79 | Services.Initialize (); |
80 | InterfaceManager.Initialize (); |
81 | - |
82 | + |
83 | // Now allow loading of non-services. |
84 | foreach (string path in ExtensionPaths) |
85 | AddinManager.AddExtensionNodeHandler (path, OnPluginChanged); |
86 | @@ -297,12 +297,19 @@ |
87 | /// </returns> |
88 | public static IEnumerable<IConfigurable> ConfigurablesForAddin (string id) |
89 | { |
90 | - return ObjectsForAddin<IConfigurable> (id); |
91 | + IEnumerable<TypeExtensionNode> configs = AddinManager.GetExtensionNodes ("/Do/Config") |
92 | + .OfType <TypeExtensionNode> ().Where (n => Addin.GetIdName (n.Addin.Id) == Addin.GetIdName (id)); |
93 | + |
94 | + foreach (TypeExtensionNode page in configs) { |
95 | + yield return page.GetInstance () as IConfigurable; |
96 | + } |
97 | + |
98 | + yield break; |
99 | } |
100 | - |
101 | + |
102 | static string AddinIdWithoutVersion (string id) |
103 | { |
104 | return id.Substring (0, id.IndexOf (',')); |
105 | } |
106 | } |
107 | -} |
108 | +} |
109 | \ No newline at end of file |
110 | |
111 | === modified file 'Do/src/Do.UI/ColorConfigurationWidget.cs' |
112 | --- Do/src/Do.UI/ColorConfigurationWidget.cs 2009-04-21 02:15:39 +0000 |
113 | +++ Do/src/Do.UI/ColorConfigurationWidget.cs 2009-06-19 02:19:46 +0000 |
114 | @@ -44,6 +44,7 @@ |
115 | public ColorConfigurationWidget () |
116 | { |
117 | Build (); |
118 | + |
119 | AppPaintable = true; |
120 | Themes = new List<string> (); |
121 | Interface.Util.Appearance.SetColormap (this); |
122 | @@ -57,7 +58,6 @@ |
123 | composite_warning_widget.Visible = true; |
124 | theme_combo.Sensitive = false; |
125 | } |
126 | - |
127 | // Setup theme combo |
128 | theme_combo.Active = Math.Max (0, Themes.IndexOf (Do.Preferences.Theme)); |
129 | pin_check.Active = Do.Preferences.AlwaysShowResults; |
130 | @@ -65,6 +65,10 @@ |
131 | theme_configuration_container.ShowAll (); |
132 | } |
133 | |
134 | + public string Title { |
135 | + get { return Catalog.GetString ("Appearance"); } |
136 | + } |
137 | + |
138 | public Gtk.Bin GetConfiguration () |
139 | { |
140 | return this; |
141 | @@ -86,11 +90,13 @@ |
142 | if (theme_configuration_container.Child != null) |
143 | theme_configuration_container.Remove (theme_configuration_container.Child); |
144 | |
145 | + |
146 | if (Do.Controller.Window is IConfigurable) { |
147 | IConfigurable window = Do.Controller.Window as IConfigurable; |
148 | Gtk.Bin bin = window.GetConfiguration (); |
149 | theme_configuration_container.Add (bin); |
150 | } |
151 | + |
152 | theme_configuration_container.ShowAll (); |
153 | } |
154 | |
155 | @@ -98,18 +104,6 @@ |
156 | { |
157 | } |
158 | |
159 | - public new string Name { |
160 | - get { return Catalog.GetString ("Appearance"); } |
161 | - } |
162 | - |
163 | - public string Description { |
164 | - get { return ""; } |
165 | - } |
166 | - |
167 | - public string Icon { |
168 | - get { return ""; } |
169 | - } |
170 | - |
171 | public override void Dispose () |
172 | { |
173 | if (theme_configuration_container.Child != null) |
174 | |
175 | === modified file 'Do/src/Do.UI/GeneralPreferencesWidget.cs' |
176 | --- Do/src/Do.UI/GeneralPreferencesWidget.cs 2009-02-18 00:31:48 +0000 |
177 | +++ Do/src/Do.UI/GeneralPreferencesWidget.cs 2009-06-19 02:19:46 +0000 |
178 | @@ -33,39 +33,32 @@ |
179 | |
180 | namespace Do.UI |
181 | { |
182 | - [System.ComponentModel.Category("Do")] |
183 | - [System.ComponentModel.ToolboxItem(true)] |
184 | - public partial class GeneralPreferencesWidget : Bin, IConfigurable |
185 | - { |
186 | - new public string Name { |
187 | - get { return Catalog.GetString ("General"); } |
188 | - } |
189 | - |
190 | - public string Description { |
191 | - get { return ""; } |
192 | - } |
193 | - |
194 | - public string Icon { |
195 | - get { return ""; } |
196 | - } |
197 | + [System.ComponentModel.Category("Do")] |
198 | + [System.ComponentModel.ToolboxItem(true)] |
199 | + public partial class GeneralPreferencesWidget : Bin, IConfigurable |
200 | + { |
201 | |
202 | public GeneralPreferencesWidget () |
203 | { |
204 | - Build (); |
205 | - |
206 | - // Setup checkboxes |
207 | - hide_check.Active = Do.Preferences.QuietStart; |
208 | - login_check.Active = AutostartEnabled; |
209 | - notification_check.Active = TrayIconService.Visible; |
210 | - } |
211 | - |
212 | + Build (); |
213 | + |
214 | + // Setup checkboxes |
215 | + hide_check.Active = Do.Preferences.QuietStart; |
216 | + login_check.Active = AutostartEnabled; |
217 | + notification_check.Active = TrayIconService.Visible; |
218 | + } |
219 | + |
220 | + public string Title { |
221 | + get { return Catalog.GetString ("General"); } |
222 | + } |
223 | + |
224 | public Bin GetConfiguration () |
225 | { |
226 | - return this; |
227 | + return this; |
228 | } |
229 | - |
230 | + |
231 | protected bool AutostartEnabled { |
232 | - get { |
233 | + get { |
234 | return Services.System.IsAutoStartEnabled (); |
235 | } |
236 | |
237 | @@ -80,17 +73,17 @@ |
238 | |
239 | protected virtual void OnLoginCheckClicked (object sender, EventArgs e) |
240 | { |
241 | - AutostartEnabled = login_check.Active; |
242 | + AutostartEnabled = login_check.Active; |
243 | } |
244 | |
245 | protected virtual void OnHideCheckClicked (object sender, EventArgs e) |
246 | { |
247 | - Do.Preferences.QuietStart = hide_check.Active; |
248 | + Do.Preferences.QuietStart = hide_check.Active; |
249 | } |
250 | |
251 | protected virtual void OnNotificationCheckClicked (object sender, System.EventArgs e) |
252 | { |
253 | - TrayIconService.Visible = notification_check.Active; |
254 | + TrayIconService.Visible = notification_check.Active; |
255 | } |
256 | - } |
257 | -} |
258 | + } |
259 | +} |
260 | \ No newline at end of file |
261 | |
262 | === modified file 'Do/src/Do.UI/KeybindingsPreferencesWidget.cs' |
263 | --- Do/src/Do.UI/KeybindingsPreferencesWidget.cs 2009-01-27 05:08:07 +0000 |
264 | +++ Do/src/Do.UI/KeybindingsPreferencesWidget.cs 2009-06-19 02:19:46 +0000 |
265 | @@ -36,18 +36,6 @@ |
266 | { |
267 | private KeybindingTreeView kbview; |
268 | |
269 | - new public string Name { |
270 | - get { return Catalog.GetString ("Keyboard"); } |
271 | - } |
272 | - |
273 | - public string Description { |
274 | - get { return ""; } |
275 | - } |
276 | - |
277 | - public string Icon { |
278 | - get { return ""; } |
279 | - } |
280 | - |
281 | public KeybindingsPreferencesWidget () |
282 | { |
283 | Build (); |
284 | @@ -58,9 +46,13 @@ |
285 | action_scroll.ShowAll (); |
286 | } |
287 | |
288 | + public string Title { |
289 | + get { return Catalog.GetString ("Keyboard"); } |
290 | + } |
291 | + |
292 | public Bin GetConfiguration () |
293 | - { |
294 | - return this; |
295 | - } |
296 | + { |
297 | + return this; |
298 | + } |
299 | } |
300 | -} |
301 | +} |
302 | \ No newline at end of file |
303 | |
304 | === modified file 'Do/src/Do.UI/ManagePluginsPreferencesWidget.cs' |
305 | --- Do/src/Do.UI/ManagePluginsPreferencesWidget.cs 2009-06-05 19:45:45 +0000 |
306 | +++ Do/src/Do.UI/ManagePluginsPreferencesWidget.cs 2009-06-19 02:19:46 +0000 |
307 | @@ -50,18 +50,6 @@ |
308 | PluginNodeView nview; |
309 | SearchEntry search_entry; |
310 | |
311 | - new public string Name { |
312 | - get { return Catalog.GetString ("Plugins"); } |
313 | - } |
314 | - |
315 | - public string Description { |
316 | - get { return ""; } |
317 | - } |
318 | - |
319 | - public string Icon { |
320 | - get { return ""; } |
321 | - } |
322 | - |
323 | public ManagePluginsPreferencesWidget () |
324 | { |
325 | Build (); |
326 | @@ -98,6 +86,15 @@ |
327 | Services.Application.RunOnMainThread (() => search_entry.InnerEntry.GrabFocus ()); |
328 | } |
329 | |
330 | + public string Title { |
331 | + get { return Catalog.GetString ("Plugins"); } |
332 | + } |
333 | + |
334 | + public Bin GetConfiguration () |
335 | + { |
336 | + return this; |
337 | + } |
338 | + |
339 | protected void OnDragDataReceived (object sender, DragDataReceivedArgs args) |
340 | { |
341 | string data; |
342 | @@ -144,11 +141,6 @@ |
343 | } |
344 | } |
345 | |
346 | - public Bin GetConfiguration () |
347 | - { |
348 | - return this; |
349 | - } |
350 | - |
351 | private void OnPluginSelected (object sender, PluginSelectionEventArgs e) |
352 | { |
353 | UpdateButtonState (); |
354 | |
355 | === modified file 'Do/src/Do.UI/PluginConfigurationWindow.cs' |
356 | --- Do/src/Do.UI/PluginConfigurationWindow.cs 2008-12-21 23:34:14 +0000 |
357 | +++ Do/src/Do.UI/PluginConfigurationWindow.cs 2009-06-19 20:29:23 +0000 |
358 | @@ -37,18 +37,20 @@ |
359 | |
360 | const string TitleMarkup = |
361 | "<span weight=\"heavy\" size=\"large\">{0}</span>"; |
362 | - |
363 | + |
364 | public PluginConfigurationWindow (string id) : |
365 | base (WindowType.Toplevel) |
366 | { |
367 | Addin addin; |
368 | + |
369 | + Build (); |
370 | + |
371 | IEnumerable<IConfigurable> configs; |
372 | |
373 | - Build (); |
374 | - |
375 | addin = AddinManager.Registry.GetAddin (id); |
376 | configs = PluginManager.ConfigurablesForAddin (id); |
377 | Title = string.Format ("{0} Configuration", addin.Name); |
378 | + |
379 | notebook.RemovePage (0); |
380 | notebook.ShowTabs = configs.Count () > 1; |
381 | |
382 | @@ -61,7 +63,7 @@ |
383 | |
384 | try { |
385 | config = configurable.GetConfiguration (); |
386 | - notebook.AppendPage (config, new Label (configurable.Name)); |
387 | + notebook.AppendPage (config, new Label (configurable.Title)); |
388 | config.ShowAll (); |
389 | } catch (Exception e) { |
390 | Log.Error ("Failed to load configuration: {0}", e.Message); |
391 | @@ -73,6 +75,11 @@ |
392 | protected virtual void OnBtnCloseClicked (object sender, EventArgs e) |
393 | { |
394 | Hide (); |
395 | + int i=0; |
396 | + while (i < notebook.NPages) { |
397 | + notebook.RemovePage (i); |
398 | + i++; |
399 | + } |
400 | } |
401 | } |
402 | } |
403 | |
404 | === modified file 'Do/src/Do.UI/PreferencesWindow.cs' |
405 | --- Do/src/Do.UI/PreferencesWindow.cs 2009-04-22 04:42:48 +0000 |
406 | +++ Do/src/Do.UI/PreferencesWindow.cs 2009-06-19 02:19:46 +0000 |
407 | @@ -57,7 +57,7 @@ |
408 | |
409 | // Add notebook pages. |
410 | foreach (IConfigurable page in Pages) { |
411 | - notebook.AppendPage (page.GetConfiguration (), new Label (page.Name)); |
412 | + notebook.AppendPage (page.GetConfiguration (), new Label (page.Title)); |
413 | } |
414 | |
415 | // Sets default page to the plugins tab, a good default. |
Makes config widgets a MA extension, namely "/Do/Config". IConfigurable is altered slightly, and Plugin manifests must be changed to reflect this change.
Please see plugin merge request that tracks the changes in core that this would make: /code.edge. launchpad. net/~cszikszoy/ do-plugins/ MA-Config- Extension/ +merge/ 7701
https:/