Load Image from bytes

[bruno-rino]

Currently Image loads files by file path or URL. This PR adds a bytes object as an option.
Refs #1348

I'd like feedback on whether overloading the positional argument for the Image constructor is well received. Its name, path, makes no sense anymore. I changed it to value.

Implemented on all backends, but only tested on macOS.

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

[freakboy3742]

General direction looks good; one comment inline about the change to the API for Image().

One other suggestion relates to the example. The example test case works to demonstrate the implemented behavior, but isn't a very realistic "real world" case.

I'd like to see an example that is closer to "reality" - e.g., generating a new image with Pillow, storing it as bytes, then showing that image. In addition to proving that the "real world" use case has been satisfied, it serves as useful documentation-by-example to anyone wondering how to use the API.

[freakboy3742]

I can see what you're doing here, but the error handling on this is incomplete. It allows specifying both value and path, and doesn't catch the Image() case (at least, not before crashing later).

I'd also suggest changing the function prototype to:

def __init__(self, value=None, *, path=None, data=None):

If you specify value, you get full "autodetection" behavior; if you specify path, you get the existing "path or url" conversion; and if you specify data, it's assumed to be in the right format. Specifying more than one argument would be an error. This doesn't have to be a deprecation path, either - it doesn't seem unreasonable as an ongoing API.

[bruno-rino]

I'd never encountered that lonely star in the args before. Powerful stuff.

If your preference is to preserve separate path and data arguments, I personally would prefer this function prototype:

def __init__(self, path=None, data=None):

Simpler, more obvious. One way of doing things. More Zen.

But you're the BDFL. Let me know.

[freakboy3742]

That's a good point - I got so focussed on the change introducing value that I missed the obvious option. Let's go with (path=None, data=None).

FYI - there's an analogous / that matches * in args. * forces all subsequent arguments to be kwargs; / forces all prior arguments to be positional. The full form of a function is effectively:

def name(positional_only_parameters, /, positional_or_keyword_parameters, *, keyword_only_parameters):

See PEP570 (and PEP457) for details.

[mhsmith]

In that case, data should probably be a keyword-only argument, while path can continue to be either keyword or positional, for backward compatibility. In other words:

def __init__(self, path=None, *, data=None):

[codecov]

Codecov Report

Merging #1588 (f9ec63c) into main (dfc7273) will decrease coverage by 9.76%.
The diff coverage is n/a.

Impacted Files	Coverage Δ
src/core/toga/handlers.py	40.00% <0.00%> (-6.67%)	⬇️
src/core/toga/platform.py	17.64% <0.00%> (-5.89%)	⬇️
src/core/toga/style/applicator.py	84.21% <0.00%> (-5.27%)	⬇️
src/core/toga/style/pack.py	87.08% <0.00%> (-2.50%)	⬇️
src/core/tests/test_image.py
src/core/toga/images.py
src/dummy/toga_dummy/images.py
src/core/tests/widgets/test_base.py
src/core/tests/test_documents.py
src/core/tests/sources/test_tree_source.py
... and 114 more

[freakboy3742]

I've made a couple of small tweaks to the example, but otherwise: This is great - thanks!