Re: svn commit: r820232 - in /ofbiz/trunk/framework: common/webcommon/portal/showPo
Locked
Re: svn commit: r820232 - in /ofbiz/trunk/framework: common/webcommon/portal/showPo
 
|
1. You don't need to use a counter in FreeMarker loops, there is a
built-in variable provided 2. Having to know in advance that you are going to render a form more than once is far from an ideal solution, what is two different screens use the same form on the same page? Quick fixes do not belong in the framework. 3. What about screens that use the screen cache? 4. The ScreenRenderer class now has a field called renderFormSeqNumber even though the ScreenRenderer knows nothing about the form widget. In general I think this solution is messy and not well thought out, it concerns me because we already have the iterateId solution you provided the other day and I'm worried that we are going to end up with a whole stack of strange variables that make no sense to look at in the code. I would prefer to see a solution where the widgets or the screens or the context keeps track of what widgets have previously been rendered and have then automatically take care of uniquely identifying the form elements themselves. Regards Scott On 30/09/2009, at 10:51 PM, [hidden email] wrote: > Author: hansbak > Date: Wed Sep 30 09:51:01 2009 > New Revision: 820232 > > URL: http://svn.apache.org/viewvc?rev=820232&view=rev > Log: > fix for OFBIZ-2414: if a portlet is included more than once the > links do not work, this fix will make these links unique. Sponsored > By Antwebsystems Co.Ltd: employee Berm > > Modified: > ofbiz/trunk/framework/common/webcommon/portal/showPortalPage.ftl > ofbiz/trunk/framework/widget/src/org/ofbiz/widget/WidgetWorker.java > ofbiz/trunk/framework/widget/src/org/ofbiz/widget/form/ > ModelForm.java > ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ > ScreenRenderer.java > > Modified: ofbiz/trunk/framework/common/webcommon/portal/ > showPortalPage.ftl > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/webcommon/portal/showPortalPage.ftl?rev=820232&r1=820231&r2=820232&view=diff > = > = > = > = > = > = > = > = > ====================================================================== > --- ofbiz/trunk/framework/common/webcommon/portal/showPortalPage.ftl > (original) > +++ ofbiz/trunk/framework/common/webcommon/portal/showPortalPage.ftl > Wed Sep 30 09:51:01 2009 > @@ -20,6 +20,7 @@ > <#if portalPage?has_content> > <table width="100%"> > <tr> > + <#assign renderSeq = 0/> > <#list portalPageColumns?if_exists as portalPageColumn> > <td style="vertical-align: top; <#if > portalPageColumn.columnWidthPercentage?has_content> width:$ > {portalPageColumn.columnWidthPercentage}%;</#if>"> > <#assign firstInColumn = true/> > @@ -31,7 +32,9 @@ > ${setRequestAttribute("portalPortletId", > portlet.portalPortletId)} > ${setRequestAttribute("portletSeqId", > portlet.portletSeqId)} > ${screens.render(portlet.screenLocation, > portlet.screenName)} > + ${screens.setRenderFormUniqueSeq(renderSeq)} > </div> > + <#assign renderSeq = renderSeq+1/> > </#if> > <#assign firstInColumn = false/> > </#if> > > Modified: ofbiz/trunk/framework/widget/src/org/ofbiz/widget/ > WidgetWorker.java > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/widget/src/org/ofbiz/widget/WidgetWorker.java?rev=820232&r1=820231&r2=820232&view=diff > = > = > = > = > = > = > = > = > ====================================================================== > --- ofbiz/trunk/framework/widget/src/org/ofbiz/widget/ > WidgetWorker.java (original) > +++ ofbiz/trunk/framework/widget/src/org/ofbiz/widget/ > WidgetWorker.java Wed Sep 30 09:51:01 2009 > @@ -268,15 +268,19 @@ > ModelForm modelForm = modelFormField.getModelForm(); > Integer itemIndex = (Integer) context.get("itemIndex"); > String iterateId = ""; > + String formUniqueId = ""; > String formName = (String) context.get("formName"); > if (UtilValidate.isEmpty(formName)) { > formName = modelForm.getName(); > } > if (UtilValidate.isNotEmpty(context.get("iterateId"))) { > - iterateId = (String) context.get("iterateId"); > + iterateId = (String) context.get("iterateId"); > + } > + if (UtilValidate.isNotEmpty(context.get("formUniqueId"))) { > + formUniqueId = (String) context.get("formUniqueId"); > } > if (itemIndex != null) { > - return formName + modelForm.getItemIndexSeparator() + > itemIndex.intValue() + iterateId + modelForm.getItemIndexSeparator() > + modelFormField.getName(); > + return formName + modelForm.getItemIndexSeparator() + > itemIndex.intValue() + iterateId + formUniqueId + > modelForm.getItemIndexSeparator() + modelFormField.getName(); > } else { > return formName + modelForm.getItemIndexSeparator() + > modelFormField.getName(); > } > > Modified: ofbiz/trunk/framework/widget/src/org/ofbiz/widget/form/ > ModelForm.java > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/widget/src/org/ofbiz/widget/form/ModelForm.java?rev=820232&r1=820231&r2=820232&view=diff > = > = > = > = > = > = > = > = > ====================================================================== > --- ofbiz/trunk/framework/widget/src/org/ofbiz/widget/form/ > ModelForm.java (original) > +++ ofbiz/trunk/framework/widget/src/org/ofbiz/widget/form/ > ModelForm.java Wed Sep 30 09:51:01 2009 > @@ -1423,6 +1423,10 @@ > ModelFormAction.runSubActions(this.rowActions, > localContext); > > localContext.put("itemIndex", > Integer.valueOf(itemIndex - lowIndex)); > + if > (UtilValidate.isNotEmpty(context.get("renderFormSeqNumber"))) { > + localContext.put("formUniqueId", > "_"+context.get("renderFormSeqNumber")); > + } > + > this.resetBshInterpreter(localContext); > > if (Debug.verboseOn()) Debug.logVerbose("In form got > another row, context is: " + localContext, module); > > Modified: ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ > ScreenRenderer.java > URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ScreenRenderer.java?rev=820232&r1=820231&r2=820232&view=diff > = > = > = > = > = > = > = > = > ====================================================================== > --- ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ > ScreenRenderer.java (original) > +++ ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ > ScreenRenderer.java Wed Sep 30 09:51:01 2009 > @@ -73,6 +73,7 @@ > protected Appendable writer; > protected MapStack<String> context; > protected ScreenStringRenderer screenStringRenderer; > + protected int renderFormSeqNumber = 0; > > public ScreenRenderer(Appendable writer, MapStack<String> > context, ScreenStringRenderer screenStringRenderer) { > this.writer = writer; > @@ -129,10 +130,15 @@ > writer.append(gwo.toString()); > } > } else { > + context.put("renderFormSeqNumber", > String.valueOf(renderFormSeqNumber)); > modelScreen.renderScreenString(writer, context, > screenStringRenderer); > } > return ""; > } > + > + public void setRenderFormUniqueSeq (int renderFormSeqNumber) { > + this.renderFormSeqNumber = renderFormSeqNumber; > + } > > public ScreenStringRenderer getScreenStringRenderer() { > return this.screenStringRenderer; > > |
smime.p7s (3K)
Re: svn commit: r820232 - in /ofbiz/trunk/framework: common/webcommon/portal/showPo
Administrator
|
Re: svn commit: r820232 - in /ofbiz/trunk/framework: common/webcommon/portal/showPo
|
Re: svn commit: r820232 - in /ofbiz/trunk/framework: common/webcommon/portal/showPo
|
|
Hi Hans,
That all well and good but what I have a problem with is short term solutions to long term problems, especially in the framework where things can be difficult to change further down the track. The widget code especially can be very difficult to navigate if things are allowed to get messy. My personal preference would be that things remain broken until a solid solution is put into place but I haven't got the time to propose or create one so I guess we just have to put up with these hackish solutions until someone has the time to do it properly. Even if you chose not to create a jira issue and patch for every single contribution I would beg with you to consider doing it for framework contributions and allow the community to have a say about changes to such an important piece of OFBiz. Regards Scott On 1/10/2009, at 3:24 PM, Hans Bakker wrote: > Hi Scott, > > The tip for the loop counter of freemarker is taken and we will change > it. > > If you have a proposal to fix this error a better way sure we have no > problem accepting it. As it is now, it fixes a real problem because it > happens very frequently that portlets are called pretty often in the > same screen. By the way, that is good so we reuse code. > > We are thinking about making text portlets for blocks of text where we > can build normal websites from. This error however always prevented > this > development. > > Regards, > Hans > > > On Wed, 2009-09-30 at 23:47 +1300, Scott Gray wrote: >> 1. You don't need to use a counter in FreeMarker loops, there is a >> built-in variable provided >> 2. Having to know in advance that you are going to render a form >> more >> than once is far from an ideal solution, what is two different >> screens >> use the same form on the same page? Quick fixes do not belong in the >> framework. >> 3. What about screens that use the screen cache? >> 4. The ScreenRenderer class now has a field called >> renderFormSeqNumber even though the ScreenRenderer knows nothing >> about >> the form widget. >> >> In general I think this solution is messy and not well thought out, >> it >> concerns me because we already have the iterateId solution you >> provided the other day and I'm worried that we are going to end up >> with a whole stack of strange variables that make no sense to look at >> in the code. >> >> I would prefer to see a solution where the widgets or the screens or >> the context keeps track of what widgets have previously been rendered >> and have then automatically take care of uniquely identifying the >> form >> elements themselves. >> >> Regards >> Scott >> >> On 30/09/2009, at 10:51 PM, [hidden email] wrote: >> >>> Author: hansbak >>> Date: Wed Sep 30 09:51:01 2009 >>> New Revision: 820232 >>> >>> URL: http://svn.apache.org/viewvc?rev=820232&view=rev >>> Log: >>> fix for OFBIZ-2414: if a portlet is included more than once the >>> links do not work, this fix will make these links unique. Sponsored >>> By Antwebsystems Co.Ltd: employee Berm >>> >>> Modified: >>> ofbiz/trunk/framework/common/webcommon/portal/showPortalPage.ftl >>> ofbiz/trunk/framework/widget/src/org/ofbiz/widget/ >>> WidgetWorker.java >>> ofbiz/trunk/framework/widget/src/org/ofbiz/widget/form/ >>> ModelForm.java >>> ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ >>> ScreenRenderer.java >>> >>> Modified: ofbiz/trunk/framework/common/webcommon/portal/ >>> showPortalPage.ftl >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/webcommon/portal/showPortalPage.ftl?rev=820232&r1=820231&r2=820232&view=diff >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> ==================================================================== >>> --- ofbiz/trunk/framework/common/webcommon/portal/showPortalPage.ftl >>> (original) >>> +++ ofbiz/trunk/framework/common/webcommon/portal/showPortalPage.ftl >>> Wed Sep 30 09:51:01 2009 >>> @@ -20,6 +20,7 @@ >>> <#if portalPage?has_content> >>> <table width="100%"> >>> <tr> >>> + <#assign renderSeq = 0/> >>> <#list portalPageColumns?if_exists as portalPageColumn> >>> <td style="vertical-align: top; <#if >>> portalPageColumn.columnWidthPercentage?has_content> width:$ >>> {portalPageColumn.columnWidthPercentage}%;</#if>"> >>> <#assign firstInColumn = true/> >>> @@ -31,7 +32,9 @@ >>> ${setRequestAttribute("portalPortletId", >>> portlet.portalPortletId)} >>> ${setRequestAttribute("portletSeqId", >>> portlet.portletSeqId)} >>> ${screens.render(portlet.screenLocation, >>> portlet.screenName)} >>> + ${screens.setRenderFormUniqueSeq(renderSeq)} >>> </div> >>> + <#assign renderSeq = renderSeq+1/> >>> </#if> >>> <#assign firstInColumn = false/> >>> </#if> >>> >>> Modified: ofbiz/trunk/framework/widget/src/org/ofbiz/widget/ >>> WidgetWorker.java >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/widget/src/org/ofbiz/widget/WidgetWorker.java?rev=820232&r1=820231&r2=820232&view=diff >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> ==================================================================== >>> --- ofbiz/trunk/framework/widget/src/org/ofbiz/widget/ >>> WidgetWorker.java (original) >>> +++ ofbiz/trunk/framework/widget/src/org/ofbiz/widget/ >>> WidgetWorker.java Wed Sep 30 09:51:01 2009 >>> @@ -268,15 +268,19 @@ >>> ModelForm modelForm = modelFormField.getModelForm(); >>> Integer itemIndex = (Integer) context.get("itemIndex"); >>> String iterateId = ""; >>> + String formUniqueId = ""; >>> String formName = (String) context.get("formName"); >>> if (UtilValidate.isEmpty(formName)) { >>> formName = modelForm.getName(); >>> } >>> if (UtilValidate.isNotEmpty(context.get("iterateId"))) { >>> - iterateId = (String) context.get("iterateId"); >>> + iterateId = (String) context.get("iterateId"); >>> + } >>> + if (UtilValidate.isNotEmpty(context.get("formUniqueId"))) { >>> + formUniqueId = (String) context.get("formUniqueId"); >>> } >>> if (itemIndex != null) { >>> - return formName + modelForm.getItemIndexSeparator() + >>> itemIndex.intValue() + iterateId + modelForm.getItemIndexSeparator() >>> + modelFormField.getName(); >>> + return formName + modelForm.getItemIndexSeparator() + >>> itemIndex.intValue() + iterateId + formUniqueId + >>> modelForm.getItemIndexSeparator() + modelFormField.getName(); >>> } else { >>> return formName + modelForm.getItemIndexSeparator() + >>> modelFormField.getName(); >>> } >>> >>> Modified: ofbiz/trunk/framework/widget/src/org/ofbiz/widget/form/ >>> ModelForm.java >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/widget/src/org/ofbiz/widget/form/ModelForm.java?rev=820232&r1=820231&r2=820232&view=diff >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> ==================================================================== >>> --- ofbiz/trunk/framework/widget/src/org/ofbiz/widget/form/ >>> ModelForm.java (original) >>> +++ ofbiz/trunk/framework/widget/src/org/ofbiz/widget/form/ >>> ModelForm.java Wed Sep 30 09:51:01 2009 >>> @@ -1423,6 +1423,10 @@ >>> ModelFormAction.runSubActions(this.rowActions, >>> localContext); >>> >>> localContext.put("itemIndex", >>> Integer.valueOf(itemIndex - lowIndex)); >>> + if >>> (UtilValidate.isNotEmpty(context.get("renderFormSeqNumber"))) { >>> + localContext.put("formUniqueId", >>> "_"+context.get("renderFormSeqNumber")); >>> + } >>> + >>> this.resetBshInterpreter(localContext); >>> >>> if (Debug.verboseOn()) Debug.logVerbose("In form got >>> another row, context is: " + localContext, module); >>> >>> Modified: ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ >>> ScreenRenderer.java >>> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ScreenRenderer.java?rev=820232&r1=820231&r2=820232&view=diff >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> = >>> ==================================================================== >>> --- ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ >>> ScreenRenderer.java (original) >>> +++ ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ >>> ScreenRenderer.java Wed Sep 30 09:51:01 2009 >>> @@ -73,6 +73,7 @@ >>> protected Appendable writer; >>> protected MapStack<String> context; >>> protected ScreenStringRenderer screenStringRenderer; >>> + protected int renderFormSeqNumber = 0; >>> >>> public ScreenRenderer(Appendable writer, MapStack<String> >>> context, ScreenStringRenderer screenStringRenderer) { >>> this.writer = writer; >>> @@ -129,10 +130,15 @@ >>> writer.append(gwo.toString()); >>> } >>> } else { >>> + context.put("renderFormSeqNumber", >>> String.valueOf(renderFormSeqNumber)); >>> modelScreen.renderScreenString(writer, context, >>> screenStringRenderer); >>> } >>> return ""; >>> } >>> + >>> + public void setRenderFormUniqueSeq (int renderFormSeqNumber) { >>> + this.renderFormSeqNumber = renderFormSeqNumber; >>> + } >>> >>> public ScreenStringRenderer getScreenStringRenderer() { >>> return this.screenStringRenderer; >>> >>> >> > -- > Antwebsystems.com: Quality OFBiz services for competitive rates > |
Re: svn commit: r820232 - in /ofbiz/trunk/framework: common/webcommon/portal/showPo
|
|
Re: svn commit: r820232 - in /ofbiz/trunk/framework: common/webcommon/portal/showPo
|
|
Re: svn commit: r820232 - in /ofbiz/trunk/framework: common/webcommon/portal/showPo
|
|
| Free forum by Nabble | Edit this page |