On Jun 1, 2009, at 4:12 PM, Sergiu Dumitriu wrote:

> vmassol (SVN) wrote:
>> Author: vmassol
>> Date: 2009-06-01 10:49:54 +0200 (Mon, 01 Jun 2009)
>> New Revision: 20681
>>
>> Log:
>> XWIKI-3913: Add support for Component Manager events
>>
>> * TODO: Refactor localization module to use the refactored  
>> Observation module
>> * TODO: Refactor Rendering API module by removing notion of Source  
>> Macro Manager (rollback as before) and instead use Component  
>> Manager events to register Macros dynamically.
>>
>> Added: platform/core/trunk/xwiki-component/xwiki-component-default/ 
>> src/main/java/org/xwiki/component/manager/ 
>> ComponentDescriptorAddedEvent.java
>> ===================================================================
>> --- platform/core/trunk/xwiki-component/xwiki-component-default/src/ 
>> main/java/org/xwiki/component/manager/ 
>> ComponentDescriptorAddedEvent.java                           (rev 0)
>> +++ platform/core/trunk/xwiki-component/xwiki-component-default/src/ 
>> main/java/org/xwiki/component/manager/ 
>> ComponentDescriptorAddedEvent.java   2009-06-01 08:49:54 UTC (rev  
>> 20681)
>> +    public boolean matches(Object otherEvent)
>> +    {
>> +        boolean result = false;
>> +
>> +        if  
>> (ComponentDescriptorAddedEvent 
>> .class.isAssignableFrom(otherEvent.getClass())) {
>> +            ComponentDescriptorAddedEvent event =  
>> (ComponentDescriptorAddedEvent) otherEvent;
>
> I'd like to support a wildcard here, like a listener for all component
> registrations (if getRole == null, return true).
>
> Also, why is getRole.getName compared to simply getRole?

Fixed, thanks.

>> +            if (getRole().getName().equals(event.getRole())) {
>> +                result = true;
>> +            }
>> +        }
>>
>> Modified: platform/core/trunk/xwiki-containers/xwiki-container- 
>> servlet/src/main/java/org/xwiki/container/servlet/ 
>> XWikiPlexusServletContextListener.java
>> ===================================================================
>>         // This is a temporary bridge to allow non XWiki components  
>> to lookup XWiki components.
>> -        // We're putting the XWiki Component Manager instance in  
>> the Servlet Context so that it's
>> -        // available in the XWikiAction class which in turn puts  
>> it into the XWikiContext instance.
>> -        // Class that need to lookup then just need to get it from  
>> the XWikiContext instance.
>> -        // This is of course not necessary for XWiki components  
>> since they just need to implement
>> -        // the Composable interface to get access to the Component  
>> Manager or better they simply
>> -        // need to define the Components they require as field  
>> members and configure the Plexus
>> -        // deployment descriptors (components.xml) so that they  
>> are automatically injected.
>>         servletContextEvent.getServletContext().setAttribute(
>>              
>> org.xwiki.component.manager.ComponentManager.class.getName(),  
>> this.componentManager);
>>     }
>
> Why did you remove this comment?

Because It wasn't valid any more:
- we're not using XWikiAction
- there's no components.xml
- the compnent manager is now injected.

I thought it wasn't necessary to explain all this here and that saying  
it's a temporary bridge that allows non components to lookup  
components is enough.

>> Modified: platform/core/trunk/xwiki-core/src/test/java/com/xpn/ 
>> xwiki/XWikiTest.java
>> ===================================================================
>> --- platform/core/trunk/xwiki-core/src/test/java/com/xpn/xwiki/ 
>> XWikiTest.java       2009-06-01 07:45:41 UTC (rev 20680)
>> +++ platform/core/trunk/xwiki-core/src/test/java/com/xpn/xwiki/ 
>> XWikiTest.java       2009-06-01 08:49:54 UTC (rev 20681)
>> @@ -20,6 +20,7 @@
>> package com.xpn.xwiki;
>
> A bit of duplication here:

Actually I think that a bit of duplication isn't bad in tests if it  
makes the test simpler to read. That's what I thought there.

>> -        TestSaveEventListener listener = new  
>> TestSaveEventListener();
>> +        Mock mockListener = mock(EventListener.class);
>> +         
>> mockListener 
>> .stubs().method("getName").will(returnValue("testlistener"));
>> +         
>> mockListener 
>> .expects 
>> (once()).method("getEvents").will(returnValue(Arrays.asList(new  
>> DocumentSaveEvent("xwikitest:Some.Document"))));
>> +
>>         ObservationManager om = (ObservationManager)  
>> getComponentManager().lookup(ObservationManager.class);
>> -        om.addListener(new  
>> DocumentSaveEvent("xwikitest:Some.Document"), listener);
>> +        om.addListener((EventListener) mockListener.proxy());
>> Modified: platform/core/trunk/xwiki-observation/src/main/java/org/ 
>> xwiki/observation/internal/DefaultObservationManager.java
>> ===================================================================
>
>> +                for (Event listenerEvent : regListener.events) {
>
> Why do you check the class also? match() should take care of this, and
> it prohibits event inheritance.

Fixed, thanks.

Thanks for the review!

-Vincent
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs

Reply via email to