Exclude ContentWidgets layer by default in order to increase performance

[gdessimoneinvalle]

In reference to #8749

Added a filter to exlude widgets from list if the layer ContentWidgets is not explicity chosen by the user in order to open the widget page more quickly

[sebastienros]

@MatteoPiovanelli-Laser

[BenedekFarkas]

Suggested change

	string zonePreviewImagePath = string.Format("{0}/{1}/ThemeZonePreview.png", currentTheme.Location, currentTheme.Id);
	string zonePreviewImage = _virtualPathProvider.FileExists(zonePreviewImagePath) ? zonePreviewImagePath : null;
	var zonePreviewImagePath = string.Format("{0}/{1}/ThemeZonePreview.png", currentTheme.Location, currentTheme.Id);
	var zonePreviewImage = _virtualPathProvider.FileExists(zonePreviewImagePath) ? zonePreviewImagePath : null;

[BenedekFarkas]

Simplified logic with corrected variable naming.

Suggested change

	//Only when "ContentWidgets" layer is explicitly chosen the widgets are listed, not by default (for performance reasons)
	IEnumerable<WidgetPart> widgets;
	var IdLayerContentWidget = layers.Where(x => x.Name.ToLowerInvariant() == "contentwidgets").Select(x => x.Id).FirstOrDefault();
	if (IdLayerContentWidget > 0) {
	//Ok the layer exists
	var IdsLayers = layers.Where(x => x.Name.ToLowerInvariant() != "contentwidgets").Select(x => x.Id).ToList().ToArray();
	if (layerId != null && (int)layerId == IdLayerContentWidget) {
	//If ContentWidgets is chosen explicitly then IdsLayers is not filtered, note that the editors should be advised that this can take lot of time
	IdsLayers = layers.Select(x => x.Id).ToList().ToArray();
	}
	widgets = _widgetsService.GetWidgets(IdsLayers);
	}
	else {
	widgets = _widgetsService.GetWidgets();
	}
	// Only listing the widgets of the "ContentWidgets" layer when it's explicitly chosen, not by default (for
	// performance reasons).
	IEnumerable<WidgetPart> widgets;
	var contentWidgetsLayer = layers.Where(x => x.Name.ToLowerInvariant() == "contentwidgets").FirstOrDefault();
	if (contentWidgetsLayer == null) {
	widgets = _widgetsService.GetWidgets();
	}
	else { // The "ContentWidgets" layer exists.
	var layerIds = layers.Select(x => x.Id).ToList();
	// There's no layer selected or it's not "ContentWidgets", so excluding it from the widget listing.
	if (!layerId.Equals(contentWidgetsLayer.Id)) {
	layerIds.Remove(contentWidgetsLayer.Id);
	}
	widgets = _widgetsService.GetWidgets(layerIds.ToArray());
	}

[BenedekFarkas]

Suggested change

[BenedekFarkas]

When/what creates the ContentWidgets layer? On latest 1.10.x, I added a content widget to the home page, but there was no ContentWidgets layer created and I couldn't find any reference to other than this code.

[MatteoPiovanelli-Laser]

When/what creates the ContentWidgets layer? On latest 1.10.x, I added a content widget to the home page, but there was no ContentWidgets layer created and I couldn't find any reference to other than this code.

It's created by a feature. I think that's an issue with this PR. The optimization makes sense to me, because we also sometimes generate layers/widgets dynamically.
But the list of layers that are "filtered out" should be set in a Settings, rather than hardcoded, I think.

[BenedekFarkas]

OK, but which feature and where? I can't find any clue to this layer existing in the code.

[MatteoPiovanelli-Laser]

One of our own features (hence it wouldn't be in this repo) adds a layer with that same name, so it's likely to be that one. That feature can easily lead to having many widgets.

[BenedekFarkas]

In that case this code cannot be accepted like this.

Out of curiosity, how many Content Widgets are we talking about? Isn't the performance issue a SELECT N+1 for each of them? Maybe it's an inherent issue with Content Widgets we can address.

[MatteoPiovanelli-Laser]

Can be a few widgets, but it's easily hundreds. The feature lets you add widgets that are specific to specific ContentItems.
In the BO page that displays all widgets, I don't think we have an N+1 issue, because the queries are those from the Orchard.Widgets Controller/service, that simply gets all Widgets.
It becomes a potential performance bottleneck when it fetches all those widgets (each one being a contentitem), fires off all their handlers, then renders them out (grouping them by zone and layer, if I remember correctly).

[BenedekFarkas]

OK, thanks! I'll close this PR, but we can discuss under the issue if there's an Orchard-specific improvement that can help in this case too - I have an idea.