Author: maksimkaszynski Date: 2008-04-10 04:58:03 -0400 (Thu, 10 Apr 2008) New Revision: 7726 Modified: trunk/ui/inplaceInput/src/main/java/org/richfaces/renderkit/InplaceInputBaseRenderer.java trunk/ui/inplaceInput/src/main/resources/org/richfaces/renderkit/html/css/inplaceinput.xcss trunk/ui/inplaceInput/src/main/resources/org/richfaces/renderkit/html/scripts/inplaceinput.js Log: code reviewed Modified: trunk/ui/inplaceInput/src/main/java/org/richfaces/renderkit/InplaceInputBaseRenderer.java =================================================================== --- trunk/ui/inplaceInput/src/main/java/org/richfaces/renderkit/InplaceInputBaseRenderer.java 2008-04-10 08:42:40 UTC (rev 7725) +++ trunk/ui/inplaceInput/src/main/java/org/richfaces/renderkit/InplaceInputBaseRenderer.java 2008-04-10 08:58:03 UTC (rev 7726) @@ -76,20 +76,28 @@ return; } + //TODO: + //First check NULL clientId + if (InputUtils.isDisabled(inplaceInput) || InputUtils.isReadOnly(inplaceInput)) { if (logger.isDebugEnabled()) { logger.debug(("No decoding necessary since the component " + component.getId() + " is disabled")); } + //TODO: return? } String clientId = component.getClientId(context); + //TODO: NULL is NULL? if (clientId == null) { throw new NullPointerException("component " + inplaceInput.getClientId(context) + " client id is NULL" ); } - + //TODO: + //Should use clientId instead? clientId = clientId + UIInplaceInput.VALUE_SUFFIX; Map request = context.getExternalContext().getRequestParameterMap(); + //TODO: + //Request.get is enough if (request.containsKey(clientId)) { String newValue = (String)request.get(clientId); if (newValue != null) { @@ -98,6 +106,8 @@ } } + //TODO: + //Move that code to template, maybe? public void encodeControlsFacet(FacesContext context, UIComponent component) throws IOException { UIComponent facet = component.getFacet(CONTROLS_FACET); if ((facet != null) && (facet.isRendered())) { @@ -114,12 +124,15 @@ } public String encodeScriptAttributes(FacesContext context, UIComponent component) { + //TODO: + //Use string builder instead StringBuffer attributes = new StringBuffer(); attributes.append("var attributes = "); ScriptOptions options = new ScriptOptions(component); String defaultLabel = (String)component.getAttributes().get("defaultLabel"); + //TODO: length()==0 if (defaultLabel == null || defaultLabel.equals("")) { defaultLabel = EMPTY_DEFAULT_LABEL; } @@ -176,6 +189,7 @@ return cssMap.toString(); } + //TODO: use enum? private String buildCss(UIComponent component, int key, String suffix) { Object value; StringBuffer stringBuffer = new StringBuffer(); @@ -214,6 +228,7 @@ return stringBuffer.toString(); } + //TODO: remove? public String getAsEventHandler(FacesContext context, UIComponent component, String attributeName) { String event = (String) component.getAttributes().get(attributeName); String result = null; @@ -242,6 +257,7 @@ } protected boolean isEmptyDefaultLabel(String defaultLabel) { + //TODO: remove != null? if (defaultLabel != null && EMPTY_DEFAULT_LABEL.equals(defaultLabel)) { return true; } Modified: trunk/ui/inplaceInput/src/main/resources/org/richfaces/renderkit/html/css/inplaceinput.xcss =================================================================== --- trunk/ui/inplaceInput/src/main/resources/org/richfaces/renderkit/html/css/inplaceinput.xcss 2008-04-10 08:42:40 UTC (rev 7725) +++ trunk/ui/inplaceInput/src/main/resources/org/richfaces/renderkit/html/css/inplaceinput.xcss 2008-04-10 08:58:03 UTC (rev 7726) @@ -7,6 +7,11 @@ <f:verbatim> <![CDATA[ + /*** + TODO: check for duplicates!! + ***/ + + .rich-inplace { /*position:relative;*/ } Modified: trunk/ui/inplaceInput/src/main/resources/org/richfaces/renderkit/html/scripts/inplaceinput.js =================================================================== --- trunk/ui/inplaceInput/src/main/resources/org/richfaces/renderkit/html/scripts/inplaceinput.js 2008-04-10 08:42:40 UTC (rev 7725) +++ trunk/ui/inplaceInput/src/main/resources/org/richfaces/renderkit/html/scripts/inplaceinput.js 2008-04-10 08:58:03 UTC (rev 7726) @@ -3,7 +3,7 @@ Richfaces.InplaceInput.prototype = { - + //TODO: remove $$$$$ initialize: function(clientId, temValueKeepId, valueKeepId, tabberId, attributes, events, classes, barParams) { this.inplaceInput = $(clientId); this.inplaceInput.component = this; @@ -15,7 +15,7 @@ this.events = events; this.classes = classes; - + //TODO: static methods are preferred to be called within constructor this.currentText = this.getCurrentText(); this.value = this.valueKeeper.value; @@ -23,9 +23,11 @@ this.prevState = Richfaces.InplaceInput.STATES[0]; if (this.attributes.showControls) { + //TODO: Consider passing attributes by name instead of by index this.bar = new Richfaces.InplaceInputBar(barParams[0], barParams[1], barParams[2], barParams[3], barParams[4], this.attributes.verticalPosition, this.attributes.horizontalPosition); } + //TODO: move converting 'on'-event to prototype style to utils this.editEvent = this.attributes.editEvent.substring(2,this.attributes.editEvent.length); @@ -37,16 +39,17 @@ }, + //TODO: more cleanup here - remove references to DOM elements destroy: function() { this.inplaceInput.component = null; }, initHandlers : function() { - this.inplaceInput.observe(this.editEvent, function(e){this.switchingStatesHandler(e);}.bindAsEventListener(this)); - this.inplaceInput.observe("mouseout", function(e){this.inplaceMouseOutHandler(e);}.bindAsEventListener(this)); - this.inplaceInput.observe("mouseover", function(e){this.inplaceMouseOverHandler(e);}.bindAsEventListener(this)); + this.inplaceInput.observe(this.editEvent, this.switchingStatesHandler.bindAsEventListener(this)); + this.inplaceInput.observe("mouseout", this.inplaceMouseOutHandler.bindAsEventListener(this)); + this.inplaceInput.observe("mouseover", this.inplaceMouseOverHandler.bindAsEventListener(this)); - + //TODO: do like above - no need to create extra functions this.tempValueKeeper.observe("blur", function(e){this.tmpValueBlurHandler(e);}.bindAsEventListener(this)); this.tempValueKeeper.observe("keydown", function(e){this.tmpValueKeyDownHandler(e);}.bindAsEventListener(this)); @@ -79,6 +82,7 @@ */ inplaceMouseOverHandler : function(e) { + //TODO: Nick recommends regexp var className = this.inplaceInput.className; if (this.currentState == Richfaces.InplaceInput.STATES[0]) { if (className.indexOf(this.classes.COMPONENT.VIEW.HOVERED) == -1) { @@ -100,6 +104,7 @@ } }, + //TODO: Event.element here? switchingStatesHandler : function(e) { var el = (e.srcElement) ? e.srcElement : e.target; if ((el.id == this.inplaceInput.id) || (e.type == "focus")) { @@ -120,6 +125,8 @@ } } +//DONT TUCH THIZ CODE + if (this.invokeEvent(this.events.oneditactivation, this.inplaceInput, "rich:oneditactivation", {oldValue : this.valueKeeper.value, value : this.tempValueKeeper.value})) { var textSize = this.inplaceInput.offsetWidth; @@ -210,6 +217,7 @@ } if (this.attributes.selectOnEdit) { + //TODO: implement Select all method? Richfaces.InplaceInput.textboxSelect(this.tempValueKeeper, 0, this.tempValueKeeper.value.length); } this.tempValueKeeper.focus(); @@ -221,6 +229,7 @@ this.createNewText(this.currentText); this.inplaceInput.className = this.classes.COMPONENT.VIEW.NORMAL; + //TODO: see above this.inplaceInput.observe("mouseover", function(e){this.inplaceMouseOverHandler(e);}.bindAsEventListener(this)); }, @@ -238,7 +247,9 @@ * UTILITIES */ + //TODO: rename parameter to textWidth? setInputWidth : function(textSize) { + //TODO: use constants here if (this.currentState != 1) { return; } @@ -335,7 +346,8 @@ } } }, - + //TODO: remove Bucks + //TODO: to shared library invokeEvent : function(eventFunc, element, eventName, memo) { var result = false; if (eventFunc) { @@ -370,7 +382,7 @@ this.inplaceInput.removeChild(text); } }, - + //TODO: search within childNodes for textNode.value().trim().length == 0? getCurrentText : function() { return this.inplaceInput.childNodes[4]; }, @@ -398,6 +410,7 @@ }, initDimensions : function() { + //TODO: Crap code magic numbers - I propose 325 and 675 /*this.bar.style.visibility = "hidden"; this.bar.show();*/ @@ -467,6 +480,7 @@ Richfaces.InplaceInput.textboxSelect = function(oTextbox, iStart, iEnd) { if (Prototype.Browser.IE) { + //TODO: dont check 4 browser. Check for capability instead. var oRange = oTextbox.createTextRange(); oRange.moveStart("character", iStart); oRange.moveEnd("character", -oTextbox.value.length + iEnd);