Skip to content
This repository was archived by the owner on Jun 21, 2023. It is now read-only.

Conversation

@shana
Copy link
Contributor

@shana shana commented Jan 11, 2016

We're rapidly accumulating menu handlers for the gist and copy link support, and our current code is less than ideal. This PR turns our current menu handling code into MEF components, so we can easily add as many handlers as we need.

There should be no behaviour change, everything should work exactly the same way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little wary of making these settable properties. Could we change the interface to make these read only properties and use ctor injection here like so?

[ImportingConstructor]
public MenuProvider([ImportMany]IEnumerable<IMenuHandler> menus, [ImportMany]IEnumerable<IDynamicMenuHandler> dynamicMenus)
{
    // ...
}

I believe that better represents our intentions here.

Also, I'm guessing we should expose these properties as IReadOnlyCollection<T> rather than IEnumerable<T> since these aren't lazy enumerables and we want to avoid any possibility of multiple enumerations.

@haacked
Copy link
Contributor

haacked commented Jan 13, 2016

This makes me happy! 😄

@shana
Copy link
Contributor Author

shana commented Jan 13, 2016

It makes total sense, and I remember meaning to make those readonly once I was done with everything else and haha famous last words 😋

@shana shana mentioned this pull request Jan 13, 2016
3 tasks
haacked added a commit that referenced this pull request Jan 13, 2016
@haacked haacked merged commit 8d201c3 into master Jan 13, 2016
@haacked haacked deleted the shana/new-menu-system branch January 13, 2016 18:25
@haacked
Copy link
Contributor

haacked commented Jan 13, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants