Correct image scaling on GTK

[freakboy3742]

In the process of checking box/child handling #1903 I ran the entire test suite, and noticed a regression in behaviour that the testbed suite wasn't picking up.

The imageview tests validate the size of the ImageView itself, and validate the aspect ratio handling of the image view, but doesn't validate the size of the actual image itself.

On every backend other than GTK, this doesn't matter, because the ImageView is responsible for scaling the underling image. The size of the widget and the scaling behaviour is all that needs to be validated.

However, on GTK, it's the responsibility of the user to scale the pixbuf being rendered by the ImageView to the appropriate size, performing any aspect ratio change etc. This worked when #1956 landed; but somewhere along the way, we've made a subtle change to the order of events fired, and as a result, some size change events don't result in a set_bounds call that causes a resize of the image. For example, if you have a valid layout with an ImageView, then change the image, the imageView changes size, but there's no set_bounds call, and as a result, you end up with a 200x100 imageview displaying a 640x480 image (the size that was present at the last set_bounds).

This has been corrected by performing the native rescaling in direct response to a GTK set-allocation signal - the underlying signal that tells GTK that the widget has changed size. At that point, we know the size the image should be, so we just need to make the pixbuf adhere to that size.

This is then backed up by an explicit test of the actual underlying image size. This test is a no-op on every platform except GTK, because the size of the rendered image isn't under user control.

It also corrects an edge case where the initial pixbuf set on an imageview wasn't necessarily being scaled to the correct size.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[mhsmith]

For example, if you have a valid layout with an ImageView, then change the image, the imageView changes size, but there's no set_bounds call

How is is possible for a native widget to resize without a set_bounds call? And it should be triggered after every image change, because of the refresh call at the bottom of ImageView.image.setter.

[freakboy3742]

Its not so much that the set_bounds doesn't happen; it's when the set_bounds gets the wrong size, because of where it happens in the drawing/rehint sequence.

In the tests, this manifested when setting a size on widget that already had a valid layout, or when changing the image itself (which implicitly changes the hints).

[mhsmith]

Even if set_bounds initially gets the wrong size, surely it gets the correct size in a later call before everything settles down, in which case the previous implementation would have resized the image at that time.

If it doesn't ever get the correct size, then we should fix that rather than working around it.

[freakboy3742]

Hrm - I thought this was the appropriate fix, as it was applying the image resize as a direct function of the actual widget changing size; but I can take another look in the morning if you feel there's something deeper that needs investigating.

[freakboy3742]

Also - it's not that it never gets the right size; there's just a specific sequence of changes that can cause problems, and those circumstances come up in the tests.

[freakboy3742]

After some detailed spelunking, I maintain the PR is correct, and it's not papering over any underlying problems (or, at least, to the extent that it is, it's a very deep problem that I don't think needs to be solved right now).

Take the test_explicit_size case as an example.

At the start of the test, there is a window whose only content is an unstyled ImageView ("the widget"), containing a 144x72 image ("the image"). The test will set an explicit width and height on the image, which should set the size of the widget and make the image fill the entire space of the widget, ignoring aspect ratio.

In the process of setting up the test, the image is set on the widget, which triggers a series of refreshes, and the widget is added to the window as main_content. This marks the widget and box as dirty, and triggers a layout recompute, resulting in a set_bounds call. set_bounds marks the container as dirty, indicating that GTK should perform a repaint.

The size that is provided to the set_bounds call is 640x452 - the full size of the window - because until this point, rehint has not been called on the widget. This is because on GTK, hinting only occurs once GTK does a container layout and requests a size allocation.

So - the first repaint occurs, the widget is rehinted, a size hint of 144x72 is determined, the layout is recomputed based on that layout, and the widget has it's GTK size set to 144x72. No set_bounds call is made, because we're in the middle of a container repaint; we recompute geometry by calling layout directly, but we don't call set_bounds because that would mark the container as dirty, which would trigger another redraw, ad infinitum.

This means that the only call to set_bounds to date has been made with a size of 640x452 - the full size of the window - but the widget has a size of 144x72, so at test start you see a zoomed up, fuzzy version of the middle of the full image.

One fix here would be to explicitly call rehint() when the image is set. This does fix the first rendering pass; the initial image rendering is the right size, because the hint on the image has been set correctly prior to the initial layout that calls set_bounds - but the first actual test in test_explicit_size still fails.

The test sets an explicit size on the widget of 200x300. Test setup makes 2 alterations to style - setting width and then height. Each change triggers a refresh, which marks the widget as dirty; this cascades up to the container, triggering a layout recompute, which invokes set_bounds, first with a 200x72 size, then with the 200x300 size. However, the image has not been rehinted, because no GTK layout has occurred; so the image is rescaled to 144x72 (fitting to the widget height and then maintaining aspect ratio to width), then to 200x100 (fitting to the widget width, and then maintaining aspect ratio to height). Even though both width and height have been set, aspect ratio is preserved because the widget has not been rehinted, so the old "144x72, preserve aspect ratio" hint is still in effect. A redraw then occurs, the image is rehinted (getting the now correct size of 200x300, without aspect ratio preservation), and the layout is updated to create a 200x300 widget with a 200x100 image inside.

The key observation is that in GTK, set_bounds isn't a reliable observer of the true hinted size of a widget - it's only an observer of the fact that layout has changed sufficiently to warrant a layout. The true size of the widget won't ever be known until GTK actually does a container layout.

Thus, I maintain that the PR is correct as presented. set_bounds() isn't the right place to be doing the image resize, because it's not guaranteed to have (a) a fully hinted image, or (b) the final layout size. The GTK widget will always have the right size; but we need to use the GTK size-allocate signal to get the notification that the size of the widget has been finalized.

set_bounds() modifications are common on other backends, but there's only one other usage on the GTK backend - in the SplitContainer - and there, it's really only being used to establish the ratio of the split so it can be applied later when the actual layout occurs.

That said - there may be a potential optimization here. The initial "false" layout that causes the set_bounds call to be triggered is happening at the core level. That layout is effectively redundant in GTK, because it's only serving to mark the window container as needing a redraw. It seems possible that if we were to push the call to do a layout down to the impl level, backends like cocoa could trigger a normal layout, but GTK could just mark the container as dirty. Then the GTK container layout could actually do a full layout, and call set_bounds with the actual size of the widget as a result of the final layout size. However, at least for now, I'm comfortable calling that a future optimization, falling into the same category as optimising layout so that changing width then height on a widget doesn't result in 2 complete layout passes.

[mhsmith]

The key observation is that in GTK, set_bounds isn't a reliable observer of the true hinted size of a widget - it's only an observer of the fact that layout has changed sufficiently to warrant a layout.

OK, that makes sense. I'd previously thought that this was introduced in #1794, but now I see that the GTK set_bounds was a no-op even before that, going back 6 years. For future reference, do you remember why this was done differently from the other platforms?

[freakboy3742]

The discrepancy stems from how GTK itself, and in particular the hinting process, works.

On every other toolkit, you can make a change (e.g., changing the text of a label), and that immediately changes the hinted size of the widget, so you can immediately recompute the layout that is affected by the change, and apply those size changes to reposition or resize the widget in the overall layout.

On GTK, changing the label doesn't immediately alter the hinted size of the widget. The widget doesn't change internal size until a size allocation is requested.

That then hooks into the layout process itself. On every other platform, we can (at some level) arbitrarily request that a widget be positioned at a specific location. This is what set_bounds() is doing. On GTK, the container is a widget that is able to position all it's descendents - but that means widget positioning is done as a single layout pass, disconnected from the original trigger that changed the location of the child widget.

So - at least at present, set_bounds() is being invoked at the wrong point in the layout process to be meaningful in a GTK context. It could be altered so that set_bounds becomes the literal position setting mechanism, invoked as part of the GTK layout pass, but that would require a more fundamental change to how set_bounds() calls are structured on all other platforms.

[mhsmith]

OK, my first thought was that Android might have the same problem, but it distinguishes between what size the widget would like to be (getMeasuredWidth and getMeasuredHeight), versus what size it currently is (getWidth and getHeight). Neither of these are immediately updated when you change the text of a label, but you can tell it to update the measured size by calling measure at any time.

Maybe there's some equivalent to this on GTK, but there's no need to look into it now.

[freakboy3742]

Maybe there's some equivalent to this on GTK, but there's no need to look into it now.

If there is, I haven't been able to find it. It seems to be related to the allocation process; but I haven't seen a way to directly trigger a re-allocation, short of triggering a whole-window redraw.