Do

Code review comment for lp:~cszikszoy/do/config-MA-ext

config-MA-ext
Merge into trunk

Revision history for this message

Chris S. (cszikszoy) wrote on 2009-06-23:

> 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.

> 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.

« Back to merge proposal