Bug 341660

Comment 1 Marc Khouzam 2011-04-01 15:51:58 EDT

Created attachment 192385 [details]
Proposed fix (Pawel's solution)

Attached is a cleaned-up proposed fix, based on Pawel's solution. It modifies AbstractDsfService and it gives an example of the use of the changes in DSF-GDB.  This is still a proposal that we'll have to agree or disagree on when we start working on the next release.

P.S. I'm not attached to any of the names I chose, so please feel free to suggest other names if you wish.

Things to note:
==============
-API breakage
  - As mentioned, AbstractDsfService.initialize() goes from "public" to "public final".
  - AbstractDsfService.register() goes from "protected" to "private"
  - new abstract method AbstractDsfService.getClassNamesToRegiter()

- AbstractDsfService.register() now takes a list of names instead of an array to simplify things.
- AbstractDsfService.register() no longer supports being called more than once.

Things to decide:
================
1- Should register() modify the list returned by getClassNamesToRegiter() or should it make a copy first?  Note that register() modifies the list of properties. 

2- I don't think register() should automatically add the actual class type of this object as a registered name anymore.  We should leave this up to the service itself.  The reason is that with the new getClassNamesToRegiter() method, I can see someone doing something like this:
        
         BaseClass {
           protected List<String> getClassNamesToRegiter() {
              return new ArrayList<String>();
              // Here the designer did not explicitly add
              // BaseClass.class.getName() to the list of names to register
              // because he relied on AbstractDsfService.register() to add
              // it automatically.
           }
         };
        
         DerivedClass extends baseClass {
           protected List<String> getClassNamesToRegiter() {
              List<String> classes = super.getClassNamesToRegiter();
              
              // PROBLEM, classes does not contain BaseClass.class.getName()
              // and AbstractDsfService.register() won't add it!
              
              classes.add(DerivedClass.class.getName());
              return classes;
           }
         };

Comment 2 Marc Khouzam 2011-04-01 16:03:56 EDT

Another thing to note is that based on a suggestion from Bug 341465, I made the new AbstractDsfService.getClassNamesToRegiter() return a list instead of an array.  This makes it easier for extenders which can simply add a new name to register instead of writing out the full list each time.

Comment 3 Marc Khouzam 2011-04-01 21:01:57 EDT

I just realized that we also need to make some change for unregister().  I think it should be private and automatically be called from shutdown()

Comment 4 Anton Leherbauer 2011-04-04 02:54:17 EDT

> getClassNamesToRegiter

Typo?

Comment 5 Marc Khouzam 2011-04-04 10:08:50 EDT

Created attachment 192458 [details]
Updated Proposed fix to include unregister (Pawel's solution)

(In reply to comment #4)
> > getClassNamesToRegiter
> 
> Typo?

Thanks!  getClassNamesToRegiter() was wrong, and also getPropertiesToRegiter().

Here is the updated patch that fixes the two typos and two other things:

1- I applied the same new pattern to shutdown().  I made shutdown() final, unregister() private, and I created performShutdown().  This allows to automatically unregister, just like we would automatically register().  It just does not make sense to ask an extender to perform the unregister() if it no longer explicitly does the register().

2- I added a check that getClassNamesToRegister() != null before trying to automatically register.  This would allow someone to prevent registering with OSGi if that ever is needed.  Note that unregister() has a check to see if we were previously registered, so we can do that one blindly.

This is all still a proposal that we should agree on when we start working on Juno (next release).