Add result-scripts event to apply Javascript to search results

[ThoWagen]

The purpose of this PR is to allow custom script to be run when new results have been loaded via JS. I'm not sure if this is the best solution. So please let me know if this should be done in a different way.

[demiankatz]

@ThoWagen, I was just talking to @crhallberg about this code, and he suggested that it would make a lot more sense to use an event system here instead of calling a function. If the code that currently calls initResultScripts instead triggered an event, then each module could have its own listener setup in its own init method to listen for that event and take appropriate action. That would greatly decouple the code and make this type of customization cleaner.

What do you think? Is it worth refactoring rather than adding this new mechanism? I think the result would be better, but I'm not sure if it's too much work at this stage.

I'm also requesting a review from @crhallberg in case he has anything to add.

(I'm open to considering this approach if events prove impractical, but I just think we should consider that possibility in a little more detail before moving forward with this solution).

[crhallberg]

Demian captured my thoughts here. This whole initResultScripts section could probably do with the same treatment - converting to VuFind.emit for VuFind.listen functions.

[ThoWagen]

@demiankatz @crhallberg I was not aware of VuFind's event system. I just realized that your suggested solution is already implemented just two lines after initResultScripts is called.

vufind/themes/bootstrap3/js/search.js

Line 347 in 8176e06

VuFind.emit('results-loaded', {

I am not sure if the rest of initResultScripts can be implemented like this since the init methods of the modules are called themselves.

[demiankatz]

@ThoWagen, I'm not sure if it would make sense to hook the existing results-loaded event, or to create a new results-scripts event to run the script hooks before announcing that the whole process is done through the existing results-loaded event. Maybe @crhallberg has an opinion. But I think we could start by trying either approach or the other and switch pretty easily if necessary.

You're right that we can't initialize the event hook in init, and then also have the event hook call init. I think we'd probably need to refactor the init methods so that there's a separate method like updateContainer(), and then init calls updateContainer and also provides an event listener to call updateContainer() again in response to the appropriate event.

[EreMaijala]

Note that results-loaded is currently only emitted by search.js when it loads a set of results, and that's not the case for the initial results page or other situations where initResultScripts is called (combined search, JS results disabled etc.). So I think it'd be better to emit a results-scripts event in initResultScripts for anyone to hook into. That'd be the simple change, but I'm not opposed to making this more event-based internally as well.

[ThoWagen]

I did not find any reference to the results-loaded event. @EreMaijala Did you add this because you use it in your own customization or was it added in advance for possible features in the future?

Since it seems to use some arguments of the search.js I added the new results-scripts event to initResultScripts and now the modules listen to that event and call a updateContainer method as @demiankatz suggested.

[EreMaijala]

@ThoWagen We use it, but it can be used by any downstream users to do stuff after results are loaded.

[demiankatz]

This looks good to me, and all tests are passing. I'm going to wait to merge until @crhallberg and/or @EreMaijala have had a chance for another review, just in case I'm overlooking anything.

[EreMaijala]

This looks mostly good to me, but I left a comment about the changes I'd still like to see.

Requested changes have been made.

[demiankatz]

Thanks, everyone! I have dismissed @EreMaijala's review since his feedback has clearly been addressed and he's out of office today. I hope he doesn't mind! :-)