kivy / kivy

Open source UI framework written in Python, running on Windows, Linux, macOS, Android and iOS
https://kivy.org
MIT License
17k stars 3.04k forks source link

Fix AnchorLayout children height bug (#8669) #8670

Open nanouasyn opened 1 month ago

nanouasyn commented 1 month ago

Maintainer merge checklist

8669

AnchorLayout saves the size of the child widget before recalculating it and putting it back. Setting size means that width will be set first, and then height will be set. If width handler sets a new height value, AnchorLayout will immediately replace it with the height value that it was originally going to set. This is not what it should do, in case it does not control the height of the child widget. It should not set the height at all if the size_hint_y value forbids him to change it. Even if it is going to set the same value as it was before, it is quite possible that in the process the value will change and the child widget will get the wrong height.

misl6 commented 1 month ago

That makes sense!

This is the kind of issue that may show up again in the future if nothing blocks us from re-introducing the bug, would you consider creating a simple test that exercises the issue?

nanouasyn commented 1 month ago

@misl6 I'm sorry. I can't build a lib from sources. Therefore, I can't run the tests. In this particular case, the fix was small and obvious, but writing tests blindly, without a clear understanding how testing works in the project, would be risky. In addition, all layouts are susceptible to such a potential error. Thus, we need to cover them all with tests. But as far as I can see, so far there are no such tests anywhere. Perhaps it is necessary to create an issue about the coverage of all layouts with such tests?

nanouasyn commented 1 month ago

@misl6 It seems to me that the test can be like this:

def test_anchorlayout_no_height_control(self):
    from kivy.uix.widget import Widget
    from kivy.uix.anchorlayout import AnchorLayout

    def set_height_equal_width(widget, value):
        widget.height = widget.width

    widget = Widget(size_hint=(1, None))
    widget.bind(width=set_height_equal_width)
    layout = AnchorLayout(size=(1000, 1000))
    layout.add_widget(widget)

    layout.do_layout()

    assert widget.size == [1000, 1000]
nanouasyn commented 1 month ago

A new fix. Before this commit, the new positions of child widgets were calculated based on the height or width that either the widget had before we started resizing, or that we set ourselves. But if, for example, the height changed by itself, due to the fact that we changed the width, the calculation of positions was based not on the new height, but on the outdated height. Therefore, it is necessary to reload the actual size after resizing so that the positioning remains correct in case the size in a dimension that we do not control has changed while resizing in another dimension. My previous commit corrects size errors in such situations, this one corrects positioning errors.

nanouasyn commented 1 month ago

@misl6 It's done. Tests have also been added.