Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port to castle windsor #3471

Merged
merged 17 commits into from Oct 17, 2017
Merged

Conversation

MDoerner
Copy link
Contributor

This PR ports the registration logic to Castle Windsor and makes some adjustments to the auto-magic factories because they work a bit different with CW.

I have strived to stay as close to the registration logic we had with Ninject but had to make some adjustments because there is no WhenInjectedInto. Moreover, I removed resolving the menu items during the registration in favour of letting the resolver do it when resolving the App. In addition, structured the registration logic a bit more.
Another change is that the GeneralSetting we load at the startup to determine whether to display the splash window now get passed to the container. This is used to determine whether to load SC or not, at least regarding the menu item. (Before, the settings got resolved at the spot they were needed in the registration.)

Regarding startup speed, I could not see any improvement; it might actually be even slower.

Given the more structured registration module RubberduckIoCInstaller and the fact that the registration logic now works the way around it does for most IoC containers, porting again to another IoC container should be easier next time we feel the urge to do so.

Please have a look at the new registration and review closely. Due to the nature of the container, I could not test after every step I made. Still, all tests pass and it is F5 tested. (There has not been another way to test the registration anyway.) Moreover, now there are also some new tests testing that the registration does not blow up and that the same is true for resolving the RubberduckParserstate and the Inspections.

Happy reviewing!

Closes #3287

…oule and Windsor versions of the Interceptors.
… the inspections get resolved without throwing.
# Conflicts:
#	RetailCoder.VBE/Root/EnumerableCounterInterceptor.cs
#	RetailCoder.VBE/Root/InterceptorBase.cs
#	RetailCoder.VBE/Root/RubberduckModule.cs
#	RetailCoder.VBE/Root/TimedCallLoggerInterceptor.cs
#	RetailCoder.VBE/Rubberduck.csproj
#	RetailCoder.VBE/UI/SourceControl/SourceControlProviderFactory.cs
#	Rubberduck.Parsing/packages.config
#	RubberduckTests/Mocks/MockVbeBuilder.cs
#	RubberduckTests/packages.config

Resolved conflicts: changes to RubberduckModule have been ported to RubberduckIoCInstaller
Merging last minute changes.
if (view.DialogResult == DialogResult.Cancel || !IsValidVariableName(view.NewName, forbiddenNames))
{
return string.Empty;
}

return view.NewName;
}
finally
{
_dialogFactory.Release(view);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do all factories need an explicit Release call? Not that I'm religious about it, but per Seemann's "RRR" pattern there should only need to be one single call to Release the container in the app. Calling Release on the factory instance seems counter-intuitive to me. It's releasing the view?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing why you call factory.Release with the auto-magic factories in CW is that CW tracks everything. If the view is registered in singleton scope, the release call does not do anything, at least I think so. However, if it is bound in transient scope, CW will dispose it, if disposable, and stop tracking it. Otherwise, it will hold on to a reference to the object until the container gets disposed.

@bclothier
Copy link
Contributor

The additions of various app.config worries me -- they don't work quite the same way for a project that's basically a DLL. Should we really be using the default implementation of app.config to load the settings? Because Rubberduck gets loaded as a DLL, app.config might not work as expected. We would need to do something else to ensure that the .config files can be processed.

@MDoerner
Copy link
Contributor Author

The app.config files have been added automatically when installing the CW package. I guess CW needs them and should find them since the package has added them.

Copy link
Member

@retailcoder retailcoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we proceed? Looks good to me!


//Guidelines and words of caution:

//1) Please always specify the Lisfestyle. The default is singleton, which can be confusing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo 😉


var assembliesToRegister = AssembliesToRegister().ToArray();

RegisterConfiguartion(container, assembliesToRegister);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo

return new Type[]
{
typeof(CodeExplorerCommandMenuItem),
//typeof(RegexSearchReplaceCommandMenuItem),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this also be moved into an #if DEBUG block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly should be moved there: the RegexSearchReplaceCommandMenuItem but uncommented?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be more in line with the SerializeDeclarationsMenuItem and RefactorExtractMethodCommand bindings. @retailcoder @Hosch250 is the feature ready enough to not blow everything to pieces when we bind it for debug builds?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Vogel612 no idea, @bclothier what do you say?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this PR, I'd leave ExtractMethod out. I'll make a note to update once I can merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that removing a command menu item from the menus does not cause the corresponding command not to be loaded; it only removes the menu item.

{
//note: CommandBase naming convention: [Foo]Command
//note: ICommandMenuItem naming convention for [Foo]Command: [Foo]CommandMenuItem
return itemName.Substring(0, itemName.Length - "MenuItem".Length);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love the "MenuItem".Length

@retailcoder retailcoder merged commit 0e42e40 into rubberduck-vba:next Oct 17, 2017
@MDoerner MDoerner deleted the PortToCastleWindsor branch October 22, 2017 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kill Ninject
5 participants