Add option to disable media preview

[dalcde]

This disables image previews iff prefetchStorage is disabled. This
stops the client from making any requests to third-party sites.

The change is backwards comaptible. Existing config files will have
disableMediaPreview set to undefined, which is functionally equivalent
to false.

Help wih setting up tests would be appreciated. I couldn't figure out how to
test that a preview is not displayed.

[xPaw]

I think parseHtmlMedia should always resolve to false with this option so that HTML pages will still show a description, instead of no preview at all.

And for direct media, not sure why do the http request too, if it won't be displayed, the request could be avoided in the first place. Same for thumbnails from html.
EDIT: Although we would still want to display link size?

Existing config files will have
disableMediaPreview set to undefined, which is functionally equivalent
to false.

TL falls back to default config for missing keys, FYI.

[dalcde]

Updated. Thanks!

[xPaw]

Can you add tests for this?

[dalcde]

I'm not sure how to do this. A "loading" preview exists by default and then is removed asynchronously. Checking if the preview still exists after a timeout seems prone to errors.

[xPaw]

You can still test things like not picking videos from html.

[dalcde]

I've added a test, but I'm not a huge fan of this. It modifies the global config object, which has to be restored at the end of the test. This seems pretty prone to errors in the future.

[xPaw]

You can group that test with describe and then use before and after to correctly reset it: https://stackoverflow.com/a/37897703

I also would like to see more tests like a direct link to image where it should not produce an image thumbnail, but still return the image size.

[dalcde]

Done!

[xPaw]

I don't see anything changed.

[dalcde]

Sorry forgot to push!

[xPaw]

You don't need done as there is nothing async. Same in afterEach.

[xPaw]

"I also would like to see more tests like a direct link to image where it should not produce an image thumbnail, but still return the image size."

Can you add more tests please?

[dalcde]

On Sun, Jul 19, 2020 at 08:16:22AM -0700, Pavel Djundik wrote: "I also would like to see more tests like a direct link to image where it should not produce an image thumbnail, but still return the image size."

At the moment this does not happen. It appears that the image size comes as part of the preview. If I don't return a preview, there will be no accompanied size. How would I achieve this?

[xPaw]

Ah yeah you're right that we can't do that. In that case we can avoid the image thumbnail request here:

thelounge/src/plugins/irc-events/link.js

Lines 106 to 107 in b770c33

	// Verify that thumbnail pic exists and is under allowed size
	if (thumb.length) {

As it will be dropped later on anyway. Again, I would like to see tests for it (e.g. to see that msg:preview triggered, but thumbnail url was NOT visited).

[dalcde]

I added the test, and added a similar test for videos.

[xPaw]

Thanks, that looks better now. Did you test that throw actually works there? I was thinking of tracking a bool there instead. But if throw works, that's fine too.

I would like to see tests for direct image/video links too (without html), for that you would need get routes that return correct content-type though.

That way we can also verify that direct links do not generate previews. Not sure how we would track removePreview calls though, since it won't emit a preview event.

Sorry if I'm being catious/annoying, I just want the link prefetch code to be test covered to avoid any problems in the future.

[dalcde]

On Mon, Jul 20, 2020 at 09:18:28AM -0700, Pavel Djundik wrote: Thanks, that looks better now. Did you test that `throw` actually works there? I was thinking of tracking a `bool` there instead. But if throw works, that's fine too.

Throw actually works. After all, our expect calls just throw when the unexpected happens (we end up getting a Timeout instead of an assertion failure, both with expect and the throw)

I would like to see tests for direct image/video links too (without html), for that you would need get routes that return correct content-type though. That way we can also verify that direct links do not generate previews. Not sure how we would track `removePreview` calls though, since it won't emit a preview event.

Yeah I couldn't figure out how to do that

Sorry if I'm being catious/annoying, I just want the link prefetch code to be test covered to avoid any problems in the future.

No worries. Tests are important!

[xPaw]

There's a /real-test-image.png route available for all tests that you can use.

For videos and images, you can set the type in express route, something like this: https://stackoverflow.com/questions/51661744/how-to-set-content-type-when-doing-res-send/51661772

[dalcde]

On Tue, Jul 21, 2020 at 12:49:19AM -0700, Pavel Djundik wrote: There's a `/real-test-image.png` route available for all tests that you can use. For videos and images, you can set the type in express route, something like this: https://stackoverflow.com/questions/51661744/how-to-set-content-type-when-doing-res-send/51661772

Yeah but I'm not sure about the part where I check that the preview was removed.

[xPaw]

Yeah that is a bit tricky. One solution I can imagine working is creating a Proxy on message.previews and the first change to the array would be type loading as link prefetcher discovers the link, and the second one would be the array emptying as it get removed.