[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) error handling for synchronous image loading, Scenegraph constructed 
on FX thread

ImageLoader imageLoader = new ImageLoader();

try {
     // createScene will use imageLoader.load(url) to construct images
     stage.setScene(createScene(imageLoader));
     stage.show();
} catch (ImageLoadingException e) {
     showError(e);
     exit();
}

2) error logging for asynchronously loaded images, Scenegraph 
constructed on FX thread

ImageLoader imageLoader = new ImageLoader();
imageLoader.setOnLoadingFailed(
     new EventHandler<ImageLoadingEvent>() {
         public void handle(ImageLoadingEvent event) {
             logger.log(event.getImage() + ": " + event.getException());
             event.consume();
         }
     });

Scene scene = new Scene(...);
...
Image image1 = imageLoader.loadAsync(image1Url);
Image image2 = imageLoader.loadAsync(image2Url);
...
stage.setScene(scene);
stage.show();

3) progress and error handling for asynchronously loaded images, 
Scenegraph constructed in background

final CountDownLatch allImagesLoaded = new CountDownLatch(imageCount);

ImageLoader imageLoader = new ImageLoader();
imageLoader.setOnLoadingFailed(
     new EventHandler<ImageLoadingEvent>() {
         public void handle(ImageLoadingEvent event) {
             exceptions.add(event.getImage(), event.getException());
             event.consume();
         }
     });
imageLoader.setOnLoadingProgress(
     new EventHandler<ImageLoadingEvent>() {
         public void handle(ImageLoadingEvent event) {
             updateProgress(event.getImage());
             event.consume();
         }
     });
imageLoader.setOnLoadingFinished(
     new EventHandler<ImageLoadingEvent>() {
         public void handle(ImageLoadingEvent event) {
             allImagesLoaded.countDown();
             event.consume();
         }
     });

...
Group root = new Group();
...
Image image1 = imageLoader.loadAsync(image1Url);
Image image2 = imageLoader.loadAsync(image2Url);
...
// Scenegraph is constructed here, wait for all images to finish loading
allImagesLoaded.await();
hideProgress();
if (exceptions.size() > 0) {
     showError(exceptions);
     exit();
}

display(root);

4) FXML
...
     <fx:define>
         <ImageLoader fx:id="myLoader" onLoadingFailed="#loadingFailed"/>
     </fx:define>
...
     <Image url=... loader="$myLoader"/>
...

Other answers are in-lined.

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?

Worker is nice in the situations when you get its reference before you 
have reference to the result.

So for example:

Worker worker = startBackgroundTask(arguments);

doSomethingElse();
// wait for worker to finish
// use worker.getValue()

// worker can be forgotten
worker = null;

In the case of Image loaded in background, we already have the reference 
to the unfinished result (Image) from which we get the worker. Then when 
the worker finishes, it returns the Image again? Or do we use Void 
worker here? Also the Worker stays attached to the Image for the rest of 
Image's lifetime.

> Note that whether you use event handlers or Worker, in either case you could have an ImageLoader or not.

I agree.

>   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?

Worker doesn't define any event handler and it has one worker to one 
task / result relationship. I want for ImageLoader and its registered 
handlers to be able to handle several image loading tasks at once. Which 
requires at least reference to the corresponding Image in each 
ImageLoadingEvent. Further the proposed set of events is only an initial 
set. The last time I looked the toolkit allowed to report some loading 
warnings as well and to get some metadata when the image file header has 
been loaded. So we might want to extend the set of ImageLoader events / 
event fields in the future with such information.

>> * 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?

I don't understand this question.

> Where did the setOnLoadingStarted etc methods live, on the Image or the ImageLoader?

ImageLoader

> Is there just a single ImageLoader in the world or as many as you create?

As many as you create. Of course the registered handlers will be called 
only for the images loaded through that particular ImageLoader instance.

>   Are the images loaded serially or in parallel (subject to some threshold)?

Currently there is an internal threshold. I mentioned the possibility of 
making it configurable through a ImageLoader constructor parameter.

>
>> * 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?

I don't want to duplicate the information which is already in Image.

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

What will the image.getLoadWorker().getValue() return (in the case we 
don't use Worker<void>)?

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

We already have a bind-able progress from Image.progress field.

> Thanks! Sorry for the long wait!!
> Richard

Thanks,
Lubo