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: Trunk
    • Fix Version/s: 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

          Blas Rodriguez Somoza created issue -
          Blas Rodriguez Somoza made changes -
          Field Original Value New Value
          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
          (I exclude ScreenWidgetViewHandler.java which is in widget.screen but is a piece of the html renderers)

          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;

          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 iits 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.



          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
          Blas Rodriguez Somoza made changes -
          Attachment OFBIZ-3766_html_dependencies.diff [ 12443968 ]
          Blas Rodriguez Somoza made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Jacques Le Roux made changes -
          Assignee Jacques Le Roux [ jacques.le.roux ]
          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...
          Jacques Le Roux made changes -
          Link This issue is blocked by OFBIZ-3233 [ OFBIZ-3233 ]
          Jacopo Cappellato made changes -
          Fix Version/s Release Branch 10.04 [ 12314832 ]
          Affects Version/s Release Branch 10.04 [ 12314832 ]
          Hide
          Adrian Crum added a comment -

          Fixed in rev 1522002.

          Show
          Adrian Crum added a comment - Fixed in rev 1522002.
          Adrian Crum made changes -
          Status Patch Available [ 10002 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          2h 14m 1 Blas Rodriguez Somoza 07/May/10 16:04
          Patch Available Patch Available Closed Closed
          1265d 15h 41m 1 Adrian Crum 24/Oct/13 07:46

            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