Ed Burns wrote:
>Hello Ryan,
>
>This looks good, but I have some comments. I don't need to see another
>change bundle, but I want to see your response before I give the final
>review and approval.
>
>Thanks,
>
>Ed
>
>
>
>>>>>>On Thu, 09 Dec 2004 11:07:48 -0500, Ryan Lubke <Ryan.Lubke_at_Sun.COM> said:
>>>>>>
>>>>>>
>
>RL> Cleanup up jsf-tools generator code in preparation for JSF 1.2 work.
>RL> Generators (HtmlComponent, HtmlTaglib, and RenderKitSpecification) all
>RL> configured via a properties file. This should allow enough flexibility
>RL> to use
>RL> the taglib generator and renderkit doc generators with any faces-config.
>
>RL> SECTION: Modified Files
>RL> ----------------------------
>RL> M jsf-ri/build.xml
>RL> - copy the TAG-DEF files to jsf-ri before generating tags
>
>Question: why are we copying them to ${basedir}? Generally, we don't
>like to put ephemeral artifacts into ${basedir}. Can these be made to
>reside inside ${basedir}/build during their existence?
>
>
Yes, I will do this.
>RL> M jsf-tools/src/com/sun/faces/generate/AbstractGenerator.java
>RL> - general cleanup
>RL> - moved some methods common to all generator types to new GeneratorUtils
>RL> - Added new inner class CodeWriter with general helper methods for
>RL> writing read-only, write-only, read/write properties, various comment
>RL> types. This writer also simplifies formatting. User needs to call
>RL> indent
>RL> or unindent instead of adding formatting directly into Strings as
>RL> before.
>
>RL> Index: jsf-tools/src/com/sun/faces/generate/AbstractGenerator.java
>RL> ===================================================================
>
>...
>
>RL> + protected static final Set JAVA_KEYWORDS = new HashSet(57);
>
>I don't see the real value in hard coding the size. If someone were to
>add or remove an entry in the future, this may cause an IOOBE. In the
>name of futureproofing, please return to having no size. Since this is
>a generator, performance isn't as key.
>
>...
>
>RL> + protected static Map UNWRAPPERS = new HashMap(8);
>
>Same thing here.
>
>RL> + protected static Map WRAPPERS = new HashMap(8);
>
>and here.
>
>
>
I will revert.
>RL> +
>RL> + public void unindent() {
>RL> +
>RL> + depth.pop();
>RL> + updateFormatString(depth.size());
>RL> +
>RL> + } // END unindent
>
>Personally I prefer "outdent" to "unindent" but it's your call. This
>inner class is handy. It would seem to have a wider
>
>RL> M jsf-tools/src/com/sun/faces/generate/HtmlComponentGenerator.java
>RL> - general cleanup
>RL> - leverage CodeWriter
>
>RL> Index: jsf-tools/src/com/sun/faces/generate/HtmlComponentGenerator.java
>RL> ===================================================================
>
>RL> // Generate the copyright information
>RL> - writer.write(copyright);
>RL> + writer.writeJavadocComment(
>RL> + propManager.getProperty(PropertyManager.COPYRIGHT));
>
>RL> // Generate the package declaration
>RL> - writer.write("\npackage javax.faces.component.html;\n");
>RL> + writer.fwrite("\npackage javax.faces.component.html;\n");
>RL> writer.write("\n\n");
>
>RL> // Generate the imports
>RL> - writer.write("import java.io.IOException;\n");
>RL> - writer.write("import javax.faces.context.FacesContext;\n");
>RL> - writer.write("import javax.faces.el.MethodBinding;\n");
>RL> - writer.write("import javax.faces.el.ValueBinding;\n");
>RL> + writer.fwrite("import java.io.IOException;\n");
>RL> + writer.fwrite("import javax.faces.context.FacesContext;\n");
>RL> + writer.fwrite("import javax.faces.el.MethodBinding;\n");
>RL> + writer.fwrite("import javax.faces.el.ValueBinding;\n");
>RL> writer.write("\n\n");
>
>If we're going to have methods like writeReadWriteProperty(), why not
>have:
>
>public void writePackage(String fqpn);
>
>public void writeImport(String fqcn);
>
>Of course, if you want to be fancy, you could have addImport(String) and
>then writeImports() and this would have the effect of sorting the
>imports before writing them. Now, this would be a departure from your
>existing model, though. It would be the first time anything gets
>buffered before being output. When you start to go that far, you have
>to wonder if there is some other code out there, perhaps in netbeans,
>that facilitates the code generation process by enabling you to
>configure a java object instance with a bunch of code properties, and
>then kick it to write itself to a Writer or something. As it is now,
>you have a nice compromise, so I'd settle for adding
>write{Package,Import}().
>
>
Ok - I will go that route.
>
>...
>
>RL> // Generate the restoreState() method
>RL> - writer.write(" public void restoreState(FacesContext _context,
>RL> Object _state) {\n");
>RL> - writer.write(" Object _values[] = (Object[]) _state;\n");
>RL> - writer.write(" super.restoreState(_context, _values[0]);\n");
>RL> + writer.fwrite(
>RL> + "public void restoreState(FacesContext _context, Object
>RL> _state) {\n");
>RL> + writer.indent();
>
>Yes, this illustrates my point that we have a compromise between a true
>OO solution, and a PrintWriter with a few extra methods. A true OO
>solution would have a method that writes a method, taking the signature
>in some high level way. But let's not go there.
>
>RL> M jsf-tools/src/com/sun/faces/generate/HtmlTaglibGenerator.java
>RL> - general cleanup
>RL> - Use the data contained in the properties file to initialize the
>RL> previously
>RL> static structures
>
>What you lose when you do this is the ability to look in the
>HtmlTaglibGenerator.java file and see exactly what the complete set of
>configuration info the system needs. A lot of work went into making
>those static structures have the right set of values. Now, I'm aware
>you've moved the responsibility for containing that set of values into
>the PropertyManager, and that certainly is more flexible, but who checks
>that the set of values is correct? In other words, previously,
>HtmlTaglibGenerator was responsible for "knowing" the correct set of
>config values. Now HtmlBasicTaglib12.properties has this
>responsibility. Please make it clear that the system utterly depends on
>HtmlBasicTaglib12 being correct to function properly.
>
>
>Also, there are a number of constants for which I see no modern analog:
>
>RL> - private static final String TAGCLASSPATH =
>RL> "com.sun.faces.taglib.html_basic";
>RL> - private static final String TEICLASS =
>RL> "com.sun.faces.taglib.FacesTagExtraInfo";
>RL> - private static final String BODYCONTENT = "JSP";
>RL> - private static final String REQUIRED = "false";
>RL> - private static final String RTEXPRVALUE = "false";
>
>What happened to these?
>
>
These still exist as well as the methods that returned their values
(unchanged from the previous version)
in JspTLDGenerator.
>There was a lot of special case code in this class. Generally, it's
>hard to refactor that sort of thing and make sure all the cases are
>still covered. I bet that's why we avoid special case code at all
>costs. Anyway, are you certain you've migrated all the special cases to
>the new design correctly?
>
>
The code is equivalent.
>RL> Index: jsf-tools/src/com/sun/faces/generate/HtmlTaglibGenerator.java
>RL> ===================================================================
>
>RL> + // ValueHolder and ConvertibleValueHolder components
>RL> + st = new StringTokenizer(propManager.getProperty(
>RL> + PropertyManager.VALUE_HOLDER_COMPONENTS), ",");
>RL> + convertibleValueHolderComponents = new ArrayList(st.countTokens());
>RL> + while (st.hasMoreTokens()) {
>RL> + convertibleValueHolderComponents.add(st.nextToken());
>RL> }
>RL> - reader.close();
>RL> - if (log.isDebugEnabled()) {
>RL> - log.debug(" Copyright text contains " + sb.length() +
>RL> - " characters");
>RL> +
>RL> + // ValueBinding enabled properties
>RL> + st = new StringTokenizer(propManager.getProperty(
>RL> + PropertyManager.VALUE_BINDING_PROPERTIES), ",");
>RL> + valueBindingEnabledProperties = new ArrayList(st.countTokens());
>RL> + while (st.hasMoreTokens()) {
>RL> + valueBindingEnabledProperties.add(st.nextToken());
>
>If you're going to have a PropertyManager, why make the caller do the
>StringTokenizing? Why not add a method that does
>
>StringTokenizer toker =
> propManager.getTokenizedPropertyValues(PropertyManager.VALUE_BINDING_PROPERTIES);
>
>Or even have it give you an Iterator.
>
>
I didn't want to return an Iterator etc for all values, but this had
crossed my mind as well.
I will revisit.
>RL> A jsf-tools/conf/HtmlBasicTaglib12.properties
>RL> - generator properties file to generate taglibs, components, and
>RL> renderkit docs
>RL> for JSP 1.2
>
>As I mention above. This file must be correct. Please make sure it's
>clear to future developers how important the contents of this file are.
>
>RL> A jsf-tools/src/com/sun/faces/generate/JspTLDGenerator.java
>RL> - Base class for concrete JspTDLGenerator implmentations
>
>The XMLWriter class seems to have a lot of overlap with the
>HtmlResponseWriter in the JSF RI. If this is true, and there is
>significant cut and paste code reuse, I'd like to know why you had to
>resort to cut and paste re-use.
>
>
No cut and paste was used here. The only commonality is a few of the
method
signatures.
>Ed
>
>
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_javaserverfaces.dev.java.net
For additional commands, e-mail: dev-help_at_javaserverfaces.dev.java.net