2010-02-12

Dependency Injection in e4 - just why do we want to do that?

Lately, I have been giving a number of presentations of Eclipse e4. The focus has been on the changes that we will see when we program for e4 rather than 3.x.

With all the presentations, there have been a very lively discussion on the pros and cons of the new technologies in e4, but one subject in particular have been discussed: the use of Dependency Injection (DI) and what it will mean for productivity, testing and the tooling in Eclipse we all have grown so used to.

The following examples are taken from the excellent e4 contact manager by Kai Tödter that shows off some of the interesting features of e4 including theming and DI. The manager is very e4'ish and use the e4 features as these are probably meant to be used - with other words, I don't fault Kai for the examples or the problems I find in the examples.

At first glance the use of DI seems to give some very understandable code and it has the potential to do away with some of the listener code that we see in many places in 3.x code.

E.g. to follow the current selection in a view - here just expecting a single selected object - you would do something like the following in 3.x (forget about removing the listener again...):

public class DetailsView extends ViewPart {
  @Override
  public void createPartControl(Composite parent) {
    // ...

    final ISelectionService ss = getSite().getWorkbenchWindow().getSelectionService();
    ss.addPostSelectionListener(myListener);
    myListener.selectionChanged(null, ss.getSelection());
  }

  protected void setSelection(Contact contact) {
    // ... do something with the selection
  }

  private final ISelectionListener myListener = new ISelectionListener() {
    @Override
    public void selectionChanged(IWorkbenchPart part, ISelection selection) {
      if (!(selection instanceof IStructuredSelection)) {
        setSelection(null);
        return;
      }

      final IStructuredSelection ss = (IStructuredSelection) selection;
      if (ss.isEmpty() || !(ss.getFirstElement() instanceof Contact)) {
        setSelection(null);
        return;
      }
      setSelection((Contact) ss.getFirstElement());
    }
  };

  // ...
}

whereas, you will just do something like this in e4:

public class DetailsView {
  @Inject
  public void setSelection(@Optional @Named(IServiceConstants.SELECTION) Contact contact) {
    // ... do something with the selection
    
  }

  // ...
}

So far, so good. A lot less code compared with the 3.x version and the intension is quite clear once you know the injection is persistent, so changes in the context is automatically propagated to the view.

The main difference between the two versions lies in the way that you find and access services: in 3.x you basically have a Java method chain - here getSite().getWorkbenchWindow().getSelectionService() - where the e4 version uses a magic class or string - here @Optional @Named(IServiceConstants.SELECTION). The general Java tooling helps us access the services in 3.x, but what type of tooling will help us in e4? And how will we get warned about accidental changed in the e4 string?

A much more problematic example - as far as I am concerned - can be found in the SaveHandler of the contact manager. This handler is used to save the current contact in the details view by delegating the save functionality to the doSave method of the view (Again, I have cut away some irrelevant stuff):

public class SaveHandler {
  public void execute(IEclipseContext context, @Optional IStylingEngine engine,
      @Named(IServiceConstants.ACTIVE_SHELL) Shell shell,
      @Named(IServiceConstants.ACTIVE_PART) final MContribution contribution) throws InvocationTargetException,
      InterruptedException {
    final IEclipseContext pmContext = EclipseContextFactory.create(context, null);

    final ProgressMonitorDialog dialog = new ProgressMonitorDialog(shell);
    dialog.open();

    dialog.run(true, true, new IRunnableWithProgress() {
      public void run(IProgressMonitor monitor) throws InvocationTargetException, InterruptedException {
        pmContext.set(IProgressMonitor.class.getName(), monitor);
        Object clientObject = contribution.getObject();
        ContextInjectionFactory.invoke(clientObject, "doSave", pmContext, null);
      }
    });
    // ...
  }
}

Basically the handler creates a new context for the operation, opens a progress dialog, installs the progress monitor in the context, and then delegates to the doSave method with the new context.

The problem is found in the inner run methods: the name of the method of the view that is used to do the save operation itself is hard-coded as a string. How is this ever going to work with the Java tooling? How can we get quick assists or refactoring to work here? And how will we maintain this?

There are also some other problems with regards to testability. E.g. if a new doSave is ever created in the view, this will not provoke either a compile-time or a run-time error, but instead just execute both methods.

In the 3.x version, I would let the views that supports the doSave method implement a common Java interface - e.g. IDoSaveable and then test for this in the handler. An example of how this can be done is shown below:

The interface:

public interface IDoSaveable {
  void doSave(IProgressMonitor monitor);
}

The view:

public class DetailsView extends ViewPart implements IDoSaveable {
  @Override
  public void createPartControl(Composite parent) {
    // ...
  }

  @Override
  public void doSave(IProgressMonitor monitor) {
    // ... do save
  }
}

The handler - note that this version uses adaption, but you could use instanceof instead:

public class SaveHandler extends AbstractHandler {
  @Override
  public Object execute(ExecutionEvent event) throws ExecutionException {
    final IWorkbenchPart part = HandlerUtil.getActivePartChecked(event);
    final IDoSaveable saveablePart = (IDoSaveable) Platform.getAdapterManager().getAdapter(part, IDoSaveable.class);
    if (saveablePart != null) {
      final Shell shell = HandlerUtil.getActiveShellChecked(event);
      final ProgressMonitorDialog dialog = new ProgressMonitorDialog(shell);
      dialog.open();

      dialog.run(true, true, new IRunnableWithProgress() {
        public void run(IProgressMonitor monitor) throws InvocationTargetException, InterruptedException {
          saveablePart.doSave(monitor);
        }
      });
    }
    return null;
  }
}

Personally I see many advantages with this version:
  • It is actually more readable, as it has very little magic - aprt from the use of the adaption
  • It is easy to find all the views that supports the save operation.
  • Everything is based on the Java tooling and we have a no magic constants of any sorts
  • Refactoring works!
  • The activeWhen expression for the handler can simply test whether the activePart adapts to IDoSaveaeble.
What do you think? Is the new magic DI code worth the lost functionality in the Java tooling? Can we invent some new tooling to make up for the lost functionality? 

    17 comments:

    Unknown said...

    In my opinion dependency injection makes the code becomes less readable. I'm also not very fond of magic. So the whole thing is a bit scary.

    The tooling is as you say also an issue. I've no doubt they will figure it out, but it may be a while.

    Anonymous said...

    I haven't used E4 yet but I used to work with JBoss Seam earlier. Lot's of dependency injection everywhere. I was very enthusiastic about it at start but over time i began to see more and more problems. The order of execution is sometimes far from intuitive. It's harder to debug. Refactoring doesn't work properly, etc.

    I've found that most of the boilerplate code that you get rid of with injection can be removed by use of helper classes, services, and smart use common base classes.

    Tom said...

    a) Agreed you get some JavaTooling but you are finding the selection service path only because you've learned to do it that way. Finding this code path is as hard for a newbie.

    b) What prevents you from letting the e4-DetailsView-POJO implement your interface and cast the contribition object to this interface?

    Tom said...

    ... and naturally I agree that we need tooling. If we would manage to bring in better tooling into JDT this would not only help e4 but any other technology around that uses DI

    Anonymous said...

    @Tom there are nothing that prevent you from using a mixture of the "traditional" 3.x code and the new-style DI of e4.

    And, I - for one - will do just that for a foreseeable future :-)

    Tom said...

    That's what I meant and already said in my blog post. IMHO it is a strength of e4 that it doesn't force to do stuff in any way. If you don't like something don't use it. 3.x did gave you any choice.

    Tom said...

    ... 3.x didn't gave you any choice. :-)

    Eric Rizzo said...

    This over-use of DI is by far my biggest concern with e4, to the point that I think I will be forced to get involved on the mailing list only to try to add my own voice to whatever voices are speaking out about it already.
    The e4 code samples above are an order of magnitude less readable than the 3.x ones to an "ordinary" Java programmer - I'd bet hard cash that a survey or focus group would support that assertion. IIRC, one of the primary goals of e4 is to lower the barriers to entry into Eclipse development; do people really think copious annotations and DI does that? I find that very hard to believe.

    Philipp Kursawe said...

    I also have to agree with others, that the DI in e4 looks a lot more complicated than traditional Java Code. The more I see of it, the less I understand of it.

    First of all, why use annotations when OSGi DS can very well call your service components activate method with parameters in any order and number. It just figures it out. You can have a Map parameter, a BundleContext or a ComponentContext. None of them, all of them, or only some of them. In any order. DS SCR figures it out and calls your activate method.

    If my view implements ISelectionChangeListener it most likely wants to listen for changes in the selection. So the workbench (or whoever instantiates the IWorkbenchPart) just has to register that part with the selection service. If there has to be magic, than it can be used entirely without annotations in parameters.

    One could have the following methods in its part:
    void setSelection(ITextSelection)
    Called when the selection is text.
    void setSelection(Person)
    Called when the selection is an IStructuredSelection with getFirstElement() if it is castable to Address
    void setSelection(Person[])
    Called when there is more than one item in a IStructuredSelection.

    If this should work with an Interface based approach one could define the following interfaces:
    interface IStructuredSelectionListener<T>
    void onSelection(T item);
    void onSelection(T[] items);
    }

    interface ITextSelectionListener {
    void onSelection(ITextSelection text);
    }

    The tooling for 3.x could be improved in a way, that PDE adds content assists/QuickFix for classes of type ViewPart. If you press M1+1 on the class it could provide a list of possible interfaces the ViewPart can implement to take part on workbench changes. It could describe that implementing ITextSelection would automatically notify the view about such selections in the workbench.

    I would prefer the Workbench in e4 would just make more use of traditional OSGi services and less DI with annotations. In version 3.6 we already have the IWorkbench exposed as service.

    Tom said...

    The problem with OSGi-Services (you could naturally prove me wrong I'm not 100% an expert in this area) is that they are flat and which means how can't I distunguish between 2 SelectionProvider-Services who work e.g. on different levels of the UI?

    Everything you get from DI you can access through the IEclipseContext as well (in the end everything you get through DI is coming from there :-).

    You are NOT forced to use DI (well you need to get access to the IEclipseContext most likely through the contructor). The only thing you need to know is that ALL workbench-Services (as well as all OSGi-Services) register themselves in the IEclipseContext with their Interface-Name.

    The only problem is that you need to implement the logic your own that e.g. the selection provider could change when your part moves from the workbench-window to a dialog hence you need to unregister yourself as a listener and register on the new one.

    Probably we could provide an addon-bundle the possibility to make working with the IEclipseContext easier and base implementations which give users interface-Access to generic DI-Stuff (which is the only concept the core-platform understands).

    How does that sound? You are all invited to help make the e4 experience a good one for everybody, those who like and those who don't like DI.

    e4 is what we make from it and until now nobody stepped up and worked in this area.

    Philipp Kursawe said...

    Typically in OSGi selection providers can carry a service property that desribes which selection they present. Or they could just publish different anchor-interface all inheriting from ISelectionProvider. IWorkbenchPartSelectionProvider, IDialogSelectionProvider... I am not familiar with the internals of the selection provider and how many there can be. Anyway, I think it does not matter as the selection provider is the one consuming ISelectionChangeListeners. So the currently active provider chooses his listeners (white-board-pattern). Or can there be more than one selection provider active at a time?

    But your suggestion that I need to register and unregister myself as a listener is something I hoped would be gone finally in e4. I would prefer the white-board-pattern to be used everywhere and the listener management getting rid of once and for all.

    Eric Rizzo said...

    It's not a valid argument, from my perspective, to say "you aren't forced to use DI and/or annotations" because we typically spend more time reading code than writing it. Take a look at all the e4 example code you can find posted around Planet Eclipse and in the example apps, and in the platform; if it looks like the above, it's a problem. Plug-in/RCP developers learn by example and this kind of code is simply adding to the barrier to entry.
    So while I may not be forced to write code that way, I still have to fully grok it in order to be productive with e4.

    Unknown said...

    I think Thomas, Eric and Philipp are right on. While DI is a powerful feature it can also be used to make code far less readable. In the hands of the inexperienced it most certainly will be. In some environments that may not be much of a problem. But when it comes to Eclipse we're all very much depending on being able to read and understand the code.

    That aside I'm looking forward to seeing Eclipse RCP as a strong competition to .NET for your everyday business application. And I think e4 is what's going to make this happen. At least if the codebase continues to be understandable and of good quality.

    Anonymous said...

    @Eric re letting a view implement ISelectionChangeListener or something similar is just leading to the same old interface conflicts that we already try to solve with adaption. If I want a functionality, I want it stated explicitly and with no change of conflicts.

    Anyway, there are some interface that have multiple uses - e.g. IPropertyChangeListener - what will be the functionality of this then?

    Side comment: I always teach my RCP course participants that they *never* should let their view, wizard page, dialog, etc implement interfaces as there are no way to predict if the base class (ViewPart, WizardPage, TitleAreaDialog) will implement the same method in some future.

    Anonymous said...

    @Philipp and @Tom re the use of e4 features, I think we all know that a sharp double axed weapon like DI will be used and mis-used, and in the end that will fall back on all of us:

    If the platform is getting more difficult to problem, then people will shy away from it in the end and we will all loose!

    Tom said...

    @Eric: I agree that there are no samples around who don't use DI but the small e4 team can't work on every thing so I think it is our responsibility to close this gap.

    We are happy that at least Kai has been stepping up and working on an example application.

    So the only possibility we have to solve this is to work on example code who demonstrates how you work with e4 without DI.

    I repeat once more e4 is and has always been what the committers and driving forces wanted it to be.

    Tom said...

    Please attach yourself to https://bugs.eclipse.org/bugs/show_bug.cgi?id=302824.

    I started on a set of helper classes who provide you DI-Less Access to Workbench-Services. This is only an experiment until now and I haven't talked to the DI-experts so probably this is completely wrong.

    Please also note that services like ECommandService, ... are published as standard OSGi-Services so you can access them like you access any other OSGi-Service.