[Mono-osx] [MonoMac] patch for MM-addin, Existing Framework

[Michael Hutchinson]

On Thu, Jan 27, 2011 at 4:50 PM, Duane Wandless <duane at wandless.net> wrote:
> If you haven't seen the UI running the "relative path" option probably is
> not clear.  The concept is the same as adding additional csproj files.  I

I thought csproj references as added by MD and VS were always relative...

> can move my solution around and as long as the csproj is in the same
> relative location it will be found.  I have that same functionality for
> External Frameworks.  You can add them with absolute or relative paths.

Ah, ok. Wouldn't hint paths (like assembly references) be more powerful?

>  Meaning at compile time the addin will copy them based on that
> absolute/relative flag.  But at runtime the framework loader will call
> dlopen with the path to the Contents/Frameworks location of the bundle.
> The code is in MonoMacBuildExtension.Build.  There is no OnBuild in that
> class.  MonoMacProject can override OnBuild but currently there is no

Ah, my bad, I meant MonoMacBuildExtension.Build.

> override.  It seems to be working correctly now.
> I see only a few remaining tasks:
> * create a context menu on the External Frameworks node, currently there is
> one only on the project
> * fix the path in the dlopen call
> * decide if a throw should occur on dlopen failure
> * catch errors and display nice output during the copy phase

I took a look at the added files now and I have a few more comments,
mostly style and nitpick. Sorry for the detail overload!

* Should be a blank line between members, except one-liners
(ExternalFrameworksNodeBuilder.cs line 38, ExternalFrameworkNode.cs).
* Place the opening brace of a namespace on a new line
(AddFrameworks.cs, MonoMacAddFrameworksHandler.cs)
* Don't use an "as" cast when you don't expect the cast to fail
(ExternalFrameworksNodeBuilder.cs line 52,
ExternalFrameworkNodeBuilder.c lines 102, 103).
* Use GettextCatalog to make user-visible string localizable
(ExternalFrameworkNodeBuilder.cs lines 42, 47, 106,
MonoMacAddFrameworksHandler.cs line 67)
* Instead of "Count () > 0" you can use "Any ()"
(ExternalFrameworkNodeBuilder.cs, line 54)
* Space before opening "(" in method call
(ExternalFrameworkNodeBuilder.cs lines 71, 76,
MonoMacAddFrameworksHandler.cs line 65).
* HandleMonoMacAddFrameworksHandlerNotifyFrameworksChanged method name
is rather long without adding much meaning, maybe OnFrameworkChanged
is better (ExternalFrameworksNodeBuilder.cs)
* ExternalFrameworksNodeBuilder and ExternalFrameworkNodeBuilder names
are easy to confuse.
* ExternalFrameworkNode should probably be called
MonoMacFrameworkItem. It's fundamentally a project item, "node" naming
is from the solution tree. And it should probably serialize to the
MSBuild file as a "MonoMacFramework" item.
* Path properties in ExternalFrameworkNode should use
ProjectPathItemProperty attribute instead of ItemProperty, so they're
serialized to/from MSBuild path format correctly. That means they'll
be stored as relative paths in the csproj and absolute paths in the
items fields, not sure what that means for your absolute framework
paths. Paths should also be exposed as FilePath, not string.
* Please avoid non-private fields (ExternalFrameworkNode). Project
could have a public getter and and an internal setter. Name could just
be "public string Name { get { return FullPath.FileName; } }.
* "Relatve" should be "Relative" (ExternalFrameworkNode).
* Prefer early return instead of deep nesting
(MonoMacAddFrameworksHandler.cs line 47, 53)
* FrameworksChangedArgs could use an auto property with private setter.
* MonoMacCommands should be in a MonoMacCommands.cs file for
consistency with MD. Command handlers may be below it in this file.
* Opening brace { in if block should go on same line as if
(MonoMacAddFrameworksHandler.cs, line 82)
* Dialog subclass should have suffix "Dialog"
* Dialog should use modal runloop instead of button handlers and hide.
GTK# idiom (with early return, and accessing needed values before
destroying the widgets) is:

var dlg = new FooDialog (bar) {
    Modal = true,
    TransientFor = parentWindow,
};
string valueFromDialog;
try {
    if (dialog.Run () == ResponseType.Ok)
        return;
    valueFromDialog = dialog.PropertyThatAccessesWidgetValue;
} finally {
    dlg.Destroy ();
}
//do stuff with valueFromDialog

It helpt to think of the dialog as a view :)

In MD code you should also use MessageService.RunCustomDialog instead
of calling RunCustomDialog directly. This will automatically parent
the dialog onto the main window or topmost modal dialog (setting
TransientFor etc), and implements a Mac-specific GTK bug workaround
that repositions the window in a sensible location. Since it will
Show() the window after repositioning it, you should uncheck "Visible"
for the dialog in designer's property grid, else the generated Build()
method will call Show() and the window will show as soon as it it
constructed, hence you get a "jump" when it's repositioned.

I also had an idea regarding a simple change to the generated code
that I think would improve user experience. If we make the generated
code use some standard type and method name, generate the code in
obj/$(config) and inject the file directly into the build (like
Moonlight projects do), then we can have NSApplication.Init check the
calling assembly for that method and invoke it. That way we avoid
adding a scary generated file (that's really an implementation detail)
into the root of every MonoMac project. Another nice bonus is that it
would allow framework items to have MSBuild conditions applied to make
them per-configuration, which isn't really doable if the generated
code is in the project. Does this make sense?

BTW, could you send the next version as a pull request on github so I
can make any remaining comments inline?

Thanks!

-- 
Michael Hutchinson
http://mjhutchinson.com