NIHVIVO-1304 Refactoring of widget code based on Brian Caruso's code review.

webapp/src/edu/cornell/mannlib/vitro/webapp/controller/freemarker/FreemarkerHttpServlet.java

@@ -544,8 +544,7 @@ public class FreemarkerHttpServlet extends VitroHttpServlet {
         }
     }
 
-    // This class is also used by WidgetDirective, so is public.
+    protected static class TemplateResponseValues extends BaseResponseValues {
-    public static class TemplateResponseValues extends BaseResponseValues {
         private final String templateName;
         private final Map<String, Object> map;
         

webapp/src/edu/cornell/mannlib/vitro/webapp/web/directives/WidgetDirective.java

@@ -4,7 +4,6 @@ package edu.cornell.mannlib.vitro.webapp.web.directives;
 
 import java.io.IOException;
 import java.io.Writer;
-import java.lang.reflect.Constructor;
 import java.lang.reflect.Method;
 import java.util.Map;
 
@@ -12,7 +11,6 @@ import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
 import edu.cornell.mannlib.vitro.webapp.controller.freemarker.FreemarkerHttpServlet;
-import edu.cornell.mannlib.vitro.webapp.controller.freemarker.FreemarkerHttpServlet.TemplateResponseValues;
 import edu.cornell.mannlib.vitro.webapp.utils.StringUtils;
 import edu.cornell.mannlib.vitro.webapp.web.widgets.Widget;
 import freemarker.core.Environment;
@@ -65,22 +63,18 @@ public class WidgetDirective extends BaseTemplateDirectiveModel {
         try {       
             String widgetClassName = WIDGET_PACKAGE + "." + StringUtils.capitalize(widgetName) + "Widget";
             Class<?> widgetClass = Class.forName(widgetClassName); 
-            // Use Constructor.newInstance() rather than Class.newInstance() so we can pass arguments 
+            Widget widget = (Widget) widgetClass.newInstance();             
-            // to the constructor.
+            Method method = widgetClass.getMethod(methodName, Environment.class, Map.class);
-            // Widget widget = (Widget) widgetClass.newInstance();
-            Constructor<?> widgetConstructor = widgetClass.getConstructor(new Class[]{Environment.class, String.class});
-            Widget widget = (Widget) widgetConstructor.newInstance(env, widgetName);              
-            Method method = widgetClass.getMethod(methodName);
             
             // Right now it seems to me that we will always be producing a string for the widget calls. If we need greater
             // flexibility, we can return a ResponseValues object and deal with different types here.
-            String output = (String) method.invoke(widget);
+            String output = (String) method.invoke(widget, env, params);
-            
+
-            String templateType = env.getDataModel().get("templateType").toString();
             // If we're in the body template, automatically invoke the doAssets() method, so it
-            // doesn't need to be called explicitly.
+            // doesn't need to be called explicitly from the enclosing template.
+            String templateType = env.getDataModel().get("templateType").toString();
             if ("doMarkup".equals(methodName) && FreemarkerHttpServlet.BODY_TEMPLATE_TYPE.equals(templateType)) {
-                output += widgetClass.getMethod("doAssets").invoke(widget);
+                output += widgetClass.getMethod("doAssets", Environment.class, Map.class).invoke(widget, env, params);
             }
               
             Writer out = env.getOut();

webapp/src/edu/cornell/mannlib/vitro/webapp/web/widgets/LoginWidget.java

@@ -5,25 +5,23 @@ package edu.cornell.mannlib.vitro.webapp.web.widgets;
 import java.util.HashMap;
 import java.util.Map;
 
+import javax.servlet.ServletContext;
+import javax.servlet.http.HttpServletRequest;
+
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
-import edu.cornell.mannlib.vitro.webapp.controller.freemarker.FreemarkerHttpServlet.TemplateResponseValues;
 import freemarker.core.Environment;
 
 public class LoginWidget extends Widget {
 
     private static final Log log = LogFactory.getLog(LoginWidget.class);
-    
-    public LoginWidget(Environment env, String name) {
-        super(env, name);
-    }
 
     @Override
-    protected TemplateResponseValues getTemplateResponseValues() {
+    protected WidgetTemplateValues process(Environment env, Map params, String widgetName, HttpServletRequest request, ServletContext context) {
         Map<String, Object> map = new HashMap<String, Object>();
         map.put("fruit", "bananas");
-        return new TemplateResponseValues (markupTemplateName(), map);
+        return new WidgetTemplateValues (getMarkupTemplateName(widgetName), map);
     }
 
 }

webapp/src/edu/cornell/mannlib/vitro/webapp/web/widgets/TestWidget.java

@@ -5,20 +5,18 @@ package edu.cornell.mannlib.vitro.webapp.web.widgets;
 import java.util.HashMap;
 import java.util.Map;
 
-import edu.cornell.mannlib.vitro.webapp.controller.freemarker.FreemarkerHttpServlet.TemplateResponseValues;
+import javax.servlet.ServletContext;
+import javax.servlet.http.HttpServletRequest;
+
 import freemarker.core.Environment;
 
 public class TestWidget extends Widget {
 
-    public TestWidget(Environment env, String name) {
-        super(env, name);
-    }
-
     @Override
-    protected TemplateResponseValues getTemplateResponseValues() {
+    protected WidgetTemplateValues process(Environment env, Map params, String widgetName, HttpServletRequest request, ServletContext context) {
         Map<String, Object> map = new HashMap<String, Object>();
         map.put("fruit", "bananas");
-        return new TemplateResponseValues (markupTemplateName(), map);
+        return new WidgetTemplateValues (getMarkupTemplateName(widgetName), map);
     }
 
 }

webapp/src/edu/cornell/mannlib/vitro/webapp/web/widgets/Widget.java

@@ -3,6 +3,8 @@
 package edu.cornell.mannlib.vitro.webapp.web.widgets;
 
 import java.io.IOException;
+import java.io.StringWriter;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -12,11 +14,10 @@ import javax.servlet.http.HttpServletRequest;
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
-import edu.cornell.mannlib.vitro.webapp.controller.freemarker.FreemarkerHttpServlet.TemplateResponseValues;
-import edu.cornell.mannlib.vitro.webapp.controller.freemarker.TemplateProcessingHelper;
 import freemarker.cache.TemplateLoader;
 import freemarker.core.Environment;
 import freemarker.template.Configuration;
+import freemarker.template.Template;
 import freemarker.template.TemplateHashModel;
 import freemarker.template.TemplateModelException;
 
@@ -24,31 +25,13 @@ public abstract class Widget {
 
     private static final Log log = LogFactory.getLog(Widget.class);
     
-    protected Environment env = null;
+    /* Widget implementations don't get any state when they get constructed so that they 
-    private TemplateProcessingHelper helper = null;
+     * can be reused. */
-    private String name = null;
+    public Widget() { }
- 
-//    protected TemplateDirectiveModel directive = null;
-//    protected Macro markupMacro = null;
-//    protected Macro assetsMacro = null;
     
-    public Widget(Environment env, String name) {
+    public String doAssets(Environment env, Map params) {
-       this.env = env; 
+        String widgetName = params.get("name").toString(); //getWidgetName();
-       this.name = name;
+        String templateName = getAssetsTemplateName(widgetName);
-       Configuration config = env.getConfiguration();
-       HttpServletRequest request = (HttpServletRequest) env.getCustomAttribute("request");
-       ServletContext context = (ServletContext) env.getCustomAttribute("context");
-       this.helper = new TemplateProcessingHelper(config, request, context);
-       
-       //this.directive = directive;
-       //Template template = getTemplate();
-       //Map templateMacros = template.getMacros();
-       //markupMacro = (Macro) templateMacros.get("markup");
-       //assetsMacro = (Macro) templateMacros.get("assets");
-    }
-    
-    public String doAssets() {
-        String templateName = assetsTemplateName();
 
         // Allow the assets template to be absent without generating an error.
         TemplateLoader templateLoader = env.getConfiguration().getTemplateLoader();
@@ -71,29 +54,79 @@ public abstract class Widget {
             log.error("Error getting asset values from data model.");
         }
         
-        return helper.processTemplateToString(templateName, map);  
+        return processTemplateToString(widgetName, env, templateName, map);
+ 
     }
     
+    public String doMarkup(Environment env, Map params) {
+        HttpServletRequest request = (HttpServletRequest) env.getCustomAttribute("request");
+        ServletContext context = (ServletContext) env.getCustomAttribute("context");
+        String widgetName = params.get("name").toString(); // getWidgetName();
+        WidgetTemplateValues values = process(env, params, widgetName, request, context);        
+        return processTemplateToString(widgetName, env, values);
+    }
+
     // Default assets template name. Can be overridden by subclasses.
-    protected String assetsTemplateName() {
+    protected String getAssetsTemplateName(String widgetName) {
-        return "widget-" + name + "-assets.ftl";
+        return "widget-" + widgetName + "-assets.ftl";
     }
   
-    public String doMarkup() {
-        TemplateResponseValues values = getTemplateResponseValues();
-        return helper.processTemplateToString(values);
-    }
-
     // Default markup template name. Can be overridden in subclasses, or assigned
-    // differently in the subclass doMarkup() method. For example, LoginWidget will
+    // differently in the subclass process() method. For example, LoginWidget will
     // select a template according to login processing status.
-    protected String markupTemplateName() {
+    protected String getMarkupTemplateName(String widgetName) {
-        return "widget-" + name + "-markup.ftl";
+        return "widget-" + widgetName + "-markup.ftl";
     }
     
-    protected abstract TemplateResponseValues getTemplateResponseValues();
+//    private String getWidgetName() {
+//        String name = this.getClass().getName();
+//        name= name.replaceAll(".*\\.", "");
+//        name = name.replaceAll("Widget$", "");
+//        name = name.substring(0, 1).toLowerCase() + name.substring(1);
+//        return name;
+//    }
+
+    protected abstract WidgetTemplateValues process(Environment env, Map params, String widgetName, HttpServletRequest request, ServletContext context);
+    
+    private String processTemplateToString(String widgetName, Environment env, String templateName, Map<String, Object> map) {
+        StringWriter out = new StringWriter();
+        Configuration config = env.getConfiguration();
+        try {
+            Template template = config.getTemplate(templateName);
+            template.process(map, out);
+        } catch (Throwable th) {
+            log.error("Could not process widget " + widgetName, th);
+        }
+        return out.toString();        
+    }
+    
+    private String processTemplateToString(String widgetName, Environment env, WidgetTemplateValues values) {
+        return processTemplateToString(widgetName, env, values.getTemplateName(), values.getMap());
+    }
+   
+    
+    protected static class WidgetTemplateValues {
+        private final String templateName;
+        private final Map<String, Object> map;
+        
+        public WidgetTemplateValues(String templateName, Map<String, Object> map) {
+            this.templateName = templateName;
+            this.map = map;
+        }
+
+        public WidgetTemplateValues put(String key, Object value) {
+            this.map.put(key, value);
+            return this;
+        }
+
+        public Map<String, Object> getMap() {
+            return Collections.unmodifiableMap(this.map);
+        }
+
+        public String getTemplateName() {
+            return this.templateName;
+        }
+ 
+    }
 
 }
-
-//# You can capture the output of an arbitrary part of the template into a context variable.
-//# You can interpret arbitrary context variable as if it were a template definition. 

webapp/web/templates/freemarker/widgets/test/widget-test-markup.ftl

@@ -1,5 +1,6 @@
 <#-- $This file is distributed under the terms of the license in /doc/license.txt$ -->
 
-<p>This is the test widget.</p>
+<div class="testWidget">
-
+    <h4>This is the test widget.</h4>    
-<p>${fruit}</p>
+    <p>I like ${fruit}.</p>
+</div>

webapp/web/templates/freemarker/widgets/widget-test.ftl

@@ -0,0 +1,16 @@
+<#-- $This file is distributed under the terms of the license in /doc/license.txt$ -->
+
+<#-- Test widget -->
+
+<#macro assets>
+    ${stylesheets.add("/css/test.css")}
+    ${scripts.add("/js/testscript.js")}
+    ${headScripts.add("/js/testheadscript.js")} 
+</#macro>
+
+<#macro markup>
+    <div class="testWidget">
+        <h4>This is the test widget.</h4>    
+        <p>I like ${fruit}.</p>
+    </div>
+</#macro>