[richfaces-svn-commits] JBoss Rich Faces SVN: r11996 - in trunk/ui/editor/src/main/java/org/richfaces: convert/seamtext and 2 other directories.

Author: nbelaevski Date: 2008-12-23 11:08:35 -0500 (Tue, 23 Dec 2008) New Revision: 11996 Modified: trunk/ui/editor/src/main/java/org/richfaces/component/UIEditor.java trunk/ui/editor/src/main/java/org/richfaces/convert/seamtext/DefaultSeamTextConverter.java trunk/ui/editor/src/main/java/org/richfaces/convert/seamtext/SeamTextConverterFactory.java trunk/ui/editor/src/main/java/org/richfaces/renderkit/EditorRendererBase.java trunk/ui/editor/src/main/java/org/richfaces/renderkit/resources/EditorHTMLRenderer.java Log: Editor code review results committed Modified: trunk/ui/editor/src/main/java/org/richfaces/component/UIEditor.java =================================================================== --- trunk/ui/editor/src/main/java/org/richfaces/component/UIEditor.java 2008-12-23 16:06:54 UTC (rev 11995) +++ trunk/ui/editor/src/main/java/org/richfaces/component/UIEditor.java 2008-12-23 16:08:35 UTC (rev 11996) @@ -48,6 +48,7 @@ /** Id suffix of textarea which used as target element for tinyMCE scripts*/ public static final String EDITOR_TEXT_AREA_ID_SUFFIX = "TextArea"; + //TODO nick - it's a bad idea to create converterFactory instance for each component private SeamTextConverterFactory converterFactory = new SeamTextConverterFactory(); public abstract void setType(String type); @@ -142,6 +143,8 @@ if(isUseSeamText() && converter == null) { return converterFactory.getConverter(); } + + //TODO nick - check that converter is instance of SeamTextConverter if we use SeamText return converter; } Modified: trunk/ui/editor/src/main/java/org/richfaces/convert/seamtext/DefaultSeamTextConverter.java =================================================================== --- trunk/ui/editor/src/main/java/org/richfaces/convert/seamtext/DefaultSeamTextConverter.java 2008-12-23 16:06:54 UTC (rev 11995) +++ trunk/ui/editor/src/main/java/org/richfaces/convert/seamtext/DefaultSeamTextConverter.java 2008-12-23 16:08:35 UTC (rev 11996) @@ -51,10 +51,11 @@ /** The converter id for this converter. */ public static final String CONVERTER_ID = DefaultSeamTextConverter.class.getName(); - + //TODO nick - let everybody create instance of this class private DefaultSeamTextConverter() { } + //TODO nick - this class is not necessary private static class DefaultConverterHolder { private static final SeamTextConverter INSTANCE = new DefaultSeamTextConverter(); } @@ -66,7 +67,7 @@ public Object getAsObject(FacesContext context, UIComponent component, String value) { try { - + //TODO nick - value can be null here - see JavaDoc Reader r = new StringReader(value); HtmlSeamTextLexer lexer = new HtmlSeamTextLexer(r); HtmlSeamTextParser parser = new HtmlSeamTextParser(lexer); Modified: trunk/ui/editor/src/main/java/org/richfaces/convert/seamtext/SeamTextConverterFactory.java =================================================================== --- trunk/ui/editor/src/main/java/org/richfaces/convert/seamtext/SeamTextConverterFactory.java 2008-12-23 16:06:54 UTC (rev 11995) +++ trunk/ui/editor/src/main/java/org/richfaces/convert/seamtext/SeamTextConverterFactory.java 2008-12-23 16:08:35 UTC (rev 11996) @@ -44,9 +44,11 @@ public SeamTextConverter createSeamTextConverter() { try { - + //TODO nick - use ThreadCCL Class clazz = Class.forName("org.richfaces.convert.seamtext.DefaultSeamTextConverter"); Method method = clazz.getMethod("getInstance"); + + //TODO nick - static method doesn't need any argument, better pass null return (SeamTextConverter) method.invoke(clazz); } catch(Exception e) { Modified: trunk/ui/editor/src/main/java/org/richfaces/renderkit/EditorRendererBase.java =================================================================== --- trunk/ui/editor/src/main/java/org/richfaces/renderkit/EditorRendererBase.java 2008-12-23 16:06:54 UTC (rev 11995) +++ trunk/ui/editor/src/main/java/org/richfaces/renderkit/EditorRendererBase.java 2008-12-23 16:08:35 UTC (rev 11996) @@ -82,6 +82,7 @@ * @param value - component value * @return converted to String model value */ + //TODO nick - do we really need this method? protected String getConvertedStringValue(FacesContext context, UIEditor component, Object value) { return InputUtils.getConvertedStringValue(context, component, value); @@ -151,6 +152,8 @@ InternetResource resource = getResource(resourceName); String resourceUri = resource.getUri(context, null); String suffix = resourceUri.substring(resourceUri.indexOf(resourceName) + resourceName.length()); + + //TODO nick - I doubt suffix can be null if(suffix == null){ suffix = ""; } @@ -176,6 +179,8 @@ try { ClassLoader loader = Thread.currentThread() .getContextClassLoader(); + + //TODO nick - use org.ajax4jsf.resource.util.URLToStreamHelper.urlToStream(URL) InputStream is = loader.getResourceAsStream(configName + ".properties"); if (is == null) { @@ -184,6 +189,8 @@ + configName + "' was not found in class path"); } + + //TODO nick - streams should be closed, in finally block parameters.load(is); writer.writeText(this.convertProperties(parameters), null); writer.writeText(";\n", null); @@ -204,6 +211,7 @@ * @param component - Editor component instance * @throws IOException */ + //TODO nick - merge with writeEditorConfigurationParameters() method public void writeEditorCustomPluginsParameters(FacesContext context, UIEditor component) throws IOException { ResponseWriter writer = context.getResponseWriter(); @@ -325,6 +333,8 @@ + ScriptUtils.toScript(component.getHeight()) + ";\n", null); } + + //TODO nick - use local variables if (component.getOninit() != null && component.getOninit().length() > 0) { writer.writeText("tinyMceParams.oninit = function (event) {\n" + component.getOninit() + "\n" + "};\n", null); @@ -405,6 +415,7 @@ * @return true if needed or false if only target textarea should be rendered */ public boolean shouldRenderTinyMCE(UIEditor component) { + //TODO nick - use TINY_MCE_DISABLED_MODE.equalsIgnoreCase(component.getViewMode()) if (component.getViewMode() != null && component.getViewMode().equalsIgnoreCase( TINY_MCE_DISABLED_MODE)) { Modified: trunk/ui/editor/src/main/java/org/richfaces/renderkit/resources/EditorHTMLRenderer.java =================================================================== --- trunk/ui/editor/src/main/java/org/richfaces/renderkit/resources/EditorHTMLRenderer.java 2008-12-23 16:06:54 UTC (rev 11995) +++ trunk/ui/editor/src/main/java/org/richfaces/renderkit/resources/EditorHTMLRenderer.java 2008-12-23 16:08:35 UTC (rev 11996) @@ -117,7 +117,11 @@ int total = 0; BufferedReader br = new BufferedReader(new InputStreamReader(in)); + + //TODO nick - why we need BufferedWriter? BufferedWriter bw = new BufferedWriter(new OutputStreamWriter(out)); + + //TODO nick - cannot be changed in runtime, cache in private field String scriptSuffix = getSriptMappingSuffix(); String cssSuffix = getCssMappingSuffix(); @@ -156,6 +160,7 @@ total += line.getBytes().length; } } finally { + //TODO nick - close always flushes, no need to do that explicitly bw.flush(); br.close(); bw.close();