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.

[regicidalplutophage]

I like this.
Not merging yet because I'm not sure what's up with unit tests and also I'd like to see what @xs5871 thinks, since extension being a package is something new for KMK.

[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.

[xs5871]

Suggested change

	class backend(DisplayBackend):
	class Backend(DisplayBackend):

Class names are CamelCase.
I personally don't 100% agree that all the backend implementations should have the same name. They are in seperate, appropriately named submodules though, so there's that and I'm not going to complain about it.
There could be an argument, that __init__.DisplayBackend is a slight misnomer, as it's specifically not a backend, more of a "stub" or "abstract" class, whatever your preferred nomenclature. I'd appreciate your opinion on that.

[regicidalplutophage]

My reasoning for this suggestion was to reduce tautology and make it so that one could intuit the process of setting up a backend from prior experience with another. I'm also not super into calling those backends.

[xs5871]

I get the reasoning and it's a valid point. The reason I prefer tautologies is that no matter where it turns up, be it in the import or the source itself, if you call it ssd1306.SSD1306 it's always obvious what the thing is you're talking about. It may sound derivative, but I tend to look at adafruit for conventions in general, for example: adafruit_displayio_sh1106.SH1106. That is an intentional tautology, it is verbose, but not to the point where it looks like java. (That's why I dislike the "abstract" moniker for classes. It's pseudo-technical fluff that doesn't reflect what the word is used for in day-to-day conversations.)

[Anomalocaridid]

@xs5871 Good point. I think you're right that it would be better to be explicit in this case. I'll make sure to change back the names of the backend classes.

Also I agree it is probably better to give DisplayBackend a more descriptive name. Maybe something like DisplayBackendBase?

[xs5871]

We might as well drop the "Backend" in that case and just call it DisplayBase.

[Anomalocaridid]

The display backends now have their old names back and DisplayBackend is now DisplayBase

[xs5871]

It seems my guess about the linter was wrong. Good job.

[xs5871]

Suggested change

	from kmk.extensions.display import DisplayBackend
	from . import DisplayBackend

We should be using relative imports, especially for "self contained" modules.

[Anomalocaridid]

Replaced the absolute imports with relative imports

[xs5871]

I get the reasoning and it's a valid point. The reason I prefer tautologies is that no matter where it turns up, be it in the import or the source itself, if you call it ssd1306.SSD1306 it's always obvious what the thing is you're talking about. It may sound derivative, but I tend to look at adafruit for conventions in general, for example: adafruit_displayio_sh1106.SH1106. That is an intentional tautology, it is verbose, but not to the point where it looks like java. (That's why I dislike the "abstract" moniker for classes. It's pseudo-technical fluff that doesn't reflect what the word is used for in day-to-day conversations.)

[Anomalocaridid]

Thank you!