Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

fix Issue #607: add carousel widget #626

Merged
merged 7 commits into from Aug 19, 2012

Conversation

Projects
None yet
5 participants
Contributor

hansent commented Aug 8, 2012

new carousel widget as discussed here #607

Contributor

revolunet commented Aug 8, 2012

This is promising :)
There is a bug when we slide twice quickly before the first slide ended.
Also, when testing with a GridLayout and 4 Carousels, only 2 are displayed.
When a button is added to the carousel we cannot slide anymore (click is grabbed by button)

Contributor

hansent commented Aug 8, 2012

With button, isn't that expected behaviour? If you want to slide probably not the whole area should be a button in the interface?

@tito tito and 1 other commented on an outdated diff Aug 9, 2012

kivy/uix/carousel.py
+ index = NumericProperty(0)
+ '''Get/Set the current visible slide based on index.
+
+ :data:`index` is a :class:`~kivy.properties.NumericProperty`, default
+ to 0 (first item)
+ '''
+
+ orientation = OptionProperty('horizontal', options=('horizontal', 'vertical'))
+ '''Specifies the orientation in which you can scroll the carousel
+ Can be `horizontal` or `vertical`.
+
+ :data:`orientation` is a :class:`~kivy.properties.OptionProperty`,
+ default to 'horizontal'.
+ '''
+
+ min_move = NumericProperty(0.2)
@tito

tito Aug 9, 2012

Owner

On the naming, maybe we could reuse the same as "scroll_distance" used in scrollview ? Or just the "min_distance" ? for consistency :)

@hansent

hansent Aug 9, 2012

Contributor

we could add a "scroll_distance" like in scroll view, that lets you scroll even if your over a button like @revolunet mentioned.

The min_move here is value that determines, how much the slide has to be dragged to the side in order for it to advance to the next slide on release (or if less, ease back so the current slide is in center again)...can we think of a better name?
easing_threshold ?
drag_threshold?
*_edge ?

@tito

tito Aug 11, 2012

Owner

ok, distance VS threshold... Dunno then. distance_threshold ? or drag_threshold is ok.

@tito tito and 1 other commented on an outdated diff Aug 9, 2012

kivy/uix/carousel.py
+
+ loop = BooleanProperty(False)
+ '''Allow carousel to swipe infinitely. When the user reach the last page, he will
+ get the first page when trying to swipe next. Same effect when trying to swipe the
+ first page
+
+ :data:`loop` is a :class:`~kivy.properties.BooleanProperty`,
+ default to True.
+ '''
+
+
+ # private, for internal use only
+ _offset = NumericProperty(0)
+
+ def __init__(self, *args, **kwargs):
+ self._prev = None
@tito

tito Aug 9, 2012

Owner

Use ObjectProperty() too here

@hansent

hansent Aug 9, 2012

Contributor

you mean for _prev etc ?

@tito

tito Aug 10, 2012

Owner

yep, _prev, _offset (well, numeric here) etc

@tito tito commented on an outdated diff Aug 9, 2012

kivy/uix/carousel.py
+ self._offset = 0
+
+ def on_slides(self, *args):
+ self.index = self.index % len(self.slides)
+ self._insert_visible_slides()
+ self._position_visible_slides()
+
+ def on__offset(self, *args):
+ self._position_visible_slides()
+ #reached full offset, which switches index
+ if self._offset <= self.extent * -1:
+ self.index = self.index+1
+ if self._offset >= self.extent:
+ self.index = self.index - 1
+
+ @property
@tito

tito Aug 9, 2012

Owner

Nop! Use something like:

extent = AliasProperty(_get_extent, None, bind=('orientation', 'size'))

Same for all the others + add documentation

Owner

tito commented Aug 9, 2012

A little usage within the documentation would be great too :)
@hansent @revolunet - feel free to continue to work on that one until you reach a good version for master :)

Contributor

hansent commented Aug 9, 2012

That's the plan. Will fix extend and quick scroll (also using two touches
its scrolls twice speed right now)

Sent from my iPhone

On Aug 9, 2012, at 6:37 AM, Mathieu Virbel notifications@github.com wrote:

A little usage within the documentation would be great too :)
@hansent https://github.com/hansent
@revolunethttps://github.com/revolunet- feel free to continue to
work on that one until you reach a good version
for master :)


Reply to this email directly or view it on
GitHubhttps://github.com/kivy/kivy/pull/626#issuecomment-7611781.

hansent added some commits Aug 10, 2012

animation.py: add cancel_property and cancel_all methods. works just …
…like stop, but will cancel, instead of stop (so no on_complete event fired)
uix.carousel:
    * use properties for everything
    * correct handling of multiple / simultaneous/ quick successive touches
    * formatting to comply with style guide
Contributor

hansent commented Aug 10, 2012

should be fixed now. review and then merge?

@rubik rubik and 1 other commented on an outdated diff Aug 10, 2012

kivy/uix/carousel.py
+ self._offset = 0
+
+ def on_slides(self, *args):
+ self.index = self.index % len(self.slides)
+ self._insert_visible_slides()
+ self._position_visible_slides()
+
+ def on__offset(self, *args):
+ self._position_visible_slides()
+ #if reached full offset, switche index to next or prev
+ if self._offset <= self._extent * -1:
+ self.index = self.index+1
+ if self._offset >= self._extent:
+ self.index = self.index - 1
+
+ def on__drag_touches(self, *args):
@rubik

rubik Aug 10, 2012

Why double underscore? Is there some Kivy convention I am missing?

@hansent

hansent Aug 10, 2012

Contributor

the property itself has an underscore as the first charachter (convetion
for private variable)...so then: "on_" + "_offset" gives the double _

On Fri, Aug 10, 2012 at 2:09 PM, Michele Lacchia
notifications@github.comwrote:

In kivy/uix/carousel.py:

  •    self._offset = 0
    
  • def on_slides(self, *args):
  •    self.index = self.index % len(self.slides)
    
  •    self._insert_visible_slides()
    
  •    self._position_visible_slides()
    
  • def on__offset(self, *args):
  •    self._position_visible_slides(
    
    )
  •    #if reached full offset, switche index to next or prev
    
  •    if self._offset <= self._extent \* -1:
    
  •        self.index = self.index+1
    
  •    if self._offset >= self._extent:
    
  •        self.index = self.index - 1
    
  • def on__drag_touches(self, *args):

Why double underscore? Is there some Kivy convention I am missing?


Reply to this email directly or view it on GitHubhttps://github.com/kivy/kivy/pull/626/files#r1354555.

@rubik

rubik Aug 10, 2012

Ahh ok sorry! ;)

@tito tito commented on an outdated diff Aug 11, 2012

kivy/animation.py
@@ -152,6 +152,27 @@ def stop_all(widget, *largs):
for animation in set(Animation._instances):
animation.stop(widget)
+ @staticmethod
+ def cancel_all(widget, *largs):
+ '''Stop all animations that concern a specific widget / list of
@tito

tito Aug 11, 2012

Owner

missing .. versionadded:: 1.4.0

@tito tito commented on the diff Aug 11, 2012

kivy/animation.py
@@ -180,7 +201,22 @@ def cancel(self, widget):
def stop_property(self, widget, prop):
'''Even if an animation is running, remove a property. It will not be
- animated further.
+ animated further. If it was the only/last property being animated on.
+ the widget, the animation will be stopped (see :data:`stop`)
+ '''
+ props = self._widgets.get(widget, None)
+ if not props:
+ return
+ props['properties'].pop(prop, None)
+
+ # no more properties to animation ? kill the animation.
+ if not props['properties']:
+ self.stop(widget)
+
+ def cancel_property(self, widget, prop):
+ '''Even if an animation is running, remove a property. It will not be
@tito

tito Aug 11, 2012

Owner

same, missing the versionadded

@tito tito commented on the diff Aug 11, 2012

kivy/uix/carousel.py
+ if self.index == 1:
+ return None
+ if self.loop and self.index == len(self.slides)-1:
+ return self.slides[0]
+ if self.index < len(self.slides)-1:
+ return self.slides[self.index + 1]
+ next_slide = AliasProperty(_next_slide, None, bind=('slides', 'index'))
+ '''The next slide in the Carousel, is None if the current slide is
+ the last slide in the carousel. If :data:`orientation` is 'horizontal',
+ the next slide is to the right. If :data:`orientation` is 'vertical',
+ the previous slide is towards teh top.
+
+ :data:`previous_slide` is a :class:`~kivy.properties.AliasProperty`.
+ '''
+
+ #### private properties, for internal use only ###
@tito

tito Aug 11, 2012

Owner

I don't think it's pep8 compliant no? :) ("make hook" !)

@hansent

hansent Aug 11, 2012

Contributor

I did. Fixed formatting until it passed, before i could commit. it's not
passing?

Sent from my iPhone

On Aug 11, 2012, at 4:58 AM, Mathieu Virbel notifications@github.com
wrote:

In kivy/uix/carousel.py:

  •        if self.index == 1:
    
  •            return None
    
  •    if self.loop and self.index == len(self.slides)-1:
    
  •        return self.slides[0]
    
  •    if self.index < len(self.slides)-1:
    
  •        return self.slides[self.index + 1]
    
  • next_slide = AliasProperty(_next_slide, None, bind=('slides', 'index'))
  • '''The next slide in the Carousel, is None if the current slide is
  • the last slide in the carousel. If :data:orientation is 'horizontal',
  • the next slide is to the right. If :data:orientation is 'vertical',
  • the previous slide is towards teh top.
  • :data:previous_slide is a :class:~kivy.properties.AliasProperty.
  • '''
  • private properties, for internal use only

I don't think it's pep8 compliant no? :) ("make hook" !)


Reply to this email directly or view it on
GitHubhttps://github.com/kivy/kivy/pull/626/files#r1357148.

@tito

tito Aug 11, 2012

Owner

Yes, it's passing :/ Dunno why, normally, thoses are wrong:

NO: self._index%len
OK: self._index % len
NO: len(self.slides)-1
OK: len(self.slides) - 1

Missing one empty line between next_slide and the previous function.. etc.
Why it's not working O_o
@tito

tito Aug 11, 2012

Owner

Ok, pep8 updated, pull and retry :)

@rubik

rubik Aug 11, 2012

pep8 tool in kivy is very outdated, current version in PyPI is 1.3.3.

@tito tito commented on an outdated diff Aug 11, 2012

kivy/uix/carousel.py
+ self.index = self.index % len(self.slides)
+ self._insert_visible_slides()
+ self._position_visible_slides()
+
+ def on__offset(self, *args):
+ self._position_visible_slides()
+ #if reached full offset, switche index to next or prev
+ if self._offset <= self._extent * -1:
+ self.index = self.index+1
+ if self._offset >= self._extent:
+ self.index = self.index - 1
+
+ def on__drag_touches(self, *args):
+ #only when user lets go completly, start animation
+ Animation.cancel_all(self)
+ if len(self._drag_touches) == 0:
@tito

tito Aug 11, 2012

Owner

-> not self._drag_touches

@tito tito commented on an outdated diff Aug 11, 2012

kivy/uix/carousel.py
+ elif self._offset > self.min_move * self._extent:
+ if self.loop or self.index > 0:
+ new_offset = self._extent
+ else:
+ dur = self.anim_cancel_duration
+ anim = Animation(_offset=new_offset, d=dur, t='out_quad')
+ anim.start(self)
+
+ def on_touch_down(self, touch):
+ if not self.collide_point(*touch.pos):
+ return
+ if super(Carousel, self).on_touch_down(touch):
+ return True
+ else:
+ self._drag_touches.append(touch.uid)
+ touch.grab(self)
@tito

tito Aug 11, 2012

Owner

You should return True if you take the touch too... so the code can look like::

if not super(Carousel, self).on_touch_down(touch):
    self._drag_touches.append(touch.uid)
    touch.grab(self)
return True

@ghost ghost assigned tito Aug 11, 2012

Not sure about the choice of slides direction for the vertical orientation.

My opinion is that the next slide should be towards bottom, as when you scroll a page. What do you think ?

Maybe this could be another option like direction.

Owner

hansent replied Aug 11, 2012

s/previous/next
s/teh/the

@revolunet revolunet commented on an outdated diff Aug 11, 2012

kivy/uix/carousel.py
+
+ :data:`anim_cancel_duration` is a :class:`~kivy.properties.NumericProperty`
+ , default to 0.3
+ '''
+
+ loop = BooleanProperty(False)
+ '''Allow carousel to swipe infinitely. When the user reach the last page,
+ he will get the first page when trying to swipe next. Same effect when
+ trying to swipe the first page.
+
+ :data:`loop` is a :class:`~kivy.properties.BooleanProperty`,
+ default to True.
+ '''
+
+ def _get_index(self):
+ return self._index%len(self.slides)
@revolunet

revolunet Aug 11, 2012

Contributor

should add if self.slides to prevent ZeroDivisionError exception

@revolunet revolunet commented on an outdated diff Aug 11, 2012

kivy/uix/carousel.py
+ '''
+
+ loop = BooleanProperty(False)
+ '''Allow carousel to swipe infinitely. When the user reach the last page,
+ he will get the first page when trying to swipe next. Same effect when
+ trying to swipe the first page.
+
+ :data:`loop` is a :class:`~kivy.properties.BooleanProperty`,
+ default to True.
+ '''
+
+ def _get_index(self):
+ return self._index%len(self.slides)
+
+ def _set_index(self, value):
+ self._index = value%len(self.slides)
@revolunet

revolunet Aug 11, 2012

Contributor

same

hansent added some commits Aug 12, 2012

animation:
 * add missing documentation for new cancel_property and cancel_all methods
uix.carousel:
 * fix pep8 formatting and other minor code edits
Contributor

hansent commented Aug 12, 2012

fixed the things pointed out in comments after previous commit.

What do you think about prev/next when direction is 'horizontal' :

@revolunet:

Not sure about the choice of slides direction for the vertical orientation.
My opinion is that the next slide should be towards bottom, as when you scroll a page. What do you think ?

At first I thought I agreed when you said this; but after I started making the changes I disagree (and have left like before...So next is above, previous below.)

The things is, that it makes the code quite a bit more complicated, since alot of the code that can be the same for horizontal/vertical would have to be changed as it would no longer flow in the same direction as our coordinate system. When I realized about the coordinate system, I now think that next shoudl be above (because y coordiantes also increase in this direction in kivy)

Contributor

revolunet commented Aug 12, 2012

Thanks @hansent thats great.

Having different directions in the slides makes the code a bit more complicated; But what do you/user expect when you use a vertical carousel ? I think going bottom is more natural and we should be able to have this behaviour.

The clickable elements inside slides still not work, but i guess this can be another PR.

min_move -> drag_threshold ?

Contributor

hansent commented Aug 13, 2012

Well all you have to do is reverse the list and index I change it?! Not so
bad no.

Agree min_move -> drag_threshold

Sent from my iPhone

On Aug 12, 2012, at 4:34 PM, Julien Bouquillon notifications@github.com
wrote:

Thanks @hansent https://github.com/hansent thats great.

Having different directions in the slides makes the code a bit more
complicated; But what do you/user expect when you use a vertical carousel ?
I think going bottom is more natural and we should be able to have this
behaviour.

The clickable elements inside slides still not work, but i guess this can
be another PR.

min_move -> drag_threshold ?


Reply to this email directly or view it on
GitHubhttps://github.com/kivy/kivy/pull/626#issuecomment-7680082.

Contributor

revolunet commented Aug 13, 2012

Ho good idea, so just reversing the slides and setting pos = len(self.slides) - 1 is enough ?

Do you think its worth adding a 'direction' property that does this job ?

eg: direction : direction of the next slides, can be left/right or top/bottom

Owner

tito commented Aug 13, 2012

+1 direction would be better afterall

Contributor

hansent commented Aug 13, 2012

so your suggesting to remove orientation, and instead have direction property, which is a OptionProperty with options: 'up', 'down', 'left', 'right'

if so 'up' means:
next slide is below, so when you index +1, it slides 'up' to the next slide
or 'up' means:
next slide is on top (up), so when you say index +1, it slides 'down' to next slide

Contributor

hansent commented Aug 13, 2012

oh, sorry, you said left/right top/bottom

so if direction is 'top':
next slide is on top (up), so when you say index +1, it slides 'down' to next slide

Contributor

revolunet commented Aug 13, 2012

yes direction is from where you swipe to discover the next item, with default = right :)

@rubik rubik commented on the diff Aug 13, 2012

kivy/uix/carousel.py
+
+
+if __name__ == '__main__':
+ from kivy.app import App
+
+ class Example1(App):
+
+ def build(self):
+ carousel = Carousel()
+ for i in range(10):
+ src = "http://placehold.it/480x270.png&text=slide-%d&.png" % i
+ image = Factory.AsyncImage(source=src, allow_stretch=True)
+ carousel.add_widget(image)
+ return carousel
+
+ Example1().run()
@rubik

rubik Aug 13, 2012

Shouldn't examples be moved to the examples dir?

@revolunet

revolunet Aug 13, 2012

Contributor

its very handy to have a simple example at bottom of files and eventually a more complicated in the examples dir

uix.carousel:
 * remove orientation property in favor of direction property,
   which can be (right, left, top, or bottom).
Contributor

hansent commented Aug 14, 2012

like this?

Contributor

revolunet commented Aug 14, 2012

great work ! code is more verbose but this cover every use case !
the carousel is quite complete now, thanks ;)

rubik commented Aug 14, 2012

Wow this is amazing! Can it be pulled in now?

Contributor

hansent commented Aug 14, 2012

@rubik you can pull it from my uix-carousel branch: https://github.com/hansent/kivy/tree/uix-carousel

rubik commented Aug 14, 2012

How can I? Yours is on a different repository. My git knowledge is not so wide. Should I add a new remote and then checkout from there?

Owner

tshirtman commented Aug 14, 2012

Le mar. 14 août 2012 23:01:46 CEST, Michele Lacchia a écrit :

How can I? Yours is on a different repository. My git knowledge is not
so wide.


Reply to this email directly or view it on GitHub
#626 (comment).

git remote add hansent http://github.com/hansent/kivy/
git fetch hansent
git checkout hansent uix-carousel

play, test, experiment all you want :)

git checkout master # to return to your normal branch
git merge hansent uix-carousel # if you want to merge the branch in
your master

enjoy

rubik commented Aug 15, 2012

Oh great thanks! I was suspecting that!

Contributor

hansent commented Aug 17, 2012

any other issues? otherwise I will merge this later tonight.

Contributor

revolunet commented Aug 17, 2012

didnt had a chance to play with it more but no issue detected yet.
very cool addition, thanks

hansent added a commit that referenced this pull request Aug 19, 2012

Merge pull request #626 from hansent/uix-carousel
fix Issue #607: add carousel widget

@hansent hansent merged commit 6785670 into kivy:master Aug 19, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment