openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Fixing

Just a small code tweak, but otherwise this looks good!

Diff comments:

> 
> === modified file 'openlp/core/ui/listpreviewwidget.py'
> --- openlp/core/ui/listpreviewwidget.py	2016-03-05 16:41:32 +0000
> +++ openlp/core/ui/listpreviewwidget.py	2016-04-16 07:20:31 +0000
> @@ -194,11 +194,21 @@
>          """
>          Switches to the given row.
>          """
> -        if slide >= self.slide_count():
> -            slide = self.slide_count() - 1
> -        # Scroll to next item if possible.
> -        if slide + 1 < self.slide_count():
> -            self.scrollToItem(self.item(slide + 1, 0))
> +        # Retrieve setting
> +        autoscrolling = Settings().value('advanced/autoscrolling')
> +        # Check if auto-scroll disabled (None) and validate value as dict containing 'dist' and 'pos'
> +        # 'dist' represents the slide to scroll to relative to the new slide (-1 = previous, 0 = current, 1 = next)
> +        # 'pos' represents the vert position of of the slide (0 = in view, 1 = top, 2 = middle, 3 = bottom)
> +        if (not isinstance(autoscrolling, dict)) or ('dist' not in autoscrolling) or ('pos' not in autoscrolling):

You don't actually need the brackets here. Python is clever enough to figure out what you mean.
  if not isinstance(autoscrolling, dict) or 'dist' not in autoscrolling or 'pos' not in autoscrolling:
Be careful of Boolean short-circuiting and ORs.

> +            return
> +        # prevent scrolling past list bounds
> +        scroll_to_slide = slide + autoscrolling['dist']
> +        if scroll_to_slide < 0:
> +            scroll_to_slide = 0
> +        if scroll_to_slide >= self.slide_count():
> +            scroll_to_slide = self.slide_count() - 1
> +        # Scroll to item if possible.
> +        self.scrollToItem(self.item(scroll_to_slide, 0), autoscrolling['pos'])
>          self.selectRow(slide)
>  
>      def current_slide_number(self):


-- 
https://code.launchpad.net/~knightrider0xd/openlp/auto-scroll-choice/+merge/292064
Your team OpenLP Core is subscribed to branch lp:openlp.