Merge lp:~cszikszoy/do/config-MA-ext into lp:do

[Chris S.]

[Chris S. (cszikszoy)]

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:
https://code.edge.launchpad.net/~cszikszoy/do-plugins/MA-Config-Extension/+merge/7701

[Chris Halse Rogers (raof)]

What are the benefits here? I presume this makes it easier for plugins to provide environment-specific configuration pages, but it'd be nice to have it spelled out :)

[Robert Dyer (psybers)]

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)]

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-specific gui toolkit. Do.Platform.Windows provides an extension point: Config. Addins can specify what their dependency is, either Platform.Linux, Platform.Windows etc.

[Alex Launi (alexlauni)]

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)]

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.RemovePage(i) in OnBtnCloseClicked now? Is the 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).

review: Needs Fixing

[Chris S. (cszikszoy)]

> 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 (ConfigurablesForAddin (string id) gets the id from the PluginNodeView. Behind the scenes there, PluginNodeView strips away some of the name of the plugin ID, so, when I'm looking at say the Files plugin, this method gets given this for the id: "Do.File,2.5", but Addin.GetIdName ("Do.File,2.5") returns "Do.File"

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.RemovePage(i) in OnBtnCloseClicked now? Is the
> 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.

[lp:~cszikszoy/do/config-MA-ext]

1238. By Chris S. on 2009-06-23

LINQ query & indentation fixes as per review

1239. By Chris S. on 2009-06-23

change extension point to /Do/Configuration

1240. By Chris S. on 2009-06-23

add FIXME comment

1241. By Chris S. on 2009-06-23

lift statement out of loops

[Alex Launi (alexlauni)]

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.

review: Needs Fixing

[lp:~cszikszoy/do/config-MA-ext]

1242. By Chris S. on 2009-07-07

merge w/ trunk

[Alex Launi (alexlauni)]

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

review: Approve

[Robert Dyer (psybers)]

I still won't approve until we have a merge request fixing do-plugins.

[Alex Launi (alexlauni)]

I will approve, I just won't merge.

--
-- Alex Launi

[Chris Halse Rogers (raof)]

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)]

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://code.edge.launchpad.net/~cszikszoy/do/config-MA-ext/+merge/7700
> You are reviewing the proposed merge of lp:~cszikszoy/do/config-MA-ext into
> lp:do.
>

--
--Alex Launi

Unmerged revisions

1242. By Chris S. on 2009-07-07

merge w/ trunk

1241. By Chris S. on 2009-06-23

lift statement out of loops

1240. By Chris S. on 2009-06-23

add FIXME comment

1239. By Chris S. on 2009-06-23

change extension point to /Do/Configuration

1238. By Chris S. on 2009-06-23

LINQ query & indentation fixes as per review

1237. By Chris S. on 2009-06-20

remove MA requirement for Do.Universe

1236. By Chris S. on 2009-06-20

merge trunk

1235. By Chris S. on 2009-06-20

more fixes

1234. By Chris S. on 2009-06-19

remove property

1233. By Chris S. on 2009-06-19

fix crash when opening preference window twice

=== modified file 'Do.Platform.Linux/Resources/Do.Platform.Linux.addin.xml'
--- Do.Platform.Linux/Resources/Do.Platform.Linux.addin.xml	2009-06-10 05:30:30 +0000
+++ Do.Platform.Linux/Resources/Do.Platform.Linux.addin.xml	2009-06-20 02:35:45 +0000
@@ -32,4 +32,8 @@
 		<Service type="Do.Platform.Linux.GnomeKeyringSecurePreferencesService" />
 	</Extension>
 	
+	<ExtensionPoint path="/Do/Config">
+		<ExtensionNode name="Config" objectType="Do.Platform.Linux.IConfigurable"/>
+	</ExtensionPoint>
+	
 </Addin>

=== modified file 'Do.Platform.Linux/gtk-gui/gui.stetic'
--- Do.Platform.Linux/gtk-gui/gui.stetic	2009-05-29 10:26:49 +0000
+++ Do.Platform.Linux/gtk-gui/gui.stetic	2009-06-19 20:22:55 +0000
@@ -7,7 +7,6 @@
   <import>
     <widget-library name="gnomedesktop-sharp, Version=2.20.0.0, Culture=neutral, PublicKeyToken=35e10195dab3c99f" />
     <widget-library name="notify-sharp, Version=0.4.0.0, Culture=neutral, PublicKeyToken=2df29c54e245917a" />
-    <widget-library name="../../Do.Interface.Linux/bin/Debug/Do.Interface.Linux.dll" />
     <widget-library name="gnome-sharp, Version=2.24.0.0, Culture=neutral, PublicKeyToken=35e10195dab3c99f" />
     <widget-library name="../bin/Debug/Do.Platform.Linux.dll" internal="true" />
   </import>

=== modified file 'Do.Platform.Linux/src/Do.Platform/Do.Platform.Linux/IConfigurable.cs'
--- Do.Platform.Linux/src/Do.Platform/Do.Platform.Linux/IConfigurable.cs	2008-12-21 23:10:16 +0000
+++ Do.Platform.Linux/src/Do.Platform/Do.Platform.Linux/IConfigurable.cs	2009-06-19 02:19:46 +0000
@@ -25,10 +25,7 @@
 {
 	public interface IConfigurable
 	{
-		string Name { get; }
-		string Description { get; }
-		string Icon { get; }
-		
-        Bin GetConfiguration ();
+		string Title { get; }
+		Bin GetConfiguration ();
     }
 }

=== modified file 'Do/gtk-gui/objects.xml'
--- Do/gtk-gui/objects.xml	2009-06-06 18:05:52 +0000
+++ Do/gtk-gui/objects.xml	2009-06-19 02:19:46 +0000
@@ -1,12 +1,13 @@
 <objects attr-sync="on">
+  <object type="Do.UI.GeneralPreferencesWidget" palette-category="Do" allow-children="false" base-type="Gtk.Bin">
+    <itemgroups>
+    </itemgroups>
+    <signals />
+  </object>
   <object type="Do.UI.KeybindingsPreferencesWidget" palette-category="Do" allow-children="false" base-type="Gtk.Bin">
     <itemgroups />
     <signals />
   </object>
-  <object type="Do.UI.GeneralPreferencesWidget" palette-category="Do" allow-children="false" base-type="Gtk.Bin">
-    <itemgroups />
-    <signals />
-  </object>
   <object type="Do.UI.ManagePluginsPreferencesWidget" palette-category="Do" allow-children="false" base-type="Gtk.Bin">
     <itemgroups />
     <signals />

=== modified file 'Do/src/Do.Core/PluginManager.cs'
--- Do/src/Do.Core/PluginManager.cs	2009-06-05 19:45:45 +0000
+++ Do/src/Do.Core/PluginManager.cs	2009-06-20 02:35:37 +0000
@@ -46,7 +46,7 @@
 	{
 		const string DefaultPluginIcon = "folder_tar";
 		
-		static IEnumerable<string> ExtensionPaths = new [] { "/Do/ItemSource", "/Do/Action" };
+		static IEnumerable<string> ExtensionPaths = new [] { "/Do/ItemSource", "/Do/Action", "/Do/Config" };
 
 		public static readonly IEnumerable<AddinClassifier> Classifiers =
 			new AddinClassifier [] {
@@ -80,7 +80,7 @@
 			// Initialize services before addins that may use them are loaded.
 			Services.Initialize ();
 			InterfaceManager.Initialize ();
-			
+						
 			// Now allow loading of non-services.
 			foreach (string path in ExtensionPaths)
 				AddinManager.AddExtensionNodeHandler (path, OnPluginChanged);
@@ -297,12 +297,19 @@
 		/// </returns>
 		public static IEnumerable<IConfigurable> ConfigurablesForAddin (string id)
 		{
-			return ObjectsForAddin<IConfigurable> (id);
+			IEnumerable<TypeExtensionNode> configs = AddinManager.GetExtensionNodes ("/Do/Config")
+				.OfType <TypeExtensionNode> ().Where (n => Addin.GetIdName (n.Addin.Id) == Addin.GetIdName (id));
+		
+			foreach (TypeExtensionNode page in configs) {
+					yield return page.GetInstance () as IConfigurable;
+			}
+			
+			yield break;
 		}
-		
+				
 		static string AddinIdWithoutVersion (string id)
 		{
 			return id.Substring (0, id.IndexOf (','));
 		}
 	}
-}
+}
\ No newline at end of file

=== modified file 'Do/src/Do.UI/ColorConfigurationWidget.cs'
--- Do/src/Do.UI/ColorConfigurationWidget.cs	2009-04-21 02:15:39 +0000
+++ Do/src/Do.UI/ColorConfigurationWidget.cs	2009-06-19 02:19:46 +0000
@@ -44,6 +44,7 @@
 		public ColorConfigurationWidget ()
 		{
 			Build ();
+			
 			AppPaintable = true;
 			Themes = new List<string> ();
 			Interface.Util.Appearance.SetColormap (this);
@@ -57,7 +58,6 @@
 				composite_warning_widget.Visible = true;
 				theme_combo.Sensitive = false;
 			}
-				
 			// Setup theme combo
 			theme_combo.Active = Math.Max (0, Themes.IndexOf (Do.Preferences.Theme));
 			pin_check.Active = Do.Preferences.AlwaysShowResults;
@@ -65,6 +65,10 @@
 			theme_configuration_container.ShowAll ();
 		}
 		
+		public string Title {
+			get { return Catalog.GetString ("Appearance"); }
+		}
+				
 		public Gtk.Bin GetConfiguration ()
 		{
 			return this;
@@ -86,11 +90,13 @@
 			if (theme_configuration_container.Child != null)
 					theme_configuration_container.Remove (theme_configuration_container.Child);
 			
+
 			if (Do.Controller.Window is IConfigurable) {
 				IConfigurable window = Do.Controller.Window as IConfigurable;
 				Gtk.Bin bin = window.GetConfiguration ();
 				theme_configuration_container.Add (bin);
 			}
+
 			theme_configuration_container.ShowAll ();
 		}
 
@@ -98,18 +104,6 @@
 		{
 		}
 		
-		public new string Name {
-			get { return Catalog.GetString ("Appearance"); }
-		}
-
-		public string Description {
-			get { return ""; }
-		}
-		
-		public string Icon {
-			get { return ""; }
-		}
-
 		public override void Dispose ()
 		{
 			if (theme_configuration_container.Child != null)

=== modified file 'Do/src/Do.UI/GeneralPreferencesWidget.cs'
--- Do/src/Do.UI/GeneralPreferencesWidget.cs	2009-02-18 00:31:48 +0000
+++ Do/src/Do.UI/GeneralPreferencesWidget.cs	2009-06-19 02:19:46 +0000
@@ -33,39 +33,32 @@
 
 namespace Do.UI
 {
-    [System.ComponentModel.Category("Do")]
-    [System.ComponentModel.ToolboxItem(true)]
-    public partial class GeneralPreferencesWidget : Bin, IConfigurable
-    {
-		new public string Name {
-		  get { return Catalog.GetString ("General"); }
-		}
-			
-		  public string Description {
-		  get { return ""; }
-		}
-	        
-		public string Icon {
-		  get { return ""; }
-		}
+	[System.ComponentModel.Category("Do")]
+	[System.ComponentModel.ToolboxItem(true)]
+	public partial class GeneralPreferencesWidget : Bin, IConfigurable
+	{
 		
 		public GeneralPreferencesWidget ()
 		{
-		  Build ();
-		  
-		  // Setup checkboxes
-		  hide_check.Active = Do.Preferences.QuietStart;
-		  login_check.Active = AutostartEnabled;
-		  notification_check.Active = TrayIconService.Visible;
-		}
-	        
+			Build ();
+
+			// Setup checkboxes
+			hide_check.Active = Do.Preferences.QuietStart;
+			login_check.Active = AutostartEnabled;
+			notification_check.Active = TrayIconService.Visible;
+		}
+		
+		public string Title {
+			get { return Catalog.GetString ("General"); }
+		}
+		
 		public Bin GetConfiguration ()
 		{
-		  return this;
+			return this;
 		}
-	        
+
 		protected bool AutostartEnabled {
-		  get {
+			get {
 				return Services.System.IsAutoStartEnabled ();
 			}
 			
@@ -80,17 +73,17 @@
 		
 		protected virtual void OnLoginCheckClicked (object sender, EventArgs e)
 		{
-		  AutostartEnabled = login_check.Active;
+			AutostartEnabled = login_check.Active;
 		}
 		
 		protected virtual void OnHideCheckClicked (object sender, EventArgs e)
 		{
-		  Do.Preferences.QuietStart = hide_check.Active;
+			Do.Preferences.QuietStart = hide_check.Active;
 		}
 		
 		protected virtual void OnNotificationCheckClicked (object sender, System.EventArgs e)
 		{
-		  TrayIconService.Visible = notification_check.Active;
+			TrayIconService.Visible = notification_check.Active;
 		}
-    }
-}
+	}
+}
\ No newline at end of file

=== modified file 'Do/src/Do.UI/KeybindingsPreferencesWidget.cs'
--- Do/src/Do.UI/KeybindingsPreferencesWidget.cs	2009-01-27 05:08:07 +0000
+++ Do/src/Do.UI/KeybindingsPreferencesWidget.cs	2009-06-19 02:19:46 +0000
@@ -36,18 +36,6 @@
 	{
 		private KeybindingTreeView kbview;
 		
-		new public string Name {
-			get { return Catalog.GetString ("Keyboard"); }
-		}
-		
-        public string Description {
-        	get { return ""; }
-        }
-        
-        public string Icon {
-        	get { return ""; }
-        }
-		
 		public KeybindingsPreferencesWidget ()
 		{
 			Build ();
@@ -58,9 +46,13 @@
             action_scroll.ShowAll ();
 		}
 		
+		public string Title {
+			get { return Catalog.GetString ("Keyboard"); }
+		}
+				
 		public Bin GetConfiguration ()
-        {
-        	return this;
-        }
+		{
+			return this;
+		}
 	}
-}
+}
\ No newline at end of file

=== modified file 'Do/src/Do.UI/ManagePluginsPreferencesWidget.cs'
--- Do/src/Do.UI/ManagePluginsPreferencesWidget.cs	2009-06-05 19:45:45 +0000
+++ Do/src/Do.UI/ManagePluginsPreferencesWidget.cs	2009-06-19 02:19:46 +0000
@@ -50,18 +50,6 @@
 		PluginNodeView nview;
 		SearchEntry search_entry;
 
-		new public string Name {
-			get { return Catalog.GetString ("Plugins"); }
-		}
-
-		public string Description {
-			get { return ""; }
-		}
-
-		public string Icon {
-			get { return ""; }
-		}
-
 		public ManagePluginsPreferencesWidget ()
 		{
 			Build ();
@@ -98,6 +86,15 @@
 			Services.Application.RunOnMainThread (() => search_entry.InnerEntry.GrabFocus ());
 		}
 		
+		public string Title {
+			get { return Catalog.GetString ("Plugins"); }
+		}
+				
+		public Bin GetConfiguration ()
+		{
+			return this;
+		}
+		
 		protected void OnDragDataReceived (object sender, DragDataReceivedArgs args)
 		{
 			string data;
@@ -144,11 +141,6 @@
 			} 
 		}
 
-		public Bin GetConfiguration ()
-		{
-			return this;
-		}
-
 		private void OnPluginSelected (object sender, PluginSelectionEventArgs e)
 		{
 			UpdateButtonState ();

=== modified file 'Do/src/Do.UI/PluginConfigurationWindow.cs'
--- Do/src/Do.UI/PluginConfigurationWindow.cs	2008-12-21 23:34:14 +0000
+++ Do/src/Do.UI/PluginConfigurationWindow.cs	2009-06-19 20:29:23 +0000
@@ -37,18 +37,20 @@
 
 		const string TitleMarkup =
 			"<span weight=\"heavy\" size=\"large\">{0}</span>";
-
+		
 		public PluginConfigurationWindow (string id) : 
 			base (WindowType.Toplevel)
 		{
 			Addin addin;
+
+			Build ();
+			
 			IEnumerable<IConfigurable> configs;
 
-			Build ();
-
 			addin = AddinManager.Registry.GetAddin (id);
 			configs = PluginManager.ConfigurablesForAddin (id);
 			Title = string.Format ("{0} Configuration", addin.Name);
+
 			notebook.RemovePage (0);
 			notebook.ShowTabs = configs.Count () > 1;
 
@@ -61,7 +63,7 @@
 
 				try {
 					config = configurable.GetConfiguration ();
-					notebook.AppendPage (config, new Label (configurable.Name));
+					notebook.AppendPage (config, new Label (configurable.Title));
 					config.ShowAll ();
 				} catch (Exception e) {
 					Log.Error ("Failed to load configuration: {0}", e.Message);
@@ -73,6 +75,11 @@
 		protected virtual void OnBtnCloseClicked (object sender, EventArgs e)
 		{
 			Hide ();
+			int i=0;
+			while (i < notebook.NPages) {
+				notebook.RemovePage (i);
+				i++;
+			}
 		}
 	}
 }

=== modified file 'Do/src/Do.UI/PreferencesWindow.cs'
--- Do/src/Do.UI/PreferencesWindow.cs	2009-04-22 04:42:48 +0000
+++ Do/src/Do.UI/PreferencesWindow.cs	2009-06-19 02:19:46 +0000
@@ -57,7 +57,7 @@
 			
 			// Add notebook pages.
 			foreach (IConfigurable page in Pages) {
-				notebook.AppendPage (page.GetConfiguration (), new Label (page.Name));
+				notebook.AppendPage (page.GetConfiguration (), new Label (page.Title));
 			}
 			
 			// Sets default page to the plugins tab, a good default.