Page MenuHomePhabricator

Improve loading process of the templates-in-use list
Closed, ResolvedPublic5 Estimated Story Points

Description

When fixing issues in the templates-used list (for T321032), we changed the loading process to also fetch template protection info and any missing interface messages required for protection levels. This means that the loading (which can happen very frequently when Realtime Preview is enabled) is visually disruptive because the list is built progressively and so changes the vertical space in the page.

The current system a) fetches the preview, which comes with a list of templates in use; b) for batches of templates, fetches protection info (and if required, the system messages); and then displays the list item.

Instead, it should immediately display the whole templates-used list, and then progressively fix up each item with the additional info. This would still mean that there are visual changes, but no extra list items would be added so it'd be a much smoother display. Non-existing templates would still be displayed as redlinks initially, as this info is available from the first action=parse API call.

We could either not show the edit link initially, or show it and then change it later if editing isn't permitted for the current user.

Event Timeline

Why not just show the whole thing when it's finished including protections and system messages? there is already an overlay indicating that information is being fetched. @JSengupta-WMF, any thoughts?

Could be possible. It's hard to say without knowing what are the worst case scenarios and how often they occur. Showing an overlay showing wait state works fine for a few seconds but not if it takes few minutes to load.

The usual flow is that the list is already visible, and is made opaque (it's not an overlay; the links can still be clicked) while it's refreshed. For almost every reload, none or maybe a small number of list items will change. The idea with the above would be to not change the height of the list while it's being refreshed.

It'll never take more than a few dozen seconds to load, I think. If for some reason the fetching of permissions and messages doesn't work, it'll (actually… let me check what will happen then…) — but by building the list earlier, we avoid that error state impacting the actual list of templates (it'll just break the permissions stuff; the edit links will remain as edit even if the user can't edit them). But really that seems like a rare occurrence.

Instead, it should immediately display the whole templates-used list, and then progressively fix up each item with the additional info. This would still mean that there are visual changes, but no extra list items would be added so it'd be a much smoother display. Non-existing templates would still be displayed as redlinks initially, as this info is available from the first action=parse API call.

Yes, please. I've found it frustrating that the list of templates does not appear immediately since T321032 (even more so now that the items appear one by one).

what if we don't refresh the list when updating via RTP automatically and rather add an option for a manual refresh?

The list should definitely be refreshed at least in the normal live preview (i.e. clicking "Show changes").

KSiebert set the point value for this task to 4.3.Jun 6 2023, 1:34 PM
KSiebert changed the point value for this task from 4.3 to 5.

We talked about this in the CommTech planning meeting yesterday, and think that a) it's worth looking again (although previously it was to add redirect info) at changing the action=parse API to include the protection info there (see previous discussion here); and b) if that's still not a good idea, then go ahead with the approach outlined above (immediately display the whole templates-used list, and then progressively fix up each item with the additional info).

A manual refresh is already possible via live preview. Adding the same to RTP seems like it'd make it less useful. This task is really about fixing up the ordering of the API requests so that we can avoid the visual shuffling that's happening currently as the list is rebuilt.

Change 934039 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/core@master] preview: Insert template list after it's all built, rather than before

https://gerrit.wikimedia.org/r/934039

Change 934039 merged by jenkins-bot:

[mediawiki/core@master] preview: Insert template list after it's all built, rather than before

https://gerrit.wikimedia.org/r/934039

It looks like I was perhaps overthinking this earlier, and the above fix for just holding off on showing the list until it's all been retrieved is enough to fix the UI issues. With this fix, the length of the templates list only changes when templates are added/removed, and so there's no progressive lengthening of the list.

Moving this to QA. The main things to test are that the UI doesn't jump around annoyingly, and that the template list is still correct.

dom_walden subscribed.

Templates appear to load all at the same time.

When you reload realtime preview the list goes grey for a bit while the preview loads. There can be a bit of a delay between the list no longer being grey and the list actually updating. Perhaps it should remain grey until the HTML is updated.

I tested how it dealt with the API returning an exception either for the request for the template list or any of the request for page protection properties. It seems to fail silently, not updating the template list and keeping whatever was in the list before. This might lead to the list not being updated when a user might think it was. They can always reload the preview again and hopefully not get the same exception.

Test environment: https://en.wikipedia.beta.wmflabs.org MediaWiki 1.41.0-alpha (95a84c9) 07:42, 3 July 2023.
Test browser: Firefox 102.

By the way, for the batchSize we could leverage the high API limit. Something like:

var userGroups = mw.config.get( 'wgUserGroups' );
var titlesLimit = ( userGroups.indexOf( 'sysop' ) > -1 || userGroups.indexOf( 'bot' ) > -1 ) ? 500 : 50;

@Od1n good idea, but I think it maybe should check for the apihighlimits right rather than the group membership? Although I guess for most wikis what you've got there is enough, and it avoids another query.

A code such as:

mw.user.getRights().done( function ( rights ) {
    if ( rights.indexOf( 'apihighlimits' ) > -1 ) {
        /* ... */
    }
} );

The API query is light and executed only once per pageview, as it is cached using a singleton promise.
refs: https://doc.wikimedia.org/mediawiki-core/REL1_29/js/source/mediawiki.user.html#mw-user-method-getRights

However:
The code to raise the limit to 500 would improve scalability for the few users having the "apihighlimits" right, but for the (numerous) other users, it would be an overhead that brings no benefits.
Another trouble is that getRights() is asynchronous, and we can't use async/await for the time being, so it would complicate the code significantly.
Therefore, maybe we should just keep that 50 limit.

Yes, I agree. It's probably fine as-is then. Thanks for looking into it!