appstream-glib icon indicating copy to clipboard operation
appstream-glib copied to clipboard

as-image: Handle images which are smaller than the destination

Open iainlane opened this issue 6 years ago • 9 comments

We currently don't consider this case at all, which results in negative coordinates being passed to gdk_pixbuf_copy_area().

Let's instead look and see if the source image is smaller than the destination. If it is, we just need to put it in the middle of our desintation pixbuf.

This fixes a bug in gnome-software on Debian/Ubuntu where font previews are not shown, because they are smaller than the 1024x96 (?) that gnome-software is requesting and the code tries to upscale them. A later error makes this fail, but we should probably handle this by just never upscaling.

iainlane avatar Jul 30 '18 10:07 iainlane

+1 in general, but @hughsie has the last word. One thing that leaves me puzzled here is that I experience this bug when running GNOME Software on GNOME Shell (no screenshots for fonts get shown), but when I run GS on KDE Plasma, the bug does not appear and GS works as expected. So, for some reason GNOME Software works better on Plasma than on GNOME. I also have no idea why the DE would have any impact on the behavior of a image scaling/padding function, so at the moment I just admire this bug for its weirdness.

Regardless, Iains patch looks good to me and makes sense.

ximion avatar Jul 31 '18 08:07 ximion

This change seems overly complex - I think the solution is just to fix the scaling code to handle all requested sizes - see https://github.com/hughsie/appstream-glib/pull/256

robert-ancell avatar Aug 02 '18 00:08 robert-ancell

Thanks Robert. The outcomes are different between the two cases though, I think. In this one we never upscale, but just position the source image in the middle, and in the other one (AFAICS) you upscale the required dimension so it fits.

This PR: screenshot of this PR

#256: screenshot of 256

iainlane avatar Aug 02 '18 01:08 iainlane

@iainlane yeah, correct the other MR scales in all cases. Your method looks a lot sharper which seems the right solution here. I'm not sure what the intention of as_image_save_pixbuf is supposed to be - I guess we'll have to see what @hughsie says.

robert-ancell avatar Aug 02 '18 02:08 robert-ancell

The image scaling here is deliberate as much as I remember. We had a long long discussion in the past how to handle icons that aren't the correct size for gnome-software UI. The options were a) scale icons so that it fits gnome-software UI, but makes icons blurry, or b) show pixel-perfect icons and throw off gnome-software UI elements due to that.

It's mostly a tradeoff question. I don't think there's a right answer here. We used to have (b), but then we changed it to (a) so that we could rely on gnome-software UI looking as we designed it, and instead ask app authors to ship the correct size icons.

Maybe the answer is different for front screenshots and option (b) would make more sense, but in that case I'd like to make sure that the up scaling doesn't apply to regular icons, because this here is generic code that applies to both I believe.

kalev avatar Apr 09 '19 13:04 kalev

Can't you just fix the appstream data in Ubuntu to ship font screenshots that are correctly sized for gnome-software?

kalev avatar Apr 09 '19 13:04 kalev

Isn't the value that GNOME software uses essentially arbitrary? Not sure it would be right for the appstream generator to have knowledge of the value 1024×96 to be honest.

I think icons use AsIcon and this is essentially only for screenshots, or is that wrong?

iainlane avatar Apr 09 '19 14:04 iainlane

@hughsie, is this code just for screenshots?

@ximion Does it sound right or wrong for you to have appstream generator output 1024x96 px font screenshots?

kalev avatar Apr 09 '19 14:04 kalev

@kalev The AppStream spec is very explicit about icon sizes (there are rules that must be followed), but for screenshots there are only recommendations for apps and the software center has to deal with whatever it gets. That also means though that there would be nothing wrong with having appstream-generator produce arbitrary screenshot sizes for fonts, including 1024x96px (if that doesn't mess up other software centers that started to rely on other sizes).

ximion avatar Apr 09 '19 16:04 ximion