RecycleGridLayout : Fix layout when number of widgets match number of columns

[gottadiveintopython]

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.

fixes #7255

I put "WIP" in the title because I feel like there is a better implemetation.

[tshirtman]

The parametrised test seems to repeat the implementation, i think it's better to be less smart in such cases and be explicit about tho expected result, each parameter could be a tuple of (input, exptected_output) instead.

Edit: i think it's a situation where a few lines of comments explaining the loogic of the implementation would be fine, up to you.

[matham]

The parametrised test seems to repeat the implementation, i think it's better to be less smart in such cases and be explicit about tho expected result, each parameter could be a tuple of (input, exptected_output) instead.

Agree. But also, this is not testing the implementation directly, but rather an analog of the implementation. So if one changes the implementation, the test would not be helpful anymore. So it's better to test the actual implementation and use hard-coded input/output values as @tshirtman suggested.

[matham]

I think I wasn't clear enough with what I was trying to say, but could you test an actual gridlayout widget? Beause this basically tests that the current implementation is current. But if someone changes the gridlayout in the future, the test will not test that.

[gottadiveintopython]

@tshirtman Ah sorry for my explanation being not enough. There is a situation where we cannot just sort the view indices in ascending order as the latest commit shows.

@matham No, you were clear enough. I pushed the latest commit before I read your comment.

it's better to test the actual implementation

OK, so is it ok to create a new function that implements that specific part, and use it in the unittests? or I better to use compute_visible_views() itself in the unittests somehow?

[gottadiveintopython]

When I implemented the 'orientation', I left the original code as possible in order to avoid introducing new bugs. But maybe this compute_visible_views() should be re-written entirely (if I come up with good implementation) to avoid this complicated unittests.

[matham]

Yeah, I'd say to call compute_visible_views directly and ensure the it returns them in the correct order.

Or even better, create a actual recyclegridlayout (and normal gridlayout) and set the data/children, run do_layout or whatever is required to do the layout, and then check that the children widgets are at the expected positions. But compute_visible_views is enough if you don't want to do that.

[matham]

But maybe this compute_visible_views() should be re-written entirely (if I come up with good implementation) to avoid this complicated unittests.

Ultimately, it'd be nice if we can test that the widgets are laid out correctly, irrespective of the implementation. So I'm not sure if that would help.

[matham]

Perfect. Just one more thing, it's probably better if you use the clock fixture rather than the raw clock.

[gottadiveintopython]

Test_layout.compute_layout() is not a test case, which means it cannot use any fixture directly, which means I have to do something like this:

class Test_layout:
    def compute_layout(self, *, clock):
        ...
    def test_case1(self, kivy_clock, ...):
       self.compute_layout(..., clock=kivy_clock)
    def test_case2(self, kivy_clock, ...):
       self.compute_layout(..., clock=kivy_clock)

This is not desirable, isn't it?

[matham]

This is not desirable, isn't it?

I think that's fine!? It's just more verbose, but I don't really see another way that is not hacky.

[gottadiveintopython]

@matham OK, how about the following fixture? Do you think it's a good idea?

# kivy/tests/fixtures.py
@pytest.fixture(scope='module')
def module_scope_kivy_clock():
    # same as kivy_clock


# kivy/tests/test_uix_recyclegridlayout.py
global_kivy_clock = None

@pytest.fixture(scope='module', autouse=True)
def acquire_kivy_clock(module_scope_kivy_clock):
    global global_kivy_clock
    global_kivy_clock = module_scope_kivy_clock
    yield module_scope_kivy_clock
    global_kivy_clock = None

class Test_layout:
    def compute_layout(...):
         global_kivy_clock.tick()

I myself am not sure this is good or not.

(Also, don't merge the PR yet.)

[gottadiveintopython]

TestLayout_all_the_data_is_visible might be unnecessary if TestLayout_only_a_part_of_the_data_is_visible exists.

[matham]

OK, how about the following fixture?

I think the way it is currently is better. It's simpler even if more verbose, which is not really a big deal.

TestLayout_all_the_data_is_visible might be unnecessary if TestLayout_only_a_part_of_the_data_is_visible exists.

Extra tests have never hurt anyone 😄

[matham]

Feel free to merge if you're happy with it.

[gottadiveintopython]

@matham I think the pr is mergable now. I'd appreciate if you approve it again :)