[Review request] Image loader API

[Lubomir Nerad]

Hi Richard,

On 1/23/2013 2:00 AM, Richard Bair wrote:

> Anyway, something to keep in mind. But back to Worker. I don't see the API of Worker as being incompatible in this case. Suppose for example, that you have ImageLoader implement Worker. The ImageLoader would start off with state == READY. As soon as the first request for image loading occurs (either synchronous or asynchronous) it changes state to SCHEDULED and then at the proper time (in sync case immediately, in async case when the background thread gets started) to RUNNING. Once it completes, it transitions to SUCCEEDED. When the next call to load an image occurs, it transitions back to READY->SCHEDULED->RUNNING. As long as new calls to load an image occur while it is currently RUNNING, it keeps RUNNING until they all finish processing. You could even have it transition immediately from SUCCEEDED to READY when it completes (so that READY is essentially "idle").
>
> You could spec it so that it never enters the FAILED state, and only enters CANCELLED if the user explicitly calls cancel (causing all image load operations to be cancelled).

I agree that obtaining Worker which specifies the state of the whole 
image loading process of a single ImageLoader can be useful in some use 
cases.

> Since Worker doesn't define the onXXX event handlers, you can redefine them, such that you have a subclass of WorkerStateEvent that defines your image load events, and have all the onXXX event handlers on ImageLoader to be based on those event handlers. getWorker could be defined to return ImageLoader for example.

We can do that currently only if the ImageLoadingEvent types will be 
completely unrelated to the WorkerStateEvent types. Otherwise we won't 
be able to correctly define ImageLoadingEvent.ANY or 
ImageLoadingEvent.LOADING_FINISHED associated with ImageLoadingEvent. 
Such generic event types would need to be associated with 
WorkerStateEvent and user code will need to upcast in order to access 
specific information associated with ImageLoadingEvent. If we leave the 
event types unrelated, I don't see any benefits in subclassing 
WorkerStateEvent.

So what if we leave these event classes unrelated and keep the 
ImageLoader as originally proposed (without extending Worker), but add 
the following two methods into it:

Worker getLoadingService();
Worker getLoadingTask(Image image);

The first one will allow to get the Worker which allows to observe the 
loading process as whole and the second one to observe loading of a 
single image.

Examples:

1) generic dialog with total progress
ImageLoader imageLoader = new ImageLoader();
image1 = imageLoader.loadAsync(...);
image2 = imageLoader.loadAsync(...);
image3 = imageLoader.loadAsync(...);
image4 = imageLoader.loadAsync(...);
totalProgressDialog.track(imageLoader.getLoadingService());

2) generic dialog with detailed progress
ImageLoader imageLoader = new ImageLoader();
image1 = imageLoader.loadAsync(...);
image2 = imageLoader.loadAsync(...);
image3 = imageLoader.loadAsync(...);
image4 = imageLoader.loadAsync(...);
detailedProgressDialog.track(
         imageLoader.getLoadingTask(image1),
         imageLoader.getLoadingTask(image2),
         imageLoader.getLoadingTask(image3),
         imageLoader.getLoadingTask(image4));

The advantages of this solution is that it will keep ImageLoader simple, 
while it will allow generic progress tracking (total or detailed). We 
can also return some Worker subclass which would allow to register 
WorkerStateEvent handlers or support advanced user cases (for example 
define Executor for loading service).

What do you think?

> This would at least keep the Worker / ImageLoader / Service APIs consistent and tied together without violating the Worker contract.
>
> The "value" of the ImageLoader could be defined as just being Void.
>
> But what about the other case, where you just want to use Image and not use ImageLoader? Do you have to use an ImageLoader to get better error handling?

In that case some platform default image loader will be used. We might 
allow the application to get access to it.

Thanks,
Lubo

>> 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.
> Hopefully the above (long) explanation helped here -- you really don't' have to have a one-task one-result Worker. One way to have multiple results is for the "value" to be an ObservableList that is populated with results, another is to just have Void as the type and then provide some other API (or none). Also as you noted Worker doesn't define the events (since the events were defined after the interface had already shipped so we could only add them to Worker / Task), so you actually have freedom to implement Worker and define some different (or additional) 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.
>> What will the image.getLoadWorker().getValue() return (in the case we don't use Worker<void>)?
> Will come back to this if we go that route. First in the above I took the position "ok we will have ImageLoader, what should it look like". Lets get closure on that. Maybe we can ignore the question of how to get the error when just loading via image and not using image loader (since it is sort of orthogonal question would say).
>
>>>> * 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.
> The difference is that I was thinking about the ImageLoader being a Worker at this point in the email and that the progress of that thing could be the aggregate of multiple background jobs, not just one.
>
> Richard