Add IShapePlacementProvider and ShapeTablePlacementProvider

[TFleury]

The goal is to add a way to provide shape placement overrides at runtime without rebuilding ShapeTable (as discussed in #6503)

The new IShapePlacementProvider interface allows to asynchronously (usefull if it relies on an async placement store) build an IPlacementInfoResolver for each IBuildShapeContext.

The IPlacementInfoResolver implementation keeps placement rules for the current IBuildShapeContext to be able to quickly resolve shape placements synchronously.

This PR don't add any direct way to provide placement in admin UI, but opens a way to add this feature.

Breaking changes

• Removed IBuildShapeContext argument from FindPlacementDelegate because it's not used anywhere.
• Moved ShapeTable provided placement logic from BaseDisplayManager to newly created ShapeTablePlacementProvider (the first IShapePlacementProvider implementation).
  It allowed me to remove BaseDisplayManager direct dependencies on IShapeTableManager and IThemeManager

[TFleury]

/cc @deanmarcussen @jtkech @ns8482e

[jtkech]

Will do a review more in details asap, but looks great!

Hmm, just one question about ordering, i assume that a dynamic placement would need an higher precedence than the ones defined in modules and the current theme. E.g. the templates feature has an higher priority because, as i remember, the shape binding resolvers are explicitly called before using shape descriptors in a regular way.

One solution would be to register ShapeTablePlacementProvider later, this is possible by using a separate OrchardCoreBuilder.ConfigureServices() that has an optional order parameter (0 by default) that we could set to a relative high value.

The other solution being to let the placement resolved as before but after having called the dynamic resolvers before. But on another side i like these new interfaces as IShapePlacementProvider including by default ShapeTablePlacementProvider.

[TFleury]

One solution would be to register ShapeTablePlacementProvider later, this is possible by using a separate OrchardCoreBuilder.ConfigureServices() that has an optional order parameter (0 by default) that we could set to a relative high value.

Placement resolvers are called in reverse order, so the last registered has a higher priority.
We can register ShapeTablePlacementProvider with an order below default just to ensure other implemantations with the default order are prioritized.

[ns8482e]

@TFleury I have not looked into more detail but I guess - all we need is to replace context.FindPlacement from the driver without introducing breaking changes and driver needs to register before ContentItemDisplayCoordinator which can be easily done using .ConfigureServices() from module with an Order

For example,

services.AddOrchardCms()
.ConfigureServices(services =>
               {
                   services.AddScoped<IContentDisplayHandler, CustomPlacementDisplayHandler>();
               }, -100);

and in CustomPlacementDisplayHandler

public CustomPlacementDisplayHandler(MyPlacementService service)
{
}
public async Task BuildDisplayAsync(ContentItem contentItem, BuildDisplayContext context)
{
       context.FindPlacement = myPlacementService.FindPlacement
}

and provide all your dynamic placement logic in MyPlacementService and all code stays in the module and use FindPlacementImpl from BaseDisplayManager as fallback without changing anything in OrchardCore.DisplayManagement

[jtkech]

@TFleury

Just forked your branch, i will try it and review it tomorrow, looks good after a quick look!

Maybe just some formatting to do, e.g. a space after the for and foreach keywords as i saw in BaseDisplayManager after doing a ctrl + KD ;)

[ns8482e]

@TFleury here's the quick and dirty implementation of an idea without modifying in DisplayManagement ( i.e. in a Module) https://gist.github.com/ns8482e/a0cb2399a2849af0ef9e335b28567a27

However, in ideal solution - I would foresee an event like named BuildPlacement or ContextCreated ( more generalized name) fired after context is created, that would allow a chance to modify context object before display events.

[TFleury]

@ns8482e Thank you for this contribution. I had not seen this possibility when creating the PR.

I like the idea of combining PlacementInfos.

On the other hand, I found relevant to to move ShapeTable placement resolution outside of BaseDisplayManager and to have it in a distinct provider. And it results in a lighter constructor (one parameter less)

I removed IBuildShapeContext argument from FindPlacementDelegate because it was not used, but maybe we can keep it. (It's usefull in the case of your implementation)

We just need to decide which is the best option.

I prefer to have one provider interface used by multiple services, rather than having multiple handlers replacing the FindPlacement delegate each in turn. I would be easier to debug.

[ns8482e]

I removed IBuildShapeContext argument from FindPlacementDelegate because it was not used, but maybe we can keep it. (It's usefull in the case of your implementation)

I would not change FindPlacementDelegate as it's a public property of context and we would never know how others are using as opposed to FindPlacementImpl which is private.

On the other hand, I found relevant to to move ShapeTable placement resolution outside of BaseDisplayManager

Yes that's could be an option to register provider for ShapeTable

However IMO keeping ShapeTable placement inside BaseDisplayManager allows to have default implementation that comes with the deal and developer who wants to create new CustomDisplayManager need not worry about configuring placements or ask how to in gitter to @deanmarcussen :-)

I prefer to have one provider interface used by multiple services,

I am in for it - In my example above - i used IContentDisplayHandler with -100 just to have handler execute before all other BuildDisplayAsync events.

            var context = new BuildDisplayContext(
                itemShape,
                actualDisplayType,
                groupId,
                _shapeFactory,
                await _layoutAccessor.GetLayoutAsync(),
                new ModelStateWrapperUpdater(updater)
            );


           await BindPlacementAsync(context);

// >>>> Find Placement Here with event or serviceprovider
            

            await _handlers.InvokeAsync((handler, contentItem, context) => handler.BuildDisplayAsync(contentItem, context), contentItem, context, _logger);

But IMHO adding an new handler event is much more relevant as we already have interface defined i.e. IContentItemDisplayManager and IDisplayManager.cs IContentDisplayHandler and IDisplayDriver<TModel> (Edit: Corrected interface names )

[jtkech]

Maybe some fast paths, here or before by the caller in BindPlacementAsync()

E.g.

        if (placementResolvers.Count == 1)
        {
            return placementResolvers[0].ResolvePlacement(placementContext);
        }

[TFleury]

Aggregate helper seems to be efficient (See source), do you think this will make a difference ?

I optimized CombinePlacements to return immediatly when first is null. (only one comparison against three before)

[jtkech]

Okay cool then ;)

Yes, i'm used to look at the dotnet runtime repo directly, e.g. here for Aggregate.cs

I did some apache benchmarks but didn't see any significant difference

[jtkech]

@ns8482e I took a quick look at your suggestion in your above comment, the problem is that BindPlacementAsync() may be called in any BaseDisplayManager derived classes, e.g. also in DefaultContentDefinitionDisplayManager.

@TFleury i left you some suggestions, otherwise LGTM

Combining the placements looks good but i'm thinking about perf, so not sure at 100%, maybe better to just use the 1rst non null placement as you did before. So maybe better to wait for the next triage, but it should not block this PR

[ns8482e]

@ns8482e I took a quick look at your suggestion in your above comment, the problem is that BindPlacementAsync() may be called in any BaseDisplayManager derived classes, e.g. also in DefaultContentDefinitionDisplayManager.

@jtkech Exactly - In my comment above I was suggesting to have event/hook just before calling await _handlers.InvokeAsync and after await BindPlacementAsync(context); - assuming that the event should be provided in all DisplayManager.

See https://gist.github.com/ns8482e/a0cb2399a2849af0ef9e335b28567a27 to achieve similar behavior, I had to add PlacementDisplayHandler with -100