A couple of remarks:

1) You use a required configuration dependency, which means that if your 
update() method does not reject a configuration, the component will be 
initialized and started. Checking the configured properties again for 'null' is 
unnecessary.

2) I'm not sure why you decided that init() should be start() but you forgot to 
update the documentation and you did not update the destroy() method (so now 
it's asymmetric).

3) If the configuration actually does change, I don't see how this component 
would handle that. In fact, I don't see the actions that are done in the 
start() method being undone in the stop() method, so even though this component 
is stopped, the gadget is probably still there. In fact, when stopping and 
starting the service and changing the configuration you would create a whole 
series of login gadgets at various URLs. Also, more subtle, if the 
configuration is updated and you're somewhere in your "start" method, you run 
the risk of creating a gadget with partly the old, partly the new configuration.

Greetings, Marcel


On 12 Nov 2010, at 12:10 , <subversion at amdatu.org> wrote:

> Author: ivol37 at gmail.com
> Date: Fri Nov 12 12:10:28 2010
> New Revision: 418
> 
> Log:
> [AMDATU-171] Improved error handling of the gadget registration
> 
> Modified:
>   
> trunk/amdatu-authorization/login-gadget/src/main/java/org/amdatu/authorization/login/gadget/service/LoginGadgetImpl.java
> 
> Modified: 
> trunk/amdatu-authorization/login-gadget/src/main/java/org/amdatu/authorization/login/gadget/service/LoginGadgetImpl.java
> ==============================================================================
> --- 
> trunk/amdatu-authorization/login-gadget/src/main/java/org/amdatu/authorization/login/gadget/service/LoginGadgetImpl.java
>   (original)
> +++ 
> trunk/amdatu-authorization/login-gadget/src/main/java/org/amdatu/authorization/login/gadget/service/LoginGadgetImpl.java
>   Fri Nov 12 12:10:28 2010
> @@ -19,18 +19,22 @@
> import java.net.URL;
> import java.util.Dictionary;
> 
> -import org.amdatu.opensocial.gadgetmanagement.GadgetManagement;
> import org.amdatu.authorization.login.gadget.osgi.Activator;
> -import org.amdatu.web.httpcontext.HttpContextServiceFactory;
> -import org.amdatu.web.httpcontext.ResourceProvider;
> +import org.amdatu.opensocial.gadgetmanagement.GadgetManagement;
> import org.amdatu.opensocial.shindig.GadgetCategory;
> import org.amdatu.opensocial.shindig.GadgetDefinition;
> +import org.amdatu.web.httpcontext.HttpContextServiceFactory;
> +import org.amdatu.web.httpcontext.ResourceProvider;
> import org.apache.felix.dm.Component;
> import org.osgi.framework.BundleContext;
> import org.osgi.service.cm.ConfigurationException;
> import org.osgi.service.cm.ManagedService;
> import org.osgi.service.log.LogService;
> 
> +/**
> + * This service provides the login gadget.
> + * @author ivol
> + */
> public class LoginGadgetImpl implements ResourceProvider, ManagedService {
>     // The PID and configuration properties
>     public final static String PID = "org.amdatu.authorization.login.gadget";
> @@ -46,25 +50,32 @@
>     // The private HTTP context service for this bundle
>     private Component m_httpContextComponent;
> 
> -    private String m_hostname;
> -    private String m_portnr;
> +    // Configruation entries
> +    private volatile String m_hostname;
> +    private volatile String m_portnr;
> 
>     /**
>      * The init() method is invoked by the Felix dependency manager.
>      */
> -    public void init() {
> +    public void start() {
>         // Create our own http context service which registers static 
> resources and JSPs automatically
>         m_httpContextComponent = 
> m_httpContextServiceFactory.create(m_bundleContext, this);
> 
> -        // Register the gadget with the Gadget management service. Note that 
> we can do this as
> -        // many times as we want, since the gadget URL is the unique 
> identifier
> -        String gadgetSpecUrl = "http://"; + m_hostname + ":" + m_portnr + "/" 
> + Activator.RESOURCE_ID + "/jsp/LoginGadget.jsp";
> -        GadgetDefinition gadgetDef = new GadgetDefinition(gadgetSpecUrl, 
> GadgetCategory.AMDATU_PLATFORM, true);
> -        gadgetDef.setRank(0);
> -        m_gadgetManagement.addGadget(gadgetDef);
> -        m_logService.log(LogService.LOG_INFO, "Login gadget registered on 
> URL '" + gadgetSpecUrl + "'");
> -
> -        m_logService.log(LogService.LOG_INFO, getClass().getName() + " 
> service initialized");
> +        if (m_hostname != null && m_portnr != null) {
> +            // Register the gadget with the Gadget management service. Note 
> that we can do this as
> +            // many times as we want, since the gadget URL is the unique 
> identifier
> +            String gadgetSpecUrl =
> +                "http://"; + m_hostname + ":" + m_portnr + "/" + 
> Activator.RESOURCE_ID + "/jsp/LoginGadget.jsp";
> +            GadgetDefinition gadgetDef = new GadgetDefinition(gadgetSpecUrl, 
> GadgetCategory.AMDATU_PLATFORM, true);
> +            gadgetDef.setRank(0);
> +            m_gadgetManagement.addGadget(gadgetDef);
> +            m_logService.log(LogService.LOG_INFO, "Login gadget registered 
> on URL '" + gadgetSpecUrl + "'");
> +        }
> +        else {
> +            m_logService.log(LogService.LOG_ERROR, "Login gadget could not 
> be registered, hostname=" + m_hostname
> +                + ", portnr=" + m_portnr);
> +        }
> +        m_logService.log(LogService.LOG_INFO, getClass().getName() + " 
> service started");
>     }
> 
>     // The destroy() method is automatically invoked by the Felix dependency 
> manager
> @@ -84,7 +95,7 @@
>     @SuppressWarnings("unchecked")
>     public void updated(Dictionary dictionary) throws ConfigurationException {
>         if (dictionary != null) {
> -            checkAvailability(dictionary, new String[] {HOSTNAME, PORTNR});
> +            checkAvailability(dictionary, new String[] { HOSTNAME, PORTNR });
>             m_hostname = (String) dictionary.get(HOSTNAME);
>             m_portnr = (String) dictionary.get(PORTNR);
>         }
> _______________________________________________
> Amdatu-commits mailing list
> Amdatu-commits at amdatu.org
> http://lists.amdatu.org/mailman/listinfo/amdatu-commits


Reply via email to