Disable default cover images #4061
Conversation
There was a problem hiding this comment.
Thanks, @ThoWagen. I confess that I haven't closely looked at the Javascript changes yet, but first a few questions about everything else...
There was a problem hiding this comment.
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!
|
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. |
|
Thanks, @ThoWagen. I thought it would be useful to be able to test the |
|
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. |
|
@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
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? |
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
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. |
|
Great! I will implement the changes tomorrow. |
|
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. |
There was a problem hiding this comment.
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.
themes/bootstrap5/js/covers.js
Outdated
| img.remove(); | ||
| source.remove(); | ||
| if (typeof response.data.html !== 'undefined') { | ||
| container.innerHTML = VuFind.updateCspNonce(response.data.html); |
There was a problem hiding this comment.
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.
|
@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 The last test case always fails for me even with I will look into it again next week. |
|
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... |
|
@demiankatz I fixed the tests by adding an configuration that disables browser caches for cover images, by changing the response header and also adding a hash to the url. I also added an extra 'wait' statement to the test that assures that the images are loaded before the assertions. That also removed the problem of the hiding not working at all for me. |
There was a problem hiding this comment.
Thanks, @ThoWagen, at a glance this looks like it's in good shape now... however, one suggestion based on an unexpected shift in the landscape that occurred last week!
| public function __construct( | ||
| protected string $dynamicUrl, | ||
| protected CoverLoader $coverLoader, | ||
| protected Config $config |
There was a problem hiding this comment.
laminas-config has just been abandoned; I'm beginning work to remove it from the project. I'd suggest using a plain array here, and using ->toArray in the factory, so we don't introduce a new dependency on the abandoned project.
There was a problem hiding this comment.
It might also make sense to default this to an empty array so that you don't have to modify tests when there is no configuration being tested.
There was a problem hiding this comment.
Done.
Are there any problems with replacing laminas-config with arrays that would be good to keep in mind?
There was a problem hiding this comment.
My plan is to create a VuFind\Config\Config that provides backward-compatibility with Laminas\Config\Config as a first phase. We might decide to keep that in the long term, but I think it's more likely that we'll quickly deprecate it and try to phase it out in favor of arrays. Really, the Laminas Config object was mainly useful for accessing deep configs in older versions of PHP prior to the introduction of null coalescing. Now that we have ??, it's pretty easy to access deeply-nested array properties as long as you provide a default to fall back on.
There was a problem hiding this comment.
Thanks, @ThoWagen -- tests are passing and this all looks good to me now!
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.