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

Don't generate interface properties for customized interface properties #4

Open
hfloyd opened this issue Mar 10, 2022 · 5 comments
Open

Comments

@hfloyd
Copy link
Contributor

hfloyd commented Mar 10, 2022

Hi!

So, related to #3... Sometimes I customize the property for an interface. For instance, Content Pickers or MNTP by default will be returning "IPublishedContent", but I might know that it will always be of a specific ContentType . In order to be able to call the property and get the strongly-typed model in a View, I customize the composition model and interface. Example:

Customized 'CompRelatedBlogPosts.cs'

partial interface ICompRelatedBlogPosts
    {
        IEnumerable<BlogPost> SelectedPosts { get; }
    }

    partial class CompRelatedBlogPosts
    {
        [ImplementPropertyType("SelectedPosts")]
        public IEnumerable<BlogPost> SelectedPosts => GetSelectedPosts(this);


        public static IEnumerable<BlogPost> GetSelectedPosts(ICompRelatedBlogPosts that)
        {
            return that.Value<IEnumerable<IPublishedContent>>("SelectedPosts").ToContentModels<BlogPost>(true);
        }
    }
}

Then I just add an override to set the correct type in the Models which use the Composition. Example:

partial class TextPage
    {
        #region Implementation of ICompRelatedBlogPosts
        IEnumerable<BlogPost> ICompRelatedBlogPosts.SelectedPosts => CompRelatedBlogPosts.GetSelectedPosts(this);
        #endregion
    }

This works fine. However, when I re-generate the Models, the generated composition file gets the "IPublishedContent" property again:

'CompRelatedBlogPosts.generated.cs'

 public partial interface ICompRelatedBlogPosts : IPublishedContent {

        System.Collections.Generic.IEnumerable<global::Umbraco.Cms.Core.Models.PublishedContent.IPublishedContent> SelectedPosts { get; }

    }

I can solve the issue by explicitly ignoring the property:

'GetModelsNotificationHandler.cs'

    public class GetModelsNotificationHandler : INotificationHandler<GetModelsNotification>
    {
        public void Handle(GetModelsNotification notification)
        {
            foreach (TypeModel model in notification.Models)
            {
            ...             
				//Content-Type specific processing

				if (model.Alias == CompRelatedBlogPosts.ModelTypeAlias)
				{
					foreach (PropertyModel property in model.Properties)
					{
						if (property.Alias == "SelectedPosts")
						{
							property.IsIgnored = true;
						}

					}
				}
               ... 
            }
        }
    }

But I wonder if the generating code can check for the customized property defined in a custom interface implementation and skip it, like it does for the regular properties?

@abjerner
Copy link
Member

Hi @hfloyd

I don't know how OMB handles this, but looking at the custom partial to determine property type for the interface sounds like a good idea 👍

@abjerner
Copy link
Member

If the picker is a MNTP, it might also be worth looking at our custom MNTP package.

It let's you select your custom "item converter" in the data type, which could then return the selected blog posts as BlogPost instead of IPublishedContent. Then the property will return the correct type regardless of whether you're using ModelsBuilder or calling the .Value() method directly.

If I implement #5, it would also mean that property values are better cached compared to when handling the conversion directly in the model.


I still like your proposal, so the above is just an example on another approach with the same end result 😉

@hfloyd
Copy link
Contributor Author

hfloyd commented Mar 18, 2022

@abjerner Cool package! Right now I'm working with a migrated site with a lot of standard Content Pickers in use, so it wouldn't really help all my use-cases.

Also a question - it seems your package was specifically designed such that you would manually design the data models and set up the Converter. Does it handle a basic usage of just using the standard Model for a specific existing DocType?

Regarding OMB's handling... What WAS handled by OMB was that if I "redefined" a property in an interface, it would skip re-generating it in the Composition interface. What it never did support was automatically adding the derived-models customization.

In my example, this bit I always had to add myself to all the affected Models:

partial class TextPage
    {
        #region Implementation of ICompRelatedBlogPosts
        IEnumerable<BlogPost> ICompRelatedBlogPosts.SelectedPosts => CompRelatedBlogPosts.GetSelectedPosts(this);
        #endregion
    }

Sometimes that could get annoying - if I had a Comp which I used on a lot of Doctypes, I would have to create "Custom" partials for all those Doctypes - even if this was the only customized bit. But, it's a pretty simple copy/paste for the code snippet, so not a huge deal.

@abjerner
Copy link
Member

abjerner commented Jun 2, 2022

Hey @hfloyd

Just to let you know that I haven't forgotten you, but I haven't had the time yet to look through your issues. I hope I will have that soon 😉

@hfloyd
Copy link
Contributor Author

hfloyd commented Jun 2, 2022

No problem, @abjerner I know you have other things to work on also. I like to add issues when I come across them, but certainly have no expectations for instant attention. 😊

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

No branches or pull requests

2 participants