Bryan.TongMinh wrote:

2:1 would be most appropriate I think, so 360px. I believe that passing 360 as height to File::transform is sufficient for MediaHandler to box-fit the image, but I'd need to check that to be certain.

A related issue is that images that are smaller than the default thumbnail should not be scaled up, but kept the original size. Is that feasible? That would probably also half-way solve the problem of tall files, since the huge height is a side-effect of scaling up very narrow images.

Bryan.TongMinh wrote:

Hmm this is more difficult than I thought... How do you properly scale a 1x449 image to 360 pixels height? The proper height would be 0.80 pixels and for various reasons MediaWiki will floor this to 0.

Bryan.TongMinh wrote:

Patch which modifies the way target width and height are calculated

Fixed in attached patch, but I had to modify the backend parameter normalization functions is a nasty way to ensure that the minimum image width is always 1 pixel.

Attached:

are you planning to do anything else with this patch? Apply it, say? We'll at least look at it in triage.

Bryan.TongMinh wrote:

I'd like to have somebody look over the patch before I apply it. I'm not convinced of the sanity of my changes to Generic.php.

At the end of the day, such narrow images are an edgecase. I don't think it'd be that bad if they just weren't rendered in the thumbnail spot.

(In reply to comment #7)

At the end of the day, such narrow images are an edgecase. I don't think it'd
be that bad if they just weren't rendered in the thumbnail spot.

I'd rather have the patch that can handle edge cases somewhat sanely. If we just don't render, we'll end up explaining over and over why not rendering the thumbnail is the right thing to do.

Bryan.TongMinh wrote:

(In reply to comment #8)

(In reply to comment #7)

At the end of the day, such narrow images are an edgecase. I don't think it'd
be that bad if they just weren't rendered in the thumbnail spot.

I'd rather have the patch that can handle edge cases somewhat sanely. If we
just don't render, we'll end up explaining over and over why not rendering the
thumbnail is the right thing to do.

Indeed. It should also solve an edge case that is currently already occurring, when specifying a maximum height for an 1px-wide image. See http://test.wikipedia.org/wiki/Bug_28076

This patch seems to have two parts:

• fix Special:Listfiles specifically to use a maximum box height (easy one-line fix)
• a general change to thumbnailing box sizing

Is that general change to ensure to that we have a minimum width and height of 1? Will it affect anything else? Does our resizing code that tries to match ImageMagick need adjusting?

Bryan.TongMinh wrote:

(In reply to comment #10)

This patch seems to have two parts:

• fix Special:Listfiles specifically to use a maximum box height (easy one-line

fix)

Correct, that is indeed only the one line change to SpecialListfiles.php

• a general change to thumbnailing box sizing

Correct, that should fix the bug that is present on http://test.wikipedia.org/wiki/Bug_28076

Is that general change to ensure to that we have a minimum width and height of
1? Will it affect anything else? Does our resizing code that tries to match
ImageMagick need adjusting?

The change ensures a minimum width of 1 pixel. As for ImageMagick, we specify the exact size, not the shrink factor, so that should not be a problem.

I just ran parserTests and it appears that some errors appear which may be due to this patch.

I could just commit the SpecialListfiles.php patch and open a new bug for the thumbnail sizing bug... They are two only indirectly related bugs.

(In reply to comment #11)

I could just commit the SpecialListfiles.php patch and open a new bug for the
thumbnail sizing bug...

I think that would be good.

Bryan.TongMinh wrote:

Fixed in r87171, new bug logged on bug 28762.

• Bug 28872 has been marked as a duplicate of this bug. ***

Bryan.TongMinh wrote:

*** Bug 30605 has been marked as a duplicate of this bug. ***