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

Fix AnchorLayout children height bug (#8669) #8670

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nanouasyn
Copy link
Contributor

@nanouasyn nanouasyn commented Apr 3, 2024

Maintainer merge checklist

  • Title is descriptive/clear for inclusion in release notes.
  • Applied a Component: xxx label.
  • Applied the api-deprecation or api-break label.
  • Applied the release-highlight label to be highlighted in release notes.
  • Added to the milestone version it was merged into.
  • Unittests are included in PR.
  • Properly documented, including versionadded, versionchanged as needed.

#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
Copy link
Member

misl6 commented Apr 6, 2024

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

nanouasyn commented Apr 7, 2024

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

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.

None yet

2 participants