Limit preview for large image files #503
Conversation
|
@olivierlambert please rebase on master (as tests have been fixed, and the one test we do have is somehow related to your PR ;-)). I'm reading/testing your PR right now. |
|
@olivierlambert oh and on a more pendantic side, could you please squash your commits and reword commit message to explain what it does (it's good to mention the ticket but the commit msg should be sufficient to know what it's about). |
|
First remark : why sending an empty preview ? better not to show any IMHO (the "…" thing on the first link pasted, which is a 50MB image).
Second think, I'd suggest, setting the size on kB, there is little use setting it in octets IMHO :) |
| size = req.response.headers['content-length']; | ||
| } catch(e) { | ||
| size = {}; | ||
| } |
There was a problem hiding this comment.
Maybe something more explicit than just catching all exceptions ?(I know, some existing code does like yo did), + are you sure {} is a valid size ? ;-)
|
@olivierlambert ok, reviewed, see inline comments for details. Rather than fetching the image fully, a first timeserver-side and a second time, fully, client-side, I would suggest, doing a HEAD request to get the @olivierlambert what do you think about all my comments ? |
|
|
@JocelynDelalande : I don't know where to start to avoid fetching the image as you requested. Would you like to do it in this PR or maybe as an optimisation in another one? |
olivierlambert
changed the title
olivierlambert
force-pushed
the
master
branch
2 times, most recently
from
aed3c2e to
5b749d1
Compare
| @@ -60,7 +61,9 @@ function parse(msg, url, res, client) { | |||
| thumb: "", | |||
| link: url | |||
| }; | |||
|
|
|||
|
Ok, good for fixes :) thanks for your reactivity
I was thinking on this one, but ok, that's acceptable to do it in a second time. Starting point would be to call something issuing the HEAD request before that fetch() call. |
|
@olivierlambert just tested again, you removed the empty "grey frame" but still the three dots on grey background are showing up on too big images. Once that is fixed and the trivial whitespace thing, you'll have my 👍 |
|
@olivierlambert, any news on this? :) |
|
@olivierlambert bump ? it's almost done :) |
|
Didn't get any message/alert previously sorry :/ What about the "whiteline error"? I don't understand the issue there. And IIRC (1 month ago, I could be wrong...), there is no trivial way to remove the three dots. |
You include in your commit the removal of a white line, which seems something unrelated to your edit.
Do you think you could find some time to look again please ? (css may be the way ?) those three dots with no meaning at all are a really strange thing to see for the user. |
|
1/ Oh okay, no problem with that. 2/ I'll try to take a look today |
|
I can't avoid the
Or maybe I missed something, that's also another possibility ^^ |
|
Okay, you got exactly the same behavior for other type of content. Try to paste a link to a PDF for example, you'll have the same issue. It's not related to this PR, so I'll push my version without the white space, and I suggest to create a dedicated issue for the other problem :) |
olivierlambert
force-pushed
the
master
branch
from
5b749d1 to
4ae66f5
Compare
olivierlambert
force-pushed
the
master
branch
from
4ae66f5 to
e585874
Compare
|
@JocelynDelalande PR done with the latest bits of master. Also tested it, that's fine. |
Fair enough :) Thanks for digging it. 👍 |
|
👍 and merging. Thanks a lot @olivierlambert!!! Hope to see you again soon! |
Limit preview for large image files
|
You are welcome :) |
Do not display a preview of content length greater than given in the config file (default: 512kb)