OFBiz
  1. OFBiz
  2. OFBIZ-3766

MacroScreenRenderer still uses HtmlFormRenderer (dependencies on html renderers)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: SVN trunk
    • Fix Version/s: SVN trunk
    • Component/s: framework
    • Labels:
      None

      Description

      There are a bug in MacroScreenRenderer.renderScreenletSubWidget. and several pages of the demo uses HtmlFormRenderer instead of MacroFormRenderer.

      The dependencies in org.ofbiz.widget.<non-html> directories against org.ofbiz.widget.html are the following

      1.- org\ofbiz\widget\menu\MenuWrapTransform.java --> import org.ofbiz.widget.html.HtmlMenuWrapper;
      2.- org\ofbiz\widget\screen\MacroScreenRenderer.java -->:import org.ofbiz.widget.html.HtmlScreenRenderer.ScreenletMenuRenderer;
      3.- org\ofbiz\widget\screen\ModelScreenWidget.java -->:import org.ofbiz.widget.html.HtmlMenuRenderer;
      4.- org\ofbiz\widget\screen\ModelScreenWidget.java -->:import org.ofbiz.widget.html.HtmlFormRenderer;
      5.- org\ofbiz\widget\screen\MacroScreenRenderer.java -->:import org.ofbiz.widget.html.HtmlFormRenderer;
      6.- org\ofbiz\widget\screen\ScreenFopViewHandler.java -->:import org.ofbiz.widget.html.HtmlScreenRenderer;

      I excluded:
      ScreenWidgetViewHandler.java which is in widget.screen but is a piece of the html renderers
      References to HtmlWidgetRenderer, which although it is in the html directory doesn't depend on other html directory sources.

      Comments:

      1.- OK.
      2.- The real dependency is with HtmlMenuRenderer.
      3,4.- To avoid default renderers, ModelScreenWidget should use only the renderer stored in the context.
      5.- bug.
      6.- if macro renderer will be the default one then this must be changed. It will be better if the default renderer can be taken from the context but AFAIK it isn't a default renderer in the context.

      After the patch the dependencies against html directory are:

      org\ofbiz\widget\menu\MenuWrapTransform.java --> import org.ofbiz.widget.html.HtmlMenuWrapper;
      org\ofbiz\widget\screen\MacroScreenRenderer.java -->:import org.ofbiz.widget.html.HtmlMenuRenderer;
      org\ofbiz\widget\screen\MacroScreenViewHandler.java -->:import org.ofbiz.widget.html.HtmlMenuRenderer;

      which are the expected ones, only with HtmlMenu because it isn't a macro replacement.

      Patch:

      MacroScreenRenderer.java

      • renderScreenletSubWidget method must use MacroFormRenderer instead of HtmlFormRenderer
      • Added ScreenletMenuRenderer copied from HtmlScreenRenderer. This make the dependency on HtmlMenuRenderer explicit and avoid the dependency on HtmlScreenRenderer

      MacroScreenViewHandler.java

      • To avoid using defaults in ModelScreenWidget each ViewHandler must store its form, tree and menu renderers in the context.

      ModelScreenWidget.java

      • There are not default renderers, each ScreenViewHandler must put its form, tree and menu renderers in the context.

      ScreenFopViewHandler.java

      • Use MacroScreenRenderer instead HtmlScreenRenderer.

      ScreenWidgetViewHandler.java (aka ScreenHtmlViewHandler)

      • To avoid using defaults in ModelScreenWidget each ViewHandler must store its form, tree and menu renderers in the context.

      ScreenXmlViewHandler.java

      • Should not depend on ScreenWidgetViewHandler

        Issue Links

          Activity

          Hide
          Jacques Le Roux added a comment -

          To commiters,

          Is there any intent to implement menurenderers? Else Blas's patch is a step in the right direction but does not change dramatically the situation (just make it a bit better, I'd say)

          Show
          Jacques Le Roux added a comment - To commiters, Is there any intent to implement menurenderers? Else Blas's patch is a step in the right direction but does not change dramatically the situation (just make it a bit better, I'd say)
          Hide
          Adrian Crum added a comment -

          I was looking at the rendering code a while back and thought it would be best to use the factory pattern for renderers. Factory methods could take rendering format and widget type as arguments, and then return the appropriate renderer.

          I agree that there are a lot of problems in the rendering code.

          Show
          Adrian Crum added a comment - I was looking at the rendering code a while back and thought it would be best to use the factory pattern for renderers. Factory methods could take rendering format and widget type as arguments, and then return the appropriate renderer. I agree that there are a lot of problems in the rendering code.
          Hide
          Jacques Le Roux added a comment -

          Yes Adrian, this makes sence indeed. Anyway there is no hurry, I even suspect that, except if we get some extra time, this will stay as is untill we will really get blocked there...

          In the meantime I could commit Blas'' work (which is clean) but I really wonder if it's needed...

          Show
          Jacques Le Roux added a comment - Yes Adrian, this makes sence indeed. Anyway there is no hurry, I even suspect that, except if we get some extra time, this will stay as is untill we will really get blocked there... In the meantime I could commit Blas'' work (which is clean) but I really wonder if it's needed...
          Hide
          Adrian Crum added a comment -

          Fixed in rev 1522002.

          Show
          Adrian Crum added a comment - Fixed in rev 1522002.

            People

            • Assignee:
              Jacques Le Roux
              Reporter:
              Blas Rodriguez Somoza
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development