Disable default cover images

[ThoWagen]

When no cover images are available we don't want to display any image. A solution could be to configure a completely white "no cove" image. However that invisible image can still be interacted with and that can be confusing.

This PR allows to disable default cover images completely. The endpoint returns a 404 status code instead. noCoverAvailableImage and noQRCodeAvailableImage in the config.ini are still providing default images by default.

Besides that jQuery was removed from covers.js.

[demiankatz]

Thanks, @ThoWagen. I confess that I haven't closely looked at the Javascript changes yet, but first a few questions about everything else...

[demiankatz]

I think we might want to change the QR code behavior so that it always uses "images/noQRCode.gif" if nothing else is configured; supporting the noQRCodeAvailableImage = false option doesn't seem to work well. There's no error handler on the QR code images, so the 404 causes an actual error to display in the browser... and since the QR codes are displayed inside a drop-down, it's pretty weird to open the drop down only to see a broken link inside it. Admittedly, the "no QR code available" image isn't a lot better, but at least it looks intentional.

I'm not proposing that we need to add a custom Javascript error handler for QR codes. This is a rarely-used feature, and a rarely-encountered error condition on top of that. But I think maybe if you change the default in line 71 of the QRCode Loader from ?? null to ?? 'images/noQRcode.gif' and remove the comment about setting noQRCodeAvailableImage to false in config.ini, that may be sufficient.

I've also done a code read of your other translations from jQuery to standard JS in the cover code, and it all looks good to me. However, I have not been able to test AJAX cover mode yet since I don't have access to the services that leverage that functionality. I wonder if it's worth building a Demo cover loader to exercise all the possibilities. I'm willing to do that if you think it would be helpful!

[ThoWagen]

Having the loading error handler for the record covers actually does not avoid having 404 error messages in the browser's console. Unfortunately, I did not find a way how to fix that. One could check the availability with a separate ajax request in JS before displaying the images. But that would result in double the requests and some delay when loading the images.

But I agree that we don't need to be able to deactivate the unavailable QR code images and that having an extra handler for that would not be worth it, since errors when loading QR codes should be very rare. I implemented you suggestion.

I also added a demo cover loader that selects one of three demo covers from the themes or no image. The selection is based on the checksum of the record ids. So it selects arbitrary results but in a reproducible way.

[demiankatz]

Thanks, @ThoWagen. I thought it would be useful to be able to test the ajaxcovers = true setting by adding backlink support to the demo. However, I couldn't figure out a way to make a single Demo driver handle both proxied and AJAX-driven cover cases, so I ended up just writing a separate DemoAjax class. Do you think it's worth keeping both of these, or might it be better to just remove the current Demo and replace it with DemoAjax, so the demo we provide covers the greatest number of use cases?

[demiankatz]

In any case, the good news is that my new demo class proves that your refactored cover logic handles backlinks correctly.

As for avoiding the 404 errors in the console, the best I can think of would be to do something other than a 404 response -- e.g. would it be possible to return a single pixel transparent image with something in the HTTP response that a listener could detect and use to eliminate the image? That might be a little cleaner in some other (unlikely but possible) scenarios like users with Javascript disabled. But I'm not sure if there's really a sensible mechanism to allow this.

[ThoWagen]

@demiankatz Thanks, I did not think about the backlink functionality. Unfortunately, the DemoAjax cover loader does not work on my system. The reason is, that I run my instance in a container and VuFind can not call it self using via "localhost:someport" which is provided as "serverurl". I could change my setup to fix that, but others might have the same problem then.

What dou you think about combining Demo and DemoAjax and using return 'file://' . $path; in getUrl and return $this->baseUrl . 'themes/' . $cover['theme'] . '/' . $path; in getMetadata? The backlink functionality could also be configured using the $key parameter.

As for avoiding the 404 errors in the console, the best I can think of would be to do something other than a 404 response -- e.g. would it be possible to return a single pixel transparent image with something in the HTTP response that a listener could detect and use to eliminate the image? That might be a little cleaner in some other (unlikely but possible) scenarios like users with Javascript disabled. But I'm not sure if there's really a sensible mechanism to allow this.

Good idea! We can not directly check the e.g. headers of the response in the "load" event. But we can check the size of the loaded image as long as we don't set it via css to some fixed value in the future or in local themes. We can detect the single pixel images and then hide them. This is a bit hacky but we avoid the errors in the console and also probably the servers logs. What do you think about that?

[demiankatz]

What dou you think about combining Demo and DemoAjax and using return 'file://' . $path; in getUrl and return $this->baseUrl . 'themes/' . $cover['theme'] . '/' . $path; in getMetadata? The backlink functionality could also be configured using the $key parameter.

Yes, that sounds reasonable to me. Do you want to do that, or should I? I don't think it's strictly necessary to use $key to configure the backlink functionality, since it will essentially work if ajaxcovers = true and get ignored otherwise. Might be an interesting avenue for future flexibility, though.

Good idea! We can not directly check the e.g. headers of the response in the "load" event. But we can check the size of the loaded image as long as we don't set it via css to some fixed value in the future or in local themes. We can detect the single pixel images and then hide them. This is a bit hacky but we avoid the errors in the console and also probably the servers logs. What do you think about that?

That sounds good to me! I thought we already had a 1x1 transparent GIF somewhere in the project, but I can't seem to find it, so we might need to reintroduce that part.

[ThoWagen]

Great! I will implement the changes tomorrow.

[ThoWagen]

Done.

I implemented the configurability of the backlink functionality because it it possible to use ajaxcovers without it, even if that is not really useful then.

[demiankatz]

Thanks, @ThoWagen, this is looking great!

I made one small adjustment to the Demo cover provider; I noticed that it generated different covers in AJAX vs. non-AJAX mode, likely due to the two methods passing ID parameters slightly differently and resulting in different crc32 results. I changed it so that if a recordid is provided, that is used in place of the serialized ID array, so that when writing tests, we can better predict behavior (especially if new identifiers are added in the future).

I also went ahead and wrote a Mink test to exercise some of the behavior now that the Demo provider allows it. However, I'm having trouble with the test intermittently failing. On some occasions, when it expects the cover to be hidden, it is not yet hidden. I have tried adding a delay-and-retry loop, but the problem seems not to go away once it has occurred. I'm not sure what is causing this -- a bug in the code, a flaw in my test, or some kind of bad browser behavior. (I think a flaw in my test is the most likely explanation, but I'm not sure what it would be).

Do you mind running this test locally and letting me know if you see the same issue? If so, I'd be interested in whether you have any ideas on how to fix it.

[demiankatz]

One other thing to consider: have you looked at #4065, which eliminates use of innerHTML? Maybe this can be adjusted to match the strategies found there.

[ThoWagen]

@demiankatz I also have problems with the test. First of all, when I run chrome not in headless mode the browser's image cache is used and setting the fallback image does not work anymore.

I also have to add some sleep() in the test for some cases to work.

The last test case always fails for me even with sleep but not when I run it myself by hand. My first guess is that the images is loaded before the "load" event listener is added.

I will look into it again next week.

[demiankatz]

Thanks, @ThoWagen! If your theory about the load event handler is correct, then maybe that's actually a potential timing problem that needs to be addressed.

Please let me know if you need me to look deeper into anything on this end. It would be nice if Mink offered a way to clear the browser cache, but I suspect it does not. Maybe there's some way to inject a hash onto image URLs to force things to behave properly, though that may be too much of a hack...