Split display.py into files for each backend

[Anomalocaridid]

A while ago, when I added the BuiltInDisplay backend in #846, I made an erroneous assumption about how the displays worked. I moved displayio.release_displays() to the __init__() methods of the displays that needed it, which inadvertently broke them. (Which I sincerely apologize for.)

This was later fixed in #871, at the cost of breaking BuiltInDisplay.

I have since come up with a compromise that should ensure all currently supported displays are functional: Break display.py into multiple files:

• display.py, which contains the classes for the displays that require displayio.release_displays()
• display_builtin.py, which contains BuiltInDisplay
• display_common.py, which contains the code shared by both, which is also re-exported by both of the other files for compatibility.

I tried to make all displays work while minimizing breaking changes. As far as I am aware, the only breaking change is that users of BuildInDisplay will need to import from display_builtin instead of display. I have updated Display.md accordingly.

Also, I do not own a keyboard with a SSD1306 display, so I would need someone else who has one to check that I did not break anything again.

[regicidalplutophage]

Yeah, I too were thinking of breaking up this extension, but rather than minimizing breaking changes I thought of separating every "backend" into its own file with display.py functioning as common. Though the way I envisioned it there won't be display.py, only /display/__init__.py and backend files in the /display/ directory. This should bring some degree of future-proofing in case the number of supported displays grows.

[Anomalocaridid]

That's a much better idea. I'll rework my PR to do things that way.

[Anomalocaridid]

How's this?

Now each backend is in its own file, with kmk/extensions/display/__init__.py holding common logic. I also updated README.md again.

[regicidalplutophage]

Hi! I see your latest commit and I'm trying to fact check myself on whether it's actually good form to use __init__.py as the main body of the package. @xs5871 should know better.

Also, maybe we should call each backend class within each backend module as simply backend so that we can for example:

from kmk.extensions.display import Display, TextEntry, ImageEntry, ssd1306
driver = ssd1306.backend()

Also also, as a person who brought SH1106 support, can you confirm that we need displayio.release_displays() twice in the sh1106 module? It was originally added because because SSD1306 didn't deinitialize on ctrl+c and initializing an initialized display resulted in an error. This function only needed to be called once for that purpose.

[Anomalocaridid]

Also, maybe we should call each backend class within each backend module as simply backend

Probably a good idea. I'll make sure to change it to that.

Also also, as a person who brought SH1106 support, can you confirm that we need displayio.release_displays() twice in the sh1106 module? It was originally added because because SSD1306 didn't deinitialize on ctrl+c and initializing an initialized display resulted in an error. This function only needed to be called once for that purpose.

It should only be needed once, just like the SSD1306 display. I'll make sure to fix that as well.

[Anomalocaridid]

I removed the unecessary call to displayio.release_displays() and renamed each of the display backends to just backend.

Also I probably should have brought this up earlier, but according to the CI checks, my changes cause an error that makes the tests fail. However, I am not sure how to resolve it.

[regicidalplutophage]

Yeah, unit tests aren't happy and that has to be resolved.

[xs5871]

First off: I really like what I'm seeing. Submodules are exactly the right thing to do, and should've been done for other parts of the code as well (rgb for example). What should go into __init__.py? I don't think there's a hard limit on that. Usually things that aren't meant to be imported by the user directly; there are arguments for both ends of the spectrum "as little as possible" and "everything that's not implementation specific". I don't mind sticking to the latter and split off fragments as deemed necessary.

Concerning the failing unittests: fixed in #934; nice catch -- please rebase your PR on the updated upstream. There are at least a couple of style issues I have to address, didn't look too closely at the code though yet. I expect that the linter has some complaints first.

[Anomalocaridid]

Thank you!