-
Notifications
You must be signed in to change notification settings - Fork 1k
Open up MonoDevelop APIs so I can implement a Protobuild add-in #1027
base: main
Are you sure you want to change the base?
Conversation
Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message. |
Heya! I have a few comments before anything else goes:
This is an API break and might have to be delayed for 6.0, which is going to happen after
I'm not a big fan of those changes, but maybe we could work around having to do that? Couldn't the add-in just regenerate the projects through Protobuild on solution opening ( I'm also not a big fan of This PR needs a bit of fixing here and there and a bit of rethinking. |
Opening up the New Project Dialog, Node Builders and such shouldn't happen. You may copy the implementations inside your own add-in instead of changing the API here. It would limit what we can do without breaking compatibility and that's something we want to avoid. |
@@ -49,6 +49,9 @@ public interface IViewContent : IBaseViewContent | |||
void LoadNew (System.IO.Stream content, string mimeType); | |||
void Save (string fileName); | |||
void Save (); | |||
|
|||
void Suspend(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could become an ISuspendableViewContent
, which would be casted using as
for the suspend/resume calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i.e. public interface ISuspendableViewContent : IViewContent { ... }
Also, if possible, this should go in in smaller batches which have atomic changes. The commit is too big and it would be easier to filter smaller changes and fix and merge them quickly locally rather than having to comment on everything. i.e. I'd order commits such as some fix an issue you saw, some expose some API you need (one commit for one functionality), some extend existing code, etc. |
For a bit of background on this, the reason I need Suspend / Resume is that switching tabs destroys the control holding the window handle that the embedded GL context is using. I spent ages trying to find a way that works on Linux where this wasn't necessary, but unfortunately there isn't any solution other than destroying and re-creating a GL context when the window holding it disappears.
This will work for me.
The reason for the large commit is that for a long time I was developing the Protobuild add-in inline in the IDE code, and only recently separated it out into it's own add-in. The reason for this is that I was expected to have to make far more major changes to MonoDevelop than I had to.
I'm not sure this will work because the MonoDevelop code looks for specific implementations of classes. If I can't derive solution / project I'm pretty sure debugging stops work among other things. In particular, if I copied the folder and project file node builders, then they wouldn't get all of the behaviour and contextual menu items that are currently associated with those types. The alternative to opening these up is to have Protobuild support in the upstream code (like MSBuild project support itself is), but I'm pretty sure you won't want to do this given that none of the upstream contributors are using it. |
You should be aware that we are working on a new project model and type system (based on Roslyn), which will be ready in the next months, and will be included in MD 6.0. At this point, we won't accept big changes on those systems. This needs to be reviewed by @mkrueger.
We could add something like that, using the interface proposed above to avoid breaking the api.
Looks good.
I can't accept none of those changes without an explanation. We want to avoid making api public unless really necessary. Also, we try to avoid public virtual members in the api. |
@@ -1126,7 +1126,7 @@ protected override IList<string> GetCommonBuildActions () | |||
return BuildAction.DotNetCommonActions; | |||
} | |||
|
|||
internal override void SetItemHandler (ISolutionItemHandler handler) | |||
public override void SetItemHandler (ISolutionItemHandler handler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so that I can call SetItemHandler(new MSBuildHandler(definition.Guid.ToString(), definition.Guid.ToString()));
and similar, which from memory appears to be required to get the items to behave correctly in the solution explorer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use ProjectExtensionUtil.InstallHandler.
I've added inline comments on the public / virtual changes on why they're needed and where they're used. Some of them I couldn't find references to so when I get a chance I'll try reverting them in the commit and see if everything still builds. |
I don't really need to make changes to those systems; I just need a hook point to say "the project that you see in the solution explorer may not be the one you need to use to evaluate the type system". A level of indirection where I can hand back a different project than the one in the solution explorer is all I need to make this work (since in terms of the build and type systems I still want MonoDevelop to be using the actual C# projects and solutions, while being able to present the user with a solution view that's more useful for working with cross-platform projects). |
I'm going to start breaking out this PR into smaller PRs. I'll base those PRs on the roslyn branch so that this stuff will be available in 6.0 (and to allow for those API breaks like |
Sounds good, thanks! |
This opens up a bunch of MonoDevelop APIs and adds various hooks that are required for me to implement a Protobuild add-in.
Protobuild is a cross-platform project generator; it works by generating MSBuild projects and solutions on disk when the Protobuild.exe executable is run. This allows a common project format like this: https://github.com/mono/MonoGame/blob/develop/Build/Projects/MonoGame.Framework.definition to produce the appropriate C# projects for any platform (Windows, Linux, Mac, iOS, Android, etc.).
I've been writing an add-in that will allow MonoDevelop to load Protobuild modules directly instead of requiring the user to generate projects via the command-line first. This requires additional APIs in MonoDevelop so that I can have an additional project layer on top of the MSBuild project, without losing functionality (there are a lot of places in MonoDevelop that check if the project inherits from an MSBuild project type to provide things like debugging).
Roughly: