Initial release #1

kattni · 2018-10-08T16:20:28Z

No description provided.

kattni · 2018-10-08T16:28:37Z

Tested on Hollowing with invalid file present on board.

caternuson · 2018-10-08T16:35:34Z

adafruit_slideshow.py

+        self._file_list = self._get_filenames()
+        if self._order == SlideShow.RANDOM:
+            self._file_list = sorted(self._file_list, key=lambda x: random.random())
+            self._images = self._get_next_image()


Unindent. Shouldn't be in the if. Looks like that was my bad. I only tested random. :(

I was testing it with alphabetical and it seemed to be working so I didn't think anything of it. On closer investigation it was only mostly working. Fixed.

sommersoft

Couple small things.

With regards to type-checking for arguments, there are a couple more spots you could check (dwell is int), but I think they'll fail with standard exceptions anyway.

sommersoft · 2018-10-08T16:56:35Z

adafruit_slideshow.py

+    FADE_IN = 0
+    SHOW_IMG = 1
+    FADE_OUT = 2
+    LOAD_IMG = 3


These are all internal variables. Should start with _? (_FADE_IN)

Also, now that I read further down...these should also move outside of the SlideShow() class. The variables themselves don't change, the self._current_state does, so you don't need to recreate them for every instance of the class. Calls to self.FADE_IN etc will also need to change after that.

sommersoft · 2018-10-08T16:56:48Z

adafruit_slideshow.py

+        return self._order
+
+    @order.setter
+    def order(self, order):


If there are only 2 options, I would check that the supplied argument meets one of them:

def order(self, order): if not order in [ALPHA, RANDOM]: raise ValueError("Order must be either 'RANDOM' or 'ALPHA'")

This should also be checked in __init__.

__init__ can do self.order = order to call the setter instead of setting _order directly.

So every instance of _order gets changed to order then?

Added ability to choose whether slideshow will automatically cycle through the images or whether you would like to specify an input to manually cycle through the slideshow. Added backlight brightness level control. Added ability to cycle forwards or backwards through image list. Added ability to turn off the fade in/out effects during image transition - this means the redraw effect of the image changing is visible, but it also means you can advance more quickly through the list if you choose to. Documentation updated. Various linting fixes.

jerryneedell · 2018-10-09T20:52:00Z

I found a minor bug in the slideshow_simpletest.py:

change
slideshow.order = slideshow.ALPHA
to
slideshow.order = adafruit_slideshow.PlayBackMode.ALPHA

kattni · 2018-10-09T20:52:59Z

@jerryneedell I already found that and updated it. Thanks for catching it though! Travis failed on it for me.

caternuson

Found a few minor things.

README.rst

caternuson · 2018-10-09T21:12:13Z

adafruit_slideshow.py

+        while slideshow.update():
+            pass
+
+    Example code for Hollowing Express. Sets ``dwell`` to 0 seconds, turns ``auto_advance`` off,


s/Hollowing/Hallowing

:face_palm: Thank you for catching this.

caternuson · 2018-10-09T21:12:24Z

adafruit_slideshow.py

+
+**Hardware:**
+
+ * `Adafruit Hollowing M0 Express <https://www.adafruit.com/product/3900>`_


s/Hollowing/Hallowing

caternuson · 2018-10-09T21:12:31Z

adafruit_slideshow.py

+                 if slideshow should auto play, ``False`` if you want to control advancement
+                 manually.  Default is ``True``.
+
+    Example code for Hollowing Express. With this example, the slideshow will play through once


s/Hollowing/Hallowing

caternuson · 2018-10-09T21:12:53Z

docs/index.rst

+.. toctree::
+    :caption: Related Products
+
+    Adafruit Hollowing M0 Express <https://www.adafruit.com/product/3900>


s/Hollowing/Hallowing

tannewt

These internals are more complicated than they need to be. In particular:

There aren't really 5 states, there are two: waiting or advancing. That way you only need to track the time.
You don't need a generator. Instead, just save the current index on self.

I've gone through and reworked this PR and PRed to it (so meta!) here: kattni#1 The PR also includes fixes for these inline comments.

Overall the API looked really good! I only changed how brightness works. Thanks for getting it this far! All the knobs are nice!

tannewt · 2018-10-09T21:34:10Z

adafruit_slideshow.py

+
+    # pylint: disable=too-many-arguments
+    def __init__(self, folder="/", order=PlayBackMode.RANDOM, loop=True, dwell=3, fade_effect=True,
+                 auto_advance=True):


How about adding *, after self to force these as kwargs? Otherwise things can get confusing with the multiple boolean options.

tannewt · 2018-10-09T21:35:52Z

adafruit_slideshow.py

+        if self.order == PlayBackMode.RANDOM:
+            self._file_list = sorted(self._file_list, key=lambda x: random.random())
+
+    def backlight_level_up(self, step=16):


Why not have a max_brightness property instead? That way you can just += to change it.

tannewt · 2018-10-09T21:36:43Z

adafruit_slideshow.py

+            except ValueError as error:
+                print("Incompatible image:", imagename, str(error))
+
+        return True


Why return anything?

tannewt · 2018-10-09T21:37:43Z

adafruit_slideshow.py

+                self._show_bmp(imagename)
+                self._current_state = _FADE_IN
+            except ValueError as error:
+                print("Incompatible image:", imagename, str(error))


Its weird to print from within a library. I'd suggest either now catching the exception or silently passing.

tannewt · 2018-10-09T21:40:48Z

adafruit_slideshow.py

+            odb = displayio.OnDiskBitmap(image)
+            face = displayio.Sprite(odb, pixel_shader=displayio.ColorConverter(), position=(0, 0))
+            self._group.append(face)
+            board.DISPLAY.wait_for_frame()


This will close the image file too early. It should be open until the sprite is popped from the group. We don't check this currently but it should check and throw an error.

tannewt · 2018-10-09T21:42:42Z

examples/slideshow_simpletest.py

+slideshow.loop = False
+
+# Set the order to alphabetical.
+slideshow.order = PlayBackMode.ALPHA


Why don't the examples use the constructor arguments instead? I'd only expect the properties to be used when changing from within the loop.

tannewt · 2018-10-09T21:56:20Z

adafruit_slideshow.py

+            time.sleep(0.01)
+
+    def update(self):
+        """Updates the slideshow to the next image."""


I would change this comment because it won't always go to the next image.

tannewt · 2018-10-10T00:09:26Z

The extra states jumped out at me because each if for the current state wasn't exclusive (elif). That meant that in practice you'd transition through multiple states (_FADE_OUT always goes to _LOAD_IMAGE for example) in a single call each time, which is a good sign you don't need them separate.

Simplify

tannewt

Looks good! Thanks for taking my changes

outdated

kattni · 2018-10-11T01:08:42Z

In commit caee24a, added the ability to call the name of the current image being displayed with slideshow.current_image_name. Apparently using markdown in a commit message caused it to not include the rest of the commit message.

merge from adafruit

kattni added 4 commits October 8, 2018 12:04

Initial commit.

430a05b

Removed unnecessary requirements

dc50091

Added space in code-block

a23a82e

first go at making Sphinx happy

cce8983

caternuson requested changes Oct 8, 2018

View reviewed changes

sommersoft previously requested changes Oct 8, 2018

View reviewed changes

kattni added 3 commits October 9, 2018 16:29

Added touch input example

346697b

Linting and update to match API change

229df34

caternuson requested changes Oct 9, 2018

View reviewed changes

kattni added 2 commits October 9, 2018 17:23

Fixed typo and updated example.

add0c48

Updated the REST of the example...

80faeaf

caternuson approved these changes Oct 9, 2018

View reviewed changes

tannewt added 2 commits October 9, 2018 16:56

Simplify the internals and brightness.

00ef90f

Remove builtin display requirement

20ccb32

tannewt mentioned this pull request Oct 9, 2018

Simplify kattni/Adafruit_CircuitPython_Slideshow#1

Merged

tannewt requested changes Oct 10, 2018

View reviewed changes

tannewt and others added 4 commits October 10, 2018 14:16

Remove print

084a9d5

Merge pull request #1 from tannewt/simplify

ee53f4a

Simplify

Linting. Added .

caee24a

Moar linting.

83bef67

tannewt approved these changes Oct 11, 2018

View reviewed changes

tannewt merged commit 0ac5f42 into adafruit:master Oct 11, 2018

kattni pushed a commit that referenced this pull request Nov 23, 2020

Merge pull request #1 from adafruit/master

4f8eba6

merge from adafruit


		Hardware:

		* `Adafruit Hollowing M0 Express <https://www.adafruit.com/product/3900>`_

Initial release #1

Initial release #1

Uh oh!

Conversation

kattni commented Oct 8, 2018

Uh oh!

kattni commented Oct 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sommersoft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannewt Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jerryneedell commented Oct 9, 2018

Uh oh!

kattni commented Oct 9, 2018

Uh oh!

caternuson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannewt commented Oct 10, 2018

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

kattni commented Oct 11, 2018

Uh oh!

Uh oh!

tannewt Oct 8, 2018 •

edited

Loading