Page MenuHomePhabricator

Add hooks (back) to Echo
Closed, ResolvedPublic

Description

There used to be several (not necessarily all at the same time, this is from git log -p):

ext.echo.overlay.beforeShowingOverlay
ext.echo.special.onInitialize
ext.echo.special.onLoadMore
ext.echo.updateNotificationCount

Now, there are none (we should take into account this is part of our public API, when designing them, and making changes).

We should consider which to add (not necessarily the same as before).

@Quiddity raised a use case for this, marking as read when you open the flyout, as a user script. T146294: Make a way for editors to have notifications automatically marked as read, when a flyout is opened

Details

Related Gerrit Patches:
mediawiki/extensions/Echo : master(re)Add JavaScript hooks to Notifications

Event Timeline

Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptSep 21 2016, 5:16 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Quiddity updated the task description. (Show Details)Sep 21 2016, 5:18 PM

Is beforeShowingOverlay supposed to be just before we render the notifications? Looking at a gadget that expects it (https://en.wikipedia.org/wiki/MediaWiki:Gadget-popups.js) it seems to be the case. I personally would've changed its name, honestly (beforeNotificationRender or something) but regardless, is there any documentation on any of these - and if not, where should we put this documentation now?

Also, ext.echo.special.onLoadMore is no longer relevant, because the special page has no loadMore (especially not in Javascript mode) -- should we get rid of it, and should we add, instead, a 'beforeShowingResults' (similar to 'beforeShowingOverlay') that triggers right before the current page's notifications are displayed? This will mean that a gadget like popups (above link) will work in the JS special page too, but I don't think we would want to use the same event, unless we rename the event itself to something more generic (rather than overlay-dependent) ?

Is beforeShowingOverlay supposed to be just before we render the notifications? Looking at a gadget that expects it (https://en.wikipedia.org/wiki/MediaWiki:Gadget-popups.js) it seems to be the case.

It's when the DOM element for the notification exists, but before it's shown to the user. I added this hook in May 2013, so the Echo front-end has changed since then...

I personally would've changed its name, honestly (beforeNotificationRender or something) but regardless, is there any documentation on any of these - and if not, where should we put this documentation now?

Yes, only for beforeShowingOverlay (despite no longer existing), in modules/hooks.txt. They should all be documented in the same place.

Also, ext.echo.special.onLoadMore is no longer relevant, because the special page has no loadMore (especially not in Javascript mode) -- should we get rid of it, and should we add, instead, a 'beforeShowingResults' (similar to 'beforeShowingOverlay') that triggers right before the current page's notifications are displayed? This will mean that a gadget like popups (above link) will work in the JS special page too, but I don't think we would want to use the same event, unless we rename the event itself to something more generic (rather than overlay-dependent) ?

A generic event sounds like a good idea, if the DOM structure passed to the hook can be exactly the same. Otherwise, it should be a different event (the client can find the relevant part in each listener, and then call an internal function).

Change 312940 had a related patch set uploaded (by Mooeypoo):
(re)Add JavaScript hooks to Notifications

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

Once this is reviewed/merged, we can fix up navigation-popups code, which listens to a nonexisting hook (https://en.wikipedia.org/wiki/MediaWiki:Gadget-popups.js)

Right now, navigation popups listens to a nonexisting hook, we should replace:

	mw.hook( 'ext.echo.overlay.beforeShowingOverlay' ).add( function($overlay){
		dynamicContentHandler( $overlay.find(".mw-echo-state") );
	});

(this also expected a nonexisting class structure in notifications, so it would've failed regardless of the hook)

with the new hook/operation:

	mw.hook( 'ext.echo.notifications.beforeRender' ).add( function($wrapper, $elements){
		dynamicContentHandler( $elements );
	});

I've tested this locally, and it works. It will also work with cross-wiki notifications that are loaded asynchronously, but fire the same hook with their relevant element group.

Just to make @Quiddity happy, after the code is reviewed/merged, we can fix navpopups (above comment) and we can easily add a gadget to "mark all alerts as read when opened"; I've created an experimental gadgets just to show how that will work. The code works for me locally with the hook, so it should work when the code is merged+deployed.

My experimental gadget is here - https://www.mediawiki.org/wiki/User:MSchottlender-WMF/GadgetEchoMarkAllRead.js
Warning: It's marking all alerts as read as soon as you open the popup, which I personally find destructive to my liking, but will fulfill the request in the parent task.

Change 312940 merged by jenkins-bot:
(re)Add JavaScript hooks to Notifications

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

Mooeypoo closed this task as Resolved.Apr 18 2017, 6:10 PM

Hooks were added back in, and there's a gadget that was requested. Closing. If there's any missing functionality regarding hooks, reopen or start a new task.