Carousel: Make 'Carousel' able to handle the case where 'loop == True' and 'len(slides) == 2'

[gottadiveintopython]

When a Carousel meets the following conditions:

• carousel.loop == True
• len(carousel.slides) == 2

it doesn't work properly, e.g. like this. This is probably because the next slide and the previous slide are the same under that conditions.

The tests for this PR aren't fully written because I don't have enough knowledge to automate GUI. Instead I'm using this, and testing manually.

[matham]

I would discourage using KV for things like this. KV should be reserved for actual styling rules that doesn't set the widget state, but just styles it based on the widget state and not for program logic. Adding program logic to KV makes the programs harder to trace as it violates the action at a distance principle.

[gottadiveintopython]

I like using KV like this because it's much simpler compared to doing the same thing in python. But if you don't think so, I don't do this anymore on this project.

Regardless of that, I'll remove that part because I'll make _prev_equals_next into non KivyProperty as you suggested.

[matham]

Unless the properties are needed for the users to be able to bind to, or it really simplifies the code, it is better to use a @property or normal python property rather than a KivyProperty.

That's because with Kivy properties it is harder to trace the code logic (due to action at a distance), and they add loading performance cost to the widgets.

So for example, _prev_equals_next doesn't seem to benefit at all from being a KivyProperty, so it should just be a normal property. But even _prioritize_next, I would make it a normal property and inline the code in on__prioritize_next at the one place you set it. Or you can make it a python @property with a setter that does the stuff currently in on__prioritize_next when set.

[gottadiveintopython]

I agree with you. I'll make those into non KivyProperty.

[matham]

It is generally better to directly fbind _insert_visible_slides to loop in the __init__ method, than to use the on_loop method. I know this principle is frequently violated in the Kivy codebase (even in on_slides below), but I try to fix it when I happen to work on code that does this.

The problem with this pattern is that if a user inherits and overwrites the on_xxx method for their own reason, we lose all the stuff that we wanted to happen as a result to xxx changing. So it's better to bind directly. For this case you'd need to bind a local function that eats the args passed from the callback, but it's still better.

[gottadiveintopython]

Yes, I understand what you mean. I'll fix that.

[matham]

This smells to me like it should be an assert. Logging errors is generally discouraged because it trains people to ignore them, and if this path is hit, users would see an error without much for them to do. This seems like if this path is hit it would be a bug in kivy if I read this correctly, hence an assert is more appropriate.

[gottadiveintopython]

This may be related to the order of evaluation. I mean things like this:

from kivy.lang import Builder
from kivy.factory import Factory
from kivy.properties import StringProperty


class TestEvaluationOrder(Factory.Widget):
    prop1 = StringProperty()
    prop2 = StringProperty()

    def apply_class_lang_rules(self, *args, **kwargs):
        def handler(c, self, *args):
            print(c, ':', self.prop2 == self.prop1 + ' prop2')

        self.fbind('prop1', handler, 'A')
        r = super().apply_class_lang_rules(*args, **kwargs)
        self.fbind('prop1', handler, 'B')
        return r

KV_CODE = '''
<TestEvaluationOrder>:
    prop2: self.prop1 + ' prop2'

TestEvaluationOrder:
'''
w =  Builder.load_string(KV_CODE)
w.prop1 = 'prop1'

A : False
B : True

Logging errors is generally discouraged because it trains people to ignore them

got it

[gottadiveintopython]

Thanks for your reviews as always

[matham]

I didn't test, but the changes look good. Let me know when you think it's ready to merge by removing WIP from the title.

[gottadiveintopython]

Well didn't pass Travis CI.
I'm OK with either of 1d2ce89(passed) or 9506778(didn't pass), because 9506778 is not related to the PR.

[matham]

I didn't test, but it looks ok. The travis CI osx failures are unrelated.