[widgets/scrollview]Attempt to fix nested scrollviews

[misl6]

I think this could be an easy fix to get nested scrollviews scroll flawless.
I tested it on a real-world app.

Code to reproduce:

                ScrollView:
                    size_hint: 1, 1
                    effect_cls: 'ScrollEffect'
                    do_scroll_x: False
                    scroll_type: ['content']
                    GridLayout:
                        cols: 2
                        size_hint_y: None
                        height: self.minimum_height
                        ........
                        ScrollView:
                            effect_cls: 'ScrollEffect'
                            size_hint: .706, 1
                            do_scroll_y: False
                            scroll_timeout: 55
                            scroll_type: ['content']
                            GridLayout:
                                size_hint: None, 1
                                padding: 0
                                spacing: 0
                                cols: 1
                                width: self.minimum_width
                                orientation: 'vertical'
                              .......

[matham]

The fix is great. I tested it and it worked in multiple situations without introducing new issues.

Bug analysis

This kind of bug has been plaguing ScrollView for a very very long time, so following is an analysis of the bug relating to the code in on_touch_move: https://github.com/kivy/kivy/blob/master/kivy/uix/scrollview.py#L754.

Given a parent and child who are both ScrollView, if you drag the scroll bar of the child, in on_touch_down, the parent passes the touch to the child who uses it to start the drag - as appropriate.

Then, upon a move, the parent checks if self._touch is not touch, which is False so it next checks if touch.grab_current is not self which is True. Because it's not self and scrollview prefers to only handle stuff during a grab callback, it ignores the touch and returns and does not try the child. This is the bad part IMO.

Next, the system will start processing the grab callbacks so it calls the parent again. This time for the parent if self._touch is not touch and if touch.grab_current is not self are False because it previously got the touch, so then it checks if touch.ud.get(self._get_uid()) is None which is True, that tells it that the touch is not used by the parent so it passes it on to the children by calling super(ScrollView, self).on_touch_move(touch).

Finally, now the child gets the move event. For the child, if self._touch is not touch is False, but if touch.grab_current is not self is True. Now, because scrollviews don't handle touches except only during the grab callback, the child returns True. But, because of this, this is the end of the touch and the child never gets called with the touch where if touch.grab_current is not self is False.

Current fix

The implemented fix in the PR, replaces if touch.ud.get(self._get_uid()) is None with ``if not any(key.startswith('sv.') for key in touch.ud)). This means that if there's a child who subscribed to the touch, the parent immediately passes it on to the children by calling self.dispatch_children('on_scroll_move', touch). This is not a real fix, as it's more of a bandaid - e.g. you can see a potential issue if a sibling scrollview subscribed to the touch, this may missfire, but I couldn't cause this issue.

Ultimate cause

For me, the ultimate cause, is what I have mentioned previously elsewhere, is that we frequently use the following pattern everywhere, which I think should not be allowed, except if really needed:

if touch.grab_current is not self:
    return True

IMO, we should never delay handling an event until touch.grab_current is self, because if we delay handling the event, then the grab callbacks are handled outside and after the whole widget tree has been processed. To me that is asking for trouble and leads to these kid of bugs because it's hard to account for everything and in the past led to similar bugs.

Events should always be handled during the dispatching of the tree, and grab callbacks should only be used when we get a move or up event outside the widget.

[matham]

I discovered an issue with #5617. Running that example code and dragging the buttons will periodically cause a crash due to this PR. But, this PR did improve that issue, although it's not fully fixed (buttons are still sometimes pressed and vertical scrolling doesn't work).

[matham]

So I fixed the crash I mentioned by changing it to if not any(isinstance(key, str) and key.startswith('sv.') for key in touch.ud):. Along the way I also tried fixing #5617. However, it seems impossible to fix because the code will need to be re-written. Specifically, the problem is not just the grab, but rather that the status of the touch is a state machine, but with like 10 variables across which the state is distributed (i.e. whether _touch is None, the values in ud etc.), which is impossible to track correctly.

That's why we leak touches. Someone will have to rewrite this making sure all possible states are accounted for. Surprisingly, the example code in #5617 is an amazingly good test to find the issues in the scrollview.

[misl6]

So I fixed the crash I mentioned by changing it to if not any(isinstance(key, str) and key.startswith('sv.') for key in touch.ud):. Along the way I also tried fixing #5617. However, it seems impossible to fix because the code will need to be re-written. Specifically, the problem is not just the grab, but rather that the status of the touch is a state machine, but with like 10 variables across which the state is distributed (i.e. whether _touch is None, the values in ud etc.), which is impossible to track correctly.

That's why we leak touches. Someone will have to rewrite this making sure all possible states are accounted for. Surprisingly, the example code in #5617 is an amazingly good test to find the issues in the scrollview.

@matham
I've also encountered a similar issue during runtime in my app.
As you proposed could be solved it via PR #6255

And yes, I also think that the code have to be re-writtten.