flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
166.73k stars 27.62k forks source link

Canvas.drawImage should have sample code #78087

Open Hixie opened 3 years ago

Hixie commented 3 years ago

Canvas.drawImage should have sample code showing how to render an asset. Alternatively it could point to paintImage or some such.

mateusfccp commented 3 years ago

Hey, @Hixie. May I work on this?

What are you expecting exactly from this documentation? The bare minimum construct I got to draw an Image with Canvas.drawImage was the following:

Source ```dart class CustomAssetImage extends StatefulWidget { final String asset; const CustomAssetImage({required this.asset}); @override _CustomAssetImageState createState() => _CustomAssetImageState(); } class _CustomAssetImageState extends State { late final ImageProvider _imageProvider; late final ImageStreamListener _listener; late ImageConfiguration _imageConfiguration; ImageStream? _imageStream; ImageInfo? imageInfo; @override void initState() { super.initState(); _imageProvider = AssetImage(widget.asset); _listener = ImageStreamListener( (info, _) => setState(() => imageInfo = info), ); } @override void didChangeDependencies() { super.didChangeDependencies(); _imageConfiguration = createLocalImageConfiguration(context); final stream = _imageProvider.resolve(_imageConfiguration); if (_imageStream?.key == stream.key) { return; } _imageStream?.removeListener(_listener); _imageStream = stream; _imageStream!.addListener(_listener); } @override Widget build(BuildContext context) { if (imageInfo?.image == null) { return Text('Loading...'); } else { final image = imageInfo!.image; final painter = ImagePainter(image: image); return CustomPaint( painter: painter, size: Size( image.width.toDouble(), image.height.toDouble(), ), ); } } } class ImagePainter extends CustomPainter { final ui.Image image; const ImagePainter({ required this.image, }); @override void paint(Canvas canvas, Size size) { final paint = Paint()..isAntiAlias = true; canvas.drawImage( image, Offset.zero, paint, ); } @override bool shouldRepaint(covariant ImagePainter oldDelegate) => oldDelegate.image != image; } ```

However, this uses Flutter's ImageProvider and its related API (ImageStream, ImageStreamListener, ImageConfiguration...). Maybe you are thinking of something more direct and agnostic? What do you think?

Hixie commented 3 years ago

Something like that, yeah. Maybe also a version that shows how to do it when you know you have only one version (using ExactAssetImage and not bothering with didChangeDependencies).

mateusfccp commented 3 years ago

This makes sense. I can reduce considerably the complexity of the sample by using ExactAssetImage, as it uses 1.0 as the default scale, and thus we don't have to worry about getting an ImageConfiguration from context. Instead, we can use ImageConfiguration.empty and delegate scale, bundle and package to ExactAssetImage constructor...

I am going to make a proposal PR. Where do you think it's the best place to document it? Surely, it won't make sense in Canvas.drawImage itself, as it isn't a Flutter class. Maybe in the CustomPainter documentation?

Hixie commented 3 years ago

Canvas.drawImage is a Flutter class, that's a fine place to document it. (That API is implemented in the flutter/engine repo.)

mateusfccp commented 3 years ago

Oh, I didn't know dart:ui was part of Flutter, sorry.

mateusfccp commented 3 years ago

@Hixie I opened a PR with an initial proposal on flutter/engine (https://github.com/flutter/engine/pull/25328).

Also, is there a way to compile the documentation locally? dartdoc won't work because the ui folder has not a pubspec.yaml file, so I had to make some workarounds to see the result.

flar commented 3 years ago

While dart:ui is part of Flutter, it is a dependent component. Examples of using dart:ui should not require Stateful or any kind of Widget.

If the problem is that there is no mechanism within dart:ui to load images, then that should perhaps be a feature request.

If the issue is how to tie Flutter's asset management system in to the Canvas APIs, then I would think that documentation should go on the widget that Flutter uses to interact with the low level rendering APIs and that would be CustomPaint and CustomPainter.

Hixie commented 3 years ago

It's perfectly fine for lower layers to show sample code for higher layers, we do it all the time. It would be good to also have sample code that shows how to use the layer without higher layers, though.

flar commented 3 years ago

It's a huge chunk of code, though, that makes it look like this API is hard to use, but it isn't.

Perhaps putting the chunk of code into something like the ImageAsset class and then pointing to that class from the drawImage method saying "If you need to render an image from a Flutter asset, then the [ImageAsset] class has examples of how to extract the base [Image] object from it"?

flar commented 3 years ago

I guess what I see here is that we have a very simple rendering call here that takes an image. Some places you might get the image from are simple and straightforward, but others are complicated. It is up to the complicated mechanisms to explain how to use them to get a simple image from them, not up to the rendering call to explain how to deal with other mechanisms.

Hixie commented 3 years ago

It's a huge chunk of code, though, that makes it look like this API is hard to use, but it isn't.

Let's add some other examples that are simpler.

Hixie commented 3 years ago

(The problem we need to solve is people trying to draw their asset images on canvases correctly going to Canvas.drawImage, then not knowing what steps to take next. We can push them to other parts of the API docs, but that just adds more steps to their journey. At the end of the day, we're one project, one set of APIs, and the developers don't care whether it makes drawImage look complicated or not, they just want to draw their asset.)

flar commented 3 years ago

I think directing them to where to find the information - on the Asset classes - will solve the problem of ending up at drawImage with that question. If we included all possible examples of how to manage all possible sources of images in the drawImage call itself, the multiple examples would drown out key information like what Paint properties affect the image and how they interact with the sampling of the image.

drawImage should focus on its role and if there is some information someone might need as a pre-requisite then point them at that information rather than copy it into the low level call.

Hixie commented 3 years ago

If we included all possible examples of how to manage all possible sources of images in the drawImage call itself, the multiple examples would drown out key information like what Paint properties affect the image and how they interact with the sampling of the image.

I agree, but nobody is proposing that. Right now the docs are literally twenty seven words long. We could add many examples before we approached the issue of anything drowning out anything else.

I do agree that we should add an example that shows how to use Canvas.drawImage without using the higher level primitives, but that seems orthogonal to the proposal here. Similarly, it would be great to have documentation on what Paint properties affect the image and so on, but that's a separate issue and it is in no way prevented by adding some sample code.

For context, see e.g. the docs for https://master-api.flutter.dev/flutter/rendering/RenderBox-class.html or https://master-api.flutter.dev/flutter/widgets/StatefulWidget-class.html or https://master-api.flutter.dev/flutter/material/MaterialApp-class.html.

flar commented 3 years ago

The docs for drawImage should be relatively simple as it has a very narrow scope - it takes an existing ui.Image object and places the pixels on the screen subject to 3 or 4 Paint attributes and a transform.

There are several ways to get an image to use with this method and all of them have their own idiosyncrasies that are outside its scope.

You can use ImageDescriptor/Codec/FrameInfo.image You can use PictureRecorder/Picture.toImage() You can use RenderRepaintBoundary.toImage() You can use AssetImage There are/will be external modules for image codecs and manipulation.

This method is not complex enough to host the sample code to deal with all of the intricacies of any, each, or all of those mechanisms. At best, it might mention a few of them that are local to the flutter project with pointers and if the various mechanisms require understanding the complexities of the way that widget state is managed, or how to look up RenderObject instances, then those examples should be documented in the context that introduces them. drawImage() is not responsible for the framework state management or the needs of a Codec or how widgets relate to RenderObjects.

I'd expect the RenderBox documentation to be lengthy as it is the cornerstone for much of the RenderObject infrastructure of the Flutter framework. I'd expect StatefulWidget to have lots of documentation and a tutorial because it is also the bread and butter of maintaining the state of most apps. And all of these components are somewhat unique within the industry. They are a relatively new take on how to manage app design and state.

Canvas.drawImage() could probably get away with "If you have ever done graphics before in any language since before the turn of this millennium you could probably write this doc comment yourself". That isn't to say that we shouldn't be explicit about how it interacts with the attributes and that we haven't been lax in that level of completeness so far, but that it isn't really surprising in how it works and the resulting docs would be fairly concise. The only novel thing about the example being proposed here is due to the design of asset management in the Flutter framework which is well outside the scope of a graphics draw op.

I see no reason to burden the method with descriptions of how to manage the various sources from which you can obtain the image data. At most it might be worth an @see pointer to some of the more common sources each of which can decide how much documentation and examples are needed to understand those mechanisms on a case by case basis within their own contexts.

But an example of how to locate the administration of an AssetImage within the scope of a Flutter application framework so as to produce a concrete Image object from it - should be in the AssetImage documentation - not in Canvas.drawImage().

Hixie commented 3 years ago

The docs for drawImage should be relatively simple as it has a very narrow scope

The docs should be simple, certainly, but they do not need to be brief. They can, and should, be as detailed as possible to get people productive as fast as possible.

There are several ways to get an image to use with this method and all of them have their own idiosyncrasies that are outside its scope.

You can use ImageDescriptor/Codec/FrameInfo.image You can use PictureRecorder/Picture.toImage() You can use RenderRepaintBoundary.toImage() You can use AssetImage There are/will be external modules for image codecs and manipulation.

I would be perfectly happy for us to provide sample code for each of these in these docs.

This method is not complex enough to host the sample code to deal with all of the intricacies of any, each, or all of those mechanisms.

I don't understand why a method must be complicated for it to have detailed documentation or for it to host lots of sample code.

The point of API docs is to help developers write their applications. If we could somehow create API docs such that the first API doc someone looks at contained the entirety of the code they needed to write their application, we should do so, because that would be the quickest way to solve their problem.

Canvas.drawImage() could probably get away with "If you have ever done graphics before in any language since before the turn of this millennium you could probably write this doc comment yourself".

We have 5 year olds reading this documentation. We have 65 year olds who just started programming yesterday. We have developers who have spent years working on backend server code and just decided today to try their hands at UI. We must welcome everyone, regardless of skill level. They might be graphics experts looking to know exactly how drawImage does interpolation, or they might be inexperienced hobbyists who just want to copy some code to get their Christmas card working. It's not our place to judge why they are coming here, nor is it our place to dismiss people who couldn't write the API docs.

There's a section in our style guide that talks about this. It makes the point that if someone is reading the API docs, it's for a reason. Clearly, if they could write it themselves, they would not be reading the API docs. So we should never consider it even remotely reasonable to think that our API docs could tell someone they don't need the API docs. Frankly, it is a highly condescending perspective to take.

I see no reason to burden the method with descriptions of how to manage the various sources from which you can obtain the image data.

It's not a burden. It's an honour.

But an example of how to locate the administration of an AssetImage within the scope of a Flutter application framework so as to produce a concrete Image object from it - should be in the AssetImage documentation - not in Canvas.drawImage().

I have no objection to it being in the AssetImage documentation, but that's not where people will be looking when they want to draw an image from their assets. They will likely start with "I'm in a paint method, it has a Canvas. What can Canvas do? Ah, it can drawImage. Let's see how that helps".

flar commented 3 years ago

I have no objection to it being in the AssetImage documentation, but that's not where people will be looking when they want to draw an image from their assets. They will likely start with "I'm in a paint method, it has a Canvas. What can Canvas do? Ah, it can drawImage. Let's see how that helps".

Put it in AssetImage. Point to it from drawImage.

Hixie commented 3 years ago

We can certainly do that too, but it seems weird to do so? Why make people take the extra step?

I really don't understand the objection to having useful documentation here.

flar commented 3 years ago

I really don't understand your desire to put it in drawImage. This example is how to deal with AssetImage, it is not how to deal with drawImage. I don't know how to put it more simply than that.

flar commented 3 years ago

You can use ImageDescriptor/Codec/FrameInfo.image You can use PictureRecorder/Picture.toImage() You can use RenderRepaintBoundary.toImage() You can use AssetImage There are/will be external modules for image codecs and manipulation.

I would be perfectly happy for us to provide sample code for each of these in these docs.

I would be happy to have those documented as well - in those classes, not here.

Hixie commented 3 years ago

My desire to put all of these in drawImage is that people go to drawImage to look up documentation on how to draw images.

flar commented 3 years ago

If they get to drawImage they can go there for any number of questions. If they see a dozen pieces of sample code:

etc.

How does that help the person with a specific question. They'll have to scan and scan and scan and it will all look way more complicated than it really is when they only have one question.

Instead, point them at the various pieces which come into play and then have those pieces document themselves. You can point out that there are several ways to get an image in one sentence and then they can click over to the specific one that they want for further details.

AssetImage is only 1 of 4 or 5 places they can get an image. It's not the one question they'll have when they get here.

flar commented 3 years ago

Pointers good. Entire examples, given that there are potentially a dozen completely separate questions they may have - bad.

Hixie commented 3 years ago

I'm perfectly happy for us to move the sample code elsewhere once we have other material competing with it, but again, right now there's literally only 27 words in these docs...

Hixie commented 3 years ago

(as opposed to AssetImage, which has lots and lots of documentation already -- so if we're worried about overwhelming people, the argument applies way more over there than here today.)