[REVIEW] Make controller instantiation customizable

[Daniel Zwolenski]

Great to see some traction on this one! Just a few comments from someone
who will be using this feature heavily:

Any chance we could have the method take the string exactly as it is
defined in the FXML, rather than have FXML create a class and pass that in?
I'm thinking of two scenarios for this: 1) the class being loaded is via a
different classloader (i.e. my controllers and FXML are coming from a
repository somewhere and loaded on demand via a custom URL classloader),
and 2) the developer chooses to forgo tool support and instead uses a
'name' as the controller attribute in the FXML - this name could then be
used to lookup the controller in a DI factory/module (this allows for
multiple instances of the same controller class to be used, just configured
differently, which is not supported in the proposed mechanism).

I know this is probably not inline with your view of it all, but I would
love for a way to provide a default controller so if none was specified in
the FXML, the loader would use this. Again forgoing full tool support (ie
it couldn't validate or maintain my controller) but it would allow me to
overcome a couple of the problems defined in the 'FXML compromise' section
of this:
http://www.zenjava.com/2011/12/11/javafx-and-mvp-a-smorgasbord-of-design-patterns/

Can we have the FXMLLoader throw an exception if it tries to set a non-null
@FXML field. That would prevent any confusion around double-binding when a
controller is shared. I'd prefer this over the less obvious binding to last
possible field. This would also stop a newbie error I see on the forum
every now and then: they new their fields and then do stuff to it in the
constructor not realising it gets created by FXMLLoader. An exception would
sort them out sooner.

Similarly I'm not so keen on the null resulting in default behaviour as it
could lead to confusion as well. If for example my 'getController' was
looking something up from an AppContext and that thing was null because I
messed up my logic, I would want to get an error, not have a new Controller
be created for me, which made it look like it all worked fine but caused
hard-to-find problems later, etc. I'd vote for null being an error case,
and if the developer wants to use the default behaviour in their controller
then just extend DefaultControllerFactory (which has your default logic in
it) and call super.getController(). Just my preference on that one (i.e.
honest failure vs magic auto recovery) - I could live with either.



On Wed, Dec 14, 2011 at 7:44 AM, Richard Bair <richard.bair at oracle.com>wrote:

> Hi Greg,
>
> Is there also some API added to FXMLLoader to allow registration of a
> ControllerFactory? Beyond FXMLLoader, is there any other public API added?
>
> Thanks
> Richard
>
> On Dec 13, 2011, at 11:22 AM, Greg Brown wrote:
>
> > This is a review request for RT-17268:
> >
> > http://javafx-jira.kenai.com/browse/RT-17268
> >
> > In FXML 2.0, the caller did not have any control over controller
> creation. This prevented an application from using a dependency injection
> system such as Guice or Spring to manage controller initialization. This
> change adds a ControllerFactory interface to facilitate delegation of
> controller construction:
> >
> > public interface ControllerFactory {
> >    public Object getController(Class<?> type);
> > }
> >
> > When an instance of ControllerFactory is provided to FXMLLoader, the
> loader will delegate controller construction to the factory. An
> implementation may return null to indicate that it does not or cannot
> create a controller of the given type; in this case, the default controller
> construction mechanism will be employed by the loader. Implementations may
> also "recycle" controllers such that controller instances can be shared by
> multiple FXML documents. However, developers must be aware of the
> implications of doing this; primarily, that controller field injection
> should not be used in this case since it will result in the controller
> fields containing values from only the most recently loaded document.
> >
>
>