search

OrchardCMS / Orchard

Orchard is a free, open source, community-focused Content Management System built on the ASP.NET MVC platform.
https://orchardproject.net
BSD 3-Clause "New" or "Revised" License
2.37k stars 1.12k forks source link

Performance issue when opening the widget admin page #8749

Open gdessimoneinvalle opened 5 months ago

gdessimoneinvalle commented 5 months ago

https://github.com/OrchardCMS/Orchard/blob/97648ed5a2a6c43074216786126ff8a207e6ac2f/src/Orchard.Web/Modules/Orchard.Widgets/Controllers/AdminController.cs#L87

When many content pages contain widgets we experience perfomance issues as the widget admin page takes even some minutes to open

BenedekFarkas commented 4 months ago

Continuing from #8750:

A generic-purpose solution to this could be to optionally move the layer filtering to the server-side. Right now, when you open the Widgets page and select any layer, you still see the widgets from each layer and you can hide/show the widgets of each layer on the client-side using the layer list on the right side.

This could be overridden for example by adding a site setting, which, when enabled would filter the displayed widgets down to the selected layer only by using an override of GetWidgets (with currentLayer.Id passed into it) at the place highlighted by @gdessimoneinvalle above. Since this would make the client-side filtering useless, that should be hidden too. A new, "Show all" option (with value 0) needs to be added to the layer dropdown too.

It can be taken one step further by the above site setting just providing a default value and the Widgets screen having an additional checkbox for the same purpose, but its value stored in a cookie, so admins can switch between the two behaviors for themselves.

CC: @MatteoPiovanelli-Laser

MatteoPiovanelli-Laser commented 4 months ago

Just for the sake of discussion, the things we could filter widgets on for that action are:

  1. Layer: this is what we are discussing, and I think this is what makes the most sense to have in a site setting.
  2. Zone: the usefulness of this in the context of this controller/action is debatable. Personally, I think the job of this controller is to display everything, but, since it's grouping things by Zone anyway, it may be worth it to set that up as a filter. Overall, I think this would be overengineering this feature.
  3. ContentType: I think it makes little sense to have a setting to filter this page for this.

So I think the site setting should definitely let admins select default layers whose widgets are loaded/shown; probably nothing else. I think the UI for client side filtering may still be there, but rather than operate entirely client-side, it may call a server-side api for the updated information.

I like the approach of using the cookie for admins so they may override their own setting, but I'm not sure I'd actually implement it, unless there is a trivial way to handle this: Suppose the default (site setting) is to show Layers 1 and 2, and that leads to a fast load for that page. As and admin, I may need to check something from one of the "slow" Layers: if that is saved as my cookie, the next time I may want to check something, I'm subjected to the slow load time; this will likely repeat itself several times until I happen to change the setting/cookie for myself. So interacting once with the slow layer may end up degrading the UX making it worse and making it feel like the rest of the changes we are proposing aren't even there.

BenedekFarkas commented 4 months ago

What I meant by site setting is just a simple checkbox: When enabled, the filtering happens entirely on the server-side based on the dropdown and the right-side list on the Widgets page for client-side filtering is hidden, because it becomes dysfunctional.

To expand on that, the cookie-based setting is also just a checkbox that takes precedence over the site setting.

MatteoPiovanelli-Laser commented 4 months ago

I see. I would have, in the Site Setting, the list of all Layers, for the admin to select which ones are to have their widgets displayed/hidden by default in the list. Filtering happens server-side. Then in the Widgets page I'd keep the UI that's there to filter, but change what it does: rather than hide/display something that's already client-side, it would call a server-side api to update the list.

BenedekFarkas commented 4 months ago

That sounds good in principle, although I'd advise against such an overhaul of the UI unless you can keep the current behavior going (because it's not a common problem) and you really really need it to work like this.

On the other hand, you can address the performance issue by adding just a single checkbox setting and 1 line changed in the controller.

sebastienros commented 4 months ago

What does OC do? I believe we designed it to be efficient, and nobody has complained so far.