Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

iainlane
Copy link
Contributor

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.

@ximion
Copy link
Collaborator

ximion commented Jul 31, 2018

+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.

@robert-ancell
Copy link
Collaborator

This change seems overly complex - I think the solution is just to fix the scaling code to handle all requested sizes - see #256

@iainlane
Copy link
Contributor Author

iainlane commented Aug 2, 2018

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

@robert-ancell
Copy link
Collaborator

@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.

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.
@kalev
Copy link
Collaborator

kalev commented Apr 9, 2019

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
Copy link
Collaborator

kalev commented Apr 9, 2019

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

@iainlane
Copy link
Contributor Author

iainlane commented Apr 9, 2019

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?

@kalev
Copy link
Collaborator

kalev commented Apr 9, 2019

@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?

@ximion
Copy link
Collaborator

ximion commented Apr 9, 2019

@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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants