Created attachment 195058 [details]
test.jpg

This is a 400x600 JPG for testing.

Yes, I think you're right.

You can set the "width", "height" and "scale-to-fit" properties to avoid the bug.

Created attachment 195157 [details] [review]
cairo_image_surface_get_width

This patch at least fixes the size of the GooCanvasItem, though it crops the scaled-up image. Should we scale the CairoPattern too in that utility function, a bit like the code already does for scale-to-fit=TRUE?

Also, I don't know how to get the GooCanvas from a GooCanvasImageModel, so we can convert the pixels for it too.

The model objects can't access a GooCanvas (as they may be used by 2 canvas widgets at the same time).

Since using a model is sort of deprecated, we could just document the issue and workaround and ignore it.

Or to make it work we could leave the internal width and height settings at -1,
and calculate them when they are needed in update()/get_item_at()/paint().
This is easy to do, but may break old code that depends on the old "width" and "height" property behaviour.


Also, I don't think you can use goo_canvas_convert_from_pixels(). A new function like goo_canvas_convert_pixels_to_canvas_units() is needed.

(In reply to comment #4)
> The model objects can't access a GooCanvas (as they may be used by 2 canvas
> widgets at the same time).
> 
> Since using a model is sort of deprecated, we could just document the issue and
> workaround and ignore it.

Sounds fine to me, though we should really deprecate that model API.

> Also, I don't think you can use goo_canvas_convert_from_pixels(). A new
> function like goo_canvas_convert_pixels_to_canvas_units() is needed.

What's the difference?

(In reply to comment #5)

> > Also, I don't think you can use goo_canvas_convert_from_pixels(). A new
> > function like goo_canvas_convert_pixels_to_canvas_units() is needed.
> 
> What's the difference?

goo_canvas_convert_from_pixels() depends on the current canvas scale setting.
It is used for things like converting mouse positions to canvas coordinates.

Created attachment 195843 [details] [review]
0001-GooCanvasImage-Correct-the-size-when-using-units-oth.patch

OK. This patch adds new conversion functions and uses them. However, I still don't know how best to scale the image.

I've applied the patch, with minor changes.

To scale the image to fit the specified width/height people can set
"scale-to-fit".

Does it need anything else?

Thanks, but there are a couple of problems with that:

1. The item should really have the correct width/height without me having set a sensible size.
2. scale-to-fit does not keep the aspect ratio.


For my own purposes, it would probably be enough to have a keep-aspect-ratio property, but I think fixing the default size would be more generally correct.

Created attachment 197138 [details] [review]
0001-GooCanvasImage-Correct-the-image-appearance-when-usi.patch

This patch (which can be applied with git am) seems to fix it.

I'm not sure about the position of this call in goo_canvas_image_new():

  goo_canvas_image_convert_pixbuf_sizes (item, image_data);

It is now after the varargs are processed, which means it could be after "pattern", "width", and "height" properties are set.

But it should only be called when "pixbuf" is used, shouldn't it?

Created attachment 197486 [details] [review]
0001-GooCanvasImage-Correct-the-image-appearance-when-usi3.patch

Yes, you are right. How about this one? I have moved some things in the _new() function to earlier. It works in my test case.

We already respond to the pixbuf property setting. In fact, we could just choose to provide everything in one g_object_new() call, avoiding some duplication, but I guess you are concerned about performance.

Damon, what do you think? It would be great to get this in and then have a tarball version that Glom could depend on.

This change looks wrong:

-  item = g_object_new (GOO_TYPE_CANVAS_IMAGE, NULL);
+  item = g_object_new (GOO_TYPE_CANVAS_IMAGE, "pixbuf", NULL);


This should probably be removed too:
+  g_print("scale_to_units=%f\n", priv->scale_to_units);


Other than that you can commit if you want.

Thanks for catching them. Pushed. Thanks.

We still have the problem that the pixbuf cannot be scaled properly if the pixbuf is set before it is added (indirectly) to a canvas, and there is nothing that triggers a rescale when the CanvasImage is eventually in a canvas, so that goo_canvas_item_get_canvas() would succeed.

We can override CanvasItem::set_parent(), but that still doesn't help if the parent is not in a canvas yet either.

I feel like I need some way to know when goo_canvas_item_get_canvas() would start to return non-null. Unfortunately, it's not a property and it would be lots of work to make it a property.

I suppose all the goo_canvas_image_convert_pixbuf_sizes() stuff should be
done in the update() function. But only if a pixbuf is being used and "width" and "height" haven't been set explicitly.


Though to be honest this code isn't perfect anyway, as it assumes pixbufs are sized in pixels, whereas they may have any resolution. How much of that should GooCanvas try to handle? (I can't see a way to get the resolution from GdkPixbuf so that may be impossible anyway.)

If you are using units like mm or points I doubt you would want the image sized in pixels anyway, as it wouldn't fit in with the rest of the canvas, which is resolution-independent. (i.e. it will be a different size on different resolution displays.)

So I don't know what we should do here. Maybe just pick an arbitrary default resolution like 96dpi and scale to that. I think the developer will almost certainly be setting the width and height explicitly anyway, so it isn't too important.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/goocanvas/-/issues/26.