Support for custom image format plugins

[HalfWhitt]

Refs #2142, Refs #2231

Decided to take a break from learning Objective C and tackle something in just Python for a minute : )

As described in #2142, it would be nice to have an extensible plugin mechanism for telling Toga about external image types, and include PIL support as a Toga-provided plugin. My previous PR #2231 was the first step, adding "hard-coded" PIL support; this converts that to an included plugin, with support for further plugins defined by BeeWare and/or others.

The mechanism / API is pretty simple: a plugin registers its entry point and provides a module defining what image class it's for, a function to convert data into that class (or a subclass of it), and a function to convert that class into data. Toga's Image class then builds itself a class-level dict, with the keys being classes and the values being the converters. If an image's class is recognized as a key, that converter is used. If it's not, Toga steps through the known converters to see if it's a subclass of any of them; if it is, it not only uses that converter but also saves it as a new key so it doesn't have to be searched for. (This might be premature optimization, but it's also not much more complicated than a list that's always searched.)

Everything appears to work, but please let me know if anything should be done a better way, particularly the various dependency methods to make sure the plugin's always installed. I added the new toga-pil package to the how-to editable install command, tox's install command, the testbed's dependencies, and the dependencies for each backend. I also set it to keep its version number in sync for dynamic dependency, the same as the backends.

Also for now I put the call to Image._load_converters() in App.__init__, which I'm not sure is the best place for it; for one thing, it means you can't convert to/from plugin-defined formats before creating an App instance. (Then again, I don't know if you'd ever need to, and you already can't create one from a file path without an App created. You can from data, though, which feels like an odd distinction from an end-user perspective.) I initially tried calling it at the top level of core/images, but that causes a circular import because of the plugin's dynamic version declaration. Is there a better "at the beginning, but once imports are resolved" spot?

P.S. While I was mucking about, I removed an unnecessary app fixture from several image tests, and fixed the error type of an Image created with the wrong number of arguments from ValueError to TypeError.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[HalfWhitt]

Okay I take it back, I guess I don't have the dependencies working correctly. Core passes locally but obviously the PIL plugin isn't getting installed in CI.

[HalfWhitt]

Well that was an adventure. @_@

[freakboy3742]

I haven't done a full teardown review yet - but the general approach of loading plugins looks about right. However, the reason I haven't done a full review is that I'm not convinced the broader structure of pulling this code out into a toga-pil package is quite right.

The first problem is that all the individual backends now have toga-pil as a dependency. We've gained a new package (and all the packaging complexity that goes along with that) - but the backends don't have any dependency on toga-PIL - the functionality is all in the core. Based on this setup, then toga-PIL should be a dependency of toga-core.

The second problem is that PIL isn't actually a dependency of toga-PIL. The code still needs the logic that checks whether PIL is installed at all. Ultimately, this means we're carrying the overhead of a pluggable backend, but not actually gaining the benefit of the backend being pluggable.

So - we're sort of straddling two worlds here. We've got an "optional" plugin... but it's a hard-coded dependency, so it's not actually optional. We've got a separate plugin package... but there's no way to not install the package. So why does it exist as a separate package at all?

I can see three ways to resolve this:

1. We don't make toga-PIL a standalone package, but include the code as part of toga-core. Installing toga-core would register the toga-PIL entry point - so the PIL behavior is still loaded as a plugin - but the plugin is always present when you install toga-core. This is a bit like Briefcase's backends - when you pip install briefcase, all the backends are technically plugins - but they're always installed by default. This still requires the "optional PIL import" handling; maybe the loader can check if image_class is None before finalising the registration.
2. We make toga-PIL a fully fledged standalone plugin, that does have a hard dependency on PIL; but we don't include it as a dependency of anything in toga's core. It might remain a dependency of testbed, but only as a mechanism for actually testing. We remove the optional import handling, and make PIL a hard dependency of toga-PIL.
3. Do (2), but make the toga-pil package a completely standalone repository (since it is, essentially a plugin, and can be version controlled as such).

(2) is probably the cleanest from a pure architecture point of view, because it's an explicit opt-in and doesn't require any special "is PIL installed?" logic.

However, technically, it would be a removal of an existing feature. That's not a 100% dealbreaker, but it's worth keeping in mind. It's also inescapable that PIL is, for all intents and purposes, the image library for Python; so it's a bit inconvenient to have to manually declare a dependency on toga-pil so you can use the image library that pretty much all Python users will expect to be able to use out of the box.

(3) is probably overkill. It might let us define a single image handling plugin, with community contributions for any other image types installed as optional extras (i.e., something like pip install toga-image-plugin[pil,opencv]... but if we expect everyone to contribute to Toga's official plugins, that kind of defeats the purpose of having plugins. It also makes the testing and release process harder, because you've got 2 repos to coordinate. This is already hard enough with Travertino and Toga... :-)

My inclination is that (1) is probably the right approach. It's architecturally simpler; and it preserves the expected feature of PIL "just working" if it's installed.

I'm not completely set of this though - if you've got strong feelings to the contrary, I'm happy to have the discussion.

Regardless of the approach - we should also include a "dummy" image plugin as part of toga-dummy, as a way of checking that arbitrary plugin loading works.

One other note about the redundant "app" fixture - keep in mind that a fixture that isn't used isn't necessarily redundant. Fixtures can be there to set up runtime conditions. The fact that the tests are passing suggests that might not be the case here - but if memory serves, the reason the app fixture was there before was to ensure that the app paths were established. Try running one of the "redundant app" tests by itself, and see if the test still passes - it might not, because the singleton app hasn't been instantiated.

[HalfWhitt]

...You know, I honestly hadn't thought of that. I was stuck thinking of a one-to-one correspondence between entry points and packages. I guess my only reservation against (1) is that having an actual PIL plugin package makes for a very clear, mostly copyable example, but that's probably moot with sufficient documentation. I'll go ahead with reintegrating it into core.

I included a "dummy" image converter in the core tests as a fixture, but I did it by inserting it directly into the registry, not discovering through an entry point. So yeah, including one in the dummy backend sounds like a much more robust test, I'll do that.

As for the app fixtures, boy do I feel silly. Not only did I put some of those there, I think, I also mentioned in the text of this pull request how you can't create a Toga image from a file path without an app being created. I forgot how fixtures can have persistent side effects, so running the successful suite doesn't mean any given test will pass on its own.

[HalfWhitt]

Oh, now that it's no longer a separate package with a dynamic version number that can cause a circular import, I could load plugins at the top level of core/images instead of in App's init. Won't be back at a computer till tomorrow, though.

[freakboy3742]

This definitely looks a lot better. A couple of mostly cosmetic clarifications inline.

[HalfWhitt]

What I said about premature optimization kept nagging in the back of my head... I did some rough benchmarking and found that there was really no noticeable speed difference between a dictionary lookup of known subclasses, a list of unique converters, or a combination of both (one for checking and one for searching). Even when there are way more plugins and subclasses than is reasonable! So I simplified it to a plain ol' list.

[freakboy3742]

This is starting to look really good. A couple of documentation cleanups inline; and two final architectural tweaks that I want to suggest:

Firstly - should we make this a class-based, rather than a module-based plugin?

1. It would mean you'd be able to define multiple plugins in a single module, rather than requiring one module per plugin.
2. We'd be able to publish a formal typing Protocol for the plugin, rather than relying on a semi-informal module definition
3. Since it would be a Protocol, the documentation for the protocol would be in a familiar format

The registration process is very slightly different (pil = "toga.plugins.images:PILPlugin", but it should be otherwise almost identical in implementation. Thoughts?

The second suggestion: do we actually have a use case for supporting subclassing? The "must have a compatible signature" clause seems like it could be a footgun waiting to go off. If you've got an image subclass that has an incompatible signature, it's still going to be picked up as a subclass and thereby trigger processing by the plugin - and there won't be any way to opt out of that.

I wonder if it might be preferable to limit ourselves to "declared class only", and check for a literal class rather than subclass; and require multiple plugin registrations if there's an example of subclassed images. Alternatively, the plugin could declare a list/set of "compatible" classes - but, unless we've actually got a use case in mind, maybe it's better to leave the feature off entirely and cross that bridge if/when we get to it.

[HalfWhitt]

Subclasses are a tricky question. When I was first implementing as_image and was worried about signature mismatches with subclasses of Image, you described it as a "consenting adults" situation; if a user subclasses Image and then tells an Image to convert itself to that, it's their responsibility to make sure it has a compatible initializer. Wouldn't that apply here as well?

If someone subclasses their own custom class and gives it a different signature they want Toga to know about, they can register that subclass too... though I guess they'd need to order them from most to least specific in order for them to be resolved correctly.

It is kind of a can of worms, I agree. But not supporting subclasses for custom classes feels inconsistent with how we treat Toga images. If we're going to continue supporting toga.Image subclasses, I think I'd favor a "subclasses are assumed compatible by default, unless specified otherwise" approach for custom types. I feel like an explanatory note in the docs is a good idea regardless of which way we go — this very conversation indicates it's not immediately obvious what will or should happen!

Agreed on using a protocol class instead of the module itself though, I like that idea.

[freakboy3742]

Subclasses are a tricky question. When I was first implementing as_image and was worried about signature mismatches with subclasses of Image, you described it as a "consenting adults" situation; if a user subclasses Image and then tells an Image to convert itself to that, it's their responsibility to make sure it has a compatible initializer. Wouldn't that apply here as well?

I think I was overthinking things.

There's two cases to consider here:

1. A user defining a subclass of toga.Image.
2. A third-party library that has a base Image class and subclasses of that class.

In the first case case, Toga has an expected contract, so it's up to the developer to maintain compatibility with the base toga.Image constructor. "Consenting adult" rules definitely apply here.

In the second case, we have no control over the constructor of any image subclasses. However, the Toga plugin can act as an intermediary to mitigate any constructor differences that might exist on a class-by-class basis.

I was convolving the two - a constructor for a third party image subclass that we don't have any control over, being used without a plugin as an intermediary. This situation doesn't actually exist. Third party libraries will always have a plugin to adapt to any differences in constructor; and in the case where there's an incompatible subclass, the plugin will be in a position to identify that the request can't be satisfied and raise an error. My concern was unfounded.

That said - it would definitely be worth documenting the contract for toga.Image subclasses so that it's explicit.

[HalfWhitt]

I didn't accept any of your proposed commits since the change from module to class meant I largely rewrote the docs page, but I used them as the basis for the phrasing. I also pulled in main, so I could update the Screen.as_format docstring.

[freakboy3742]

This looks great - nice work.

I've picked up some naming consistency issues (image_format, image_plugin, image_converter) in my final review; it was easier for me to just do the rename myself rather than mark up all the changes, so I've pushed an update that normalises on image_formats.

I've also done a couple of edits to the docs - most notably, clarifying that it's only constructors of toga.Image subclasses that have the "compatible constructor" requirement, and even then, only if you want to use them in as_format() calls.