[Review request] Image loader API

[Lubomir Nerad]

Hi Richard,

first I will try to list some ImageLoader use cases, so there is no 
confusion where the proposed API is:

1) synchronous image loading, scenegraph constructed on FX thread,



On 1/19/2013 2:15 AM, Richard Bair wrote:
> Hi Lubo,
>
>> To move the discussion forward I'll try to summarize my arguments against the Image.getLoadWorker solution:
>>
>> * Doesn't address the request to directly throw exceptions for synchronously loaded images (http://javafx-jira.kenai.com/browse/RT-17645). This is addressed by the proposed ImageLoader.load methods.
> I think that is beside the point -- we can have synchronous methods (static, whatever) that throw an exception or whatnot. However in the case that we have an asynchronous load, the question is, why aren't we reusing the existing API for this (Worker) that we use most everywhere else for asynchronous tasks? This is the crux of the argument (for me). We have an API which we want to use consistently so that anywhere anybody needs to do an asynchronous task, they have a consistent API. A developer could for example develop a progress dialog that just takes any Worker. You feed it a Worker and it knows what to do with it. If we create new API for Image different from Worker, then we have a situation where a special case is being created. But is it worth it?
>
> Note that whether you use event handlers or Worker, in either case you could have an ImageLoader or not. The question of ImageLoader or just putting methods on Image seem mostly (to me) to be a question of how to ask for an Image to be created.
>
> Another option is for the ImageLoader to be a Service. It could have a queue of load requests, and the progress, etc properties are based on the cumulative load of all Images. So for example:
>
> ImageLoaderService loader = new ImageLoaderService();
> Image image1 = loader.loadAsync("image1.png");
> Image image2 = loader.loadAsync("image2.png");
>
> In this case, each loadAsync call ends up putting the request on a concurrent queue. The first request to show up fires off a task which runs until all queued images are loaded, updating progress etc as appropriate. The individual progress, error state, etc of the Image could be reported on the Image as it is today.
>
> Maybe a more detailed patch or explanation of the API you were envisioning might help? When I see:
>
>> loading started (setOnLoadingStarted)
>> loading progress (setOnLoadingProgress)
>> loading finished
>>     loading succeeded (setOnLoadingSucceeded)
>>     loading failed (setOnLoadingFailed)
> I recognize it as being virtually the same as what Worker already does, so the natural question is why are we doing a separate (but similar) API? Maybe it would help me to be able to look at the actual patch?
>
>> * Makes it harder to define uniform error handling for multiple background loaded images (requires separate property change listener registration for each loaded image). ImageLoader allows to register a single onLoadingFailed handler for all its loaded images.
> I don't think any of the messages or issues have enough context for me to know what the problem here is. I'm thinking you call "load" once for each image and each image has a different load worker. Are you thinking there is a load which reports for all images? Where did the setOnLoadingStarted etc methods live, on the Image or the ImageLoader? Is there just a single ImageLoader in the world or as many as you create? Are the images loaded serially or in parallel (subject to some threshold)?
>
>> * Adds duplicate information/functionality to the Image class:
>>
>> Image.progress vs Image.loadWorker.progress
>> Image.cancel vs Image.loadWorker.cancel
>> Image.error vs Image.loadWorker.state
> Well, these can just bind to the loadWorker I guess, so they become convenience properties, essentially.
>
> So if an Image is created via an ImageLoader, you still have duplicate information/functionality -- one comes via the properties, one via the events?
>
>> * Adds unwanted overhead to images which are constructed directly from application provided pixel data.
> I guess I don't understand. It seems like in such cases the LoadWorker could be a singleton instance that is always in the "succeeded" state.
>
>> * I don't think it is easier to use from FXML. Maybe if the information provided by the Worker interface needs to be displayed, but definitely not if some additional code needs to be executed in response to failed image loading. As to ImageLoader, I already mentioned the possibilities to add additional Image constructors with ImageLoader parameter or to add an imageLoader field to the FXMLLoader class. Even if we don't do that, I found out that FXML uses its own internal builder for Image instances. It will be easy to add support for an optional imageLoader field to this builder.
> I think what I was mostly worried about here is not whether the event methods are more/less useful than a Task. It was rather that I should be able to bind a progress indicator (for example) to the progress of the job(s), and that I should be able to kick off an asynchronous load task from FXML. I think this needs to be ironed out still.
>
> Thanks! Sorry for the long wait!!
> Richard