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

Don't let 'ScrollEffect.reset()' set 'is_manual' to True #7775

Merged
merged 3 commits into from Feb 17, 2022

Conversation

gottadiveintopython
Copy link
Member

@gottadiveintopython gottadiveintopython commented Jan 29, 2022

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 #7721.
This is a substitute for #7727.

The cause of the issue

KineticEffect.is_manual should be True only when there is a touch performing a scroll. But under a certain circumstance, it can be True even though no touch events are in progress. You can confirm it by watching the is_manual property while running the #7721 example and resizing the window like shown in the #7721 video.

from kivy.app import App
from kivy.lang import Builder
from kivy.uix.boxlayout import BoxLayout
from kivy.uix.button import Button

KV = """
<BugScroll>:
    ScrollView:
        id: sv
        effect_cls:'ScrollEffect'
        BoxLayout:
            size_hint_y:None
            height:self.minimum_height
            orientation:'vertical'
            id:box
            always_overscroll: False
"""
Builder.load_string(KV)

class BugScroll(BoxLayout):
    def __init__(self, **kwargs):
        super().__init__(**kwargs)
        for i in range(100):
            self.ids.box.add_widget(Button(size_hint_y=None, height=55, text=str(i)))

class Program(App):
    def build(self):
        return BugScroll()
    def on_start(self):
        self.root.ids.sv.effect_y.bind(is_manual=lambda __, value: print(f"is_manual: {value}"))
        

if __name__ == '__main__':
    Program().run()
issue7721.mp4

Because of that, ScrollView uses its previous size while re-positioning its content when it gets resized. The following code is where it happens.

kivy/kivy/uix/scrollview.py

Lines 624 to 627 in 315b742

if self.effect_x.is_manual:
sw = vp.width - self._effect_x_start_width
else:
sw = vp.width - self.width

kivy/kivy/uix/scrollview.py

Lines 639 to 642 in 315b742

if self.effect_y.is_manual:
sh = vp.height - self._effect_y_start_height
else:
sh = vp.height - self.height

How is_manual ends up being True after a touch performing a scroll ends

  1. When a touch performing a scroll ends, is_manual is set to False
  2. The momentum of the scroll still exists so ScrollEffect.value will be updated for a while.
  3. If ScrollEffect.value exceeds the bounds(ScrollEffect.min,ScrollEffect.max), ScrollEffect.reset() gets called and is_manual will be set to True.

Why the issue doesn't occur when DampedScrollEffect is being used

It overwrites on_value() but doesn't call super().on_value(), which is the one who calls reset().

def on_value(self, *args):
scroll_min = self.min
scroll_max = self.max
if scroll_min > scroll_max:
scroll_min, scroll_max = scroll_max, scroll_min
if self.value < scroll_min:
self.overscroll = self.value - scroll_min
self.reset(scroll_min)
elif self.value > scroll_max:
self.overscroll = self.value - scroll_max
self.reset(scroll_max)
else:
self.scroll = self.value

@gottadiveintopython gottadiveintopython added the Component: Widgets kivy/uix, style.kv label Jan 29, 2022
@gottadiveintopython gottadiveintopython changed the title [WIP] Fix incorrect ScrollView's content position that happens when ScrollView gets resized while ScrollEffect is used Don't let 'ScrollEffect.reset()' reset 'ScrollEffect.is_manual' Jan 30, 2022
@gottadiveintopython
Copy link
Member Author

@HeaTTheatR
This might affect RouletteScrollEffect. StiffScrollEffect probably doesn't get affected for the same reason DampedScrollEffect doesn't.

@gottadiveintopython gottadiveintopython changed the title Don't let 'ScrollEffect.reset()' reset 'ScrollEffect.is_manual' Don't let 'ScrollEffect.reset()' set 'is_manual' to True Jan 30, 2022
Copy link
Member

@Zen-CODE Zen-CODE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gottadiveintopython Nice sleuthing. Works for me :-)

@Zen-CODE Zen-CODE merged commit 1c83cdf into kivy:master Feb 17, 2022
@matham matham added this to the 2.1.0 milestone Feb 17, 2022
@misl6 misl6 mentioned this pull request Feb 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Widgets kivy/uix, style.kv
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ScrollView effect_cls bug
3 participants