OFBiz
  1. OFBiz
  2. OFBIZ-3774

Add The Visitor Pattern To Screen Widget Model Classes

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: framework
    • Labels:
      None

      Description

      From time to time there has been interest expressed in introducing the visitor pattern to the screen widget model classes. This issue is intended to provide a forum on the subject and a means to vote on it. Details are in the comments.

      1. OFBIZ-3774.patch
        13 kB
        Adrian Crum
      2. OFBIZ-3774.patch
        76 kB
        Adrian Crum
      3. OFBIZ-3774.patch
        78 kB
        Adrian Crum
      4. OFBIZ-3774_ScreenWidgetVisitor.patch
        4 kB
        Dimitri Unruh
      5. OFBIZ-3774_ScreenWidgetVisitor.patch
        106 kB
        Dimitri Unruh

        Issue Links

          Activity

          Hide
          Adrian Crum added a comment -

          I agree that tests will help. Like any other effort in the OFBiz community, this is a voluntary one. The scope depends on the availability of volunteers.

          I appreciate your concerns and I believe those concerns have been addressed by creating a branch so development here will not interfere with the trunk.

          One thing I can assure, however (and my track record with this community bears this out) is that any issues caused by the redesign will not be left as-is with the excuse that I am too busy to fix them.

          The need for this change was covered thoroughly in the previous comments.

          Show
          Adrian Crum added a comment - I agree that tests will help. Like any other effort in the OFBiz community, this is a voluntary one. The scope depends on the availability of volunteers. I appreciate your concerns and I believe those concerns have been addressed by creating a branch so development here will not interfere with the trunk. One thing I can assure, however (and my track record with this community bears this out) is that any issues caused by the redesign will not be left as-is with the excuse that I am too busy to fix them. The need for this change was covered thoroughly in the previous comments.
          Hide
          Hans Bakker added a comment -

          As with any refactoring i am concerned about the introduction of new errors. Junit tests will certainly help here. I appreciate your your input here however for me i do not see the actual need of doing this. I am just concerned about new errors creeping in.

          Show
          Hans Bakker added a comment - As with any refactoring i am concerned about the introduction of new errors. Junit tests will certainly help here. I appreciate your your input here however for me i do not see the actual need of doing this. I am just concerned about new errors creeping in.
          Hide
          Adrian Crum added a comment -

          Certainly! Would you be willing to contribute those?

          Show
          Adrian Crum added a comment - Certainly! Would you be willing to contribute those?
          Hide
          Hans Bakker added a comment -

          In this refactoring, may we also expect junit tests?

          Show
          Hans Bakker added a comment - In this refactoring, may we also expect junit tests?
          Hide
          Adrian Crum added a comment -

          I added abstract visitor classes in rev 1215462. They are empty for now, but I imagine they will accumulate functionality as the conversion progresses.

          The next step is to extend the abstract classes to rendering classes and copy existing rendering code over to them. The tree widget and menu widget are fairly simple, so we might want to convert those first. Then we can take the lessons learned from those and apply them to the screen and form widgets.

          We will leave the original model classes untouched from now on so we can keep them updated from the trunk.

          Show
          Adrian Crum added a comment - I added abstract visitor classes in rev 1215462. They are empty for now, but I imagine they will accumulate functionality as the conversion progresses. The next step is to extend the abstract classes to rendering classes and copy existing rendering code over to them. The tree widget and menu widget are fairly simple, so we might want to convert those first. Then we can take the lessons learned from those and apply them to the screen and form widgets. We will leave the original model classes untouched from now on so we can keep them updated from the trunk.
          Hide
          Adrian Crum added a comment -

          In rev 1211809 I added exception handling to the two visitor interfaces. Classes that implement the visitor interfaces will need the ability to throw exceptions. We still need visitor interfaces/accept methods for menu models and form models.

          Show
          Adrian Crum added a comment - In rev 1211809 I added exception handling to the two visitor interfaces. Classes that implement the visitor interfaces will need the ability to throw exceptions. We still need visitor interfaces/accept methods for menu models and form models.
          Hide
          Adrian Crum added a comment -

          Dimitri,

          Thank you, but I think there is some confusion here. We don't need separate Jira issues for adding the visitor pattern to the screen widgets. Instead, we need separate Jira issues for other screen widget tasks that are not related to adding the visitor pattern to the screen widgets - tasks like the package rename you proposed.

          Steps 1 - 6 that I mentioned in my previous comment can be carried out in this issue. I was only suggesting that we make the changes for this issue in small steps so they are easy for others to follow and understand.

          Show
          Adrian Crum added a comment - Dimitri, Thank you, but I think there is some confusion here. We don't need separate Jira issues for adding the visitor pattern to the screen widgets. Instead, we need separate Jira issues for other screen widget tasks that are not related to adding the visitor pattern to the screen widgets - tasks like the package rename you proposed. Steps 1 - 6 that I mentioned in my previous comment can be carried out in this issue. I was only suggesting that we make the changes for this issue in small steps so they are easy for others to follow and understand.
          Hide
          Dimitri Unruh added a comment -

          New Jira issue: OFBIZ-4616

          Show
          Dimitri Unruh added a comment - New Jira issue: OFBIZ-4616
          Hide
          Adrian Crum added a comment -

          Yes, we need a separate Jira issue for the package rename. My hope is that we can stay in scope and provide a clear change trail for others to follow.

          With that in mind, I would like to see the redesign occur with small, easily defined steps:

          1. Add the visitor interfaces and accept methods.
          2. Create abstract classes for the interfaces.
          3. Extend the abstract classes to rendering classes and artifact-info-gatherer classes.
          4. Move existing widget model code to the new classes.
          5. Merge all changes to the trunk.
          6. Close this issue and start on the next one.

          The package rename would be a good candidate for the next redesign effort, and we will merge those changes to the trunk when they are completed.

          Show
          Adrian Crum added a comment - Yes, we need a separate Jira issue for the package rename. My hope is that we can stay in scope and provide a clear change trail for others to follow. With that in mind, I would like to see the redesign occur with small, easily defined steps: 1. Add the visitor interfaces and accept methods. 2. Create abstract classes for the interfaces. 3. Extend the abstract classes to rendering classes and artifact-info-gatherer classes. 4. Move existing widget model code to the new classes. 5. Merge all changes to the trunk. 6. Close this issue and start on the next one. The package rename would be a good candidate for the next redesign effort, and we will merge those changes to the trunk when they are completed.
          Hide
          Dimitri Unruh added a comment -

          Thank you Adrian,

          so I will continue with the screens.
          Shouldn't we open a new Jira issue now?

          Show
          Dimitri Unruh added a comment - Thank you Adrian, so I will continue with the screens. Shouldn't we open a new Jira issue now?
          Hide
          Adrian Crum added a comment -

          Added ScreenWidgetVisitor interface in rev 1211693.

          Show
          Adrian Crum added a comment - Added ScreenWidgetVisitor interface in rev 1211693.
          Hide
          Adrian Crum added a comment -

          Thank you Dimitri. I understand now. If we are going to change the package, then I would suggest something like:

          org.ofbiz.widget.factory
          org.ofbiz.widget.model.action
          org.ofbiz.widget.model.condition
          org.ofbiz.widget.model.widget
          org.ofbiz.widget.parser
          org.ofbiz.widget.renderer
          

          From my perspective, that type of packaging will facilitate code reuse.

          Show
          Adrian Crum added a comment - Thank you Dimitri. I understand now. If we are going to change the package, then I would suggest something like: org.ofbiz.widget.factory org.ofbiz.widget.model.action org.ofbiz.widget.model.condition org.ofbiz.widget.model.widget org.ofbiz.widget.parser org.ofbiz.widget.renderer From my perspective, that type of packaging will facilitate code reuse.
          Hide
          Dimitri Unruh added a comment -

          The main reason is a better overview A ModeScreenWidget.java with 2000 lines isn't easy to maintain.

          At the end I would prefer something like:

          org.ofbiz.widget.screen.action
          org.ofbiz.widget.screen.condition
          org.ofbiz.widget.screen.handler
          org.ofbiz.widget.screen.renderer
          org.ofbiz.widget.screen.widget
          

          I also would do this for form, menu and tree. So it would be more clear if you are searching for something.

          Show
          Dimitri Unruh added a comment - The main reason is a better overview A ModeScreenWidget.java with 2000 lines isn't easy to maintain. At the end I would prefer something like: org.ofbiz.widget.screen.action org.ofbiz.widget.screen.condition org.ofbiz.widget.screen.handler org.ofbiz.widget.screen.renderer org.ofbiz.widget.screen.widget I also would do this for form, menu and tree. So it would be more clear if you are searching for something.
          Hide
          Adrian Crum added a comment -

          Thank you Dimitri. I do not understand the need for a package change. Could you explain the reason for it please?

          Show
          Adrian Crum added a comment - Thank you Dimitri. I do not understand the need for a package change. Could you explain the reason for it please?
          Hide
          Dimitri Unruh added a comment -

          Adrian,

          maybe you will have a chance to review the current patch.
          You will see my idea for the package refactoring (package: org.ofbiz.widget.screen.widget). If this is something you could agree with, I will continue with it.

          Anyway, I did some "dirty" workarounds for the current version, wich we will remove at the end of the refactoring process.

          Show
          Dimitri Unruh added a comment - Adrian, maybe you will have a chance to review the current patch. You will see my idea for the package refactoring (package: org.ofbiz.widget.screen.widget). If this is something you could agree with, I will continue with it. Anyway, I did some "dirty" workarounds for the current version, wich we will remove at the end of the refactoring process.
          Hide
          Adrian Crum added a comment - - edited

          Incremental changes will be easier for me to review. On the other hand, we don't want to make the changes too small or this will drag on forever.

          Go ahead and add the accept method to the models.

          Show
          Adrian Crum added a comment - - edited Incremental changes will be easier for me to review. On the other hand, we don't want to make the changes too small or this will drag on forever. Go ahead and add the accept method to the models.
          Hide
          Dimitri Unruh added a comment -

          Adrian,

          if we don't need backward compatibility, I would like to do some package refactoring. I will provide a patch for the screen package as an example.
          Also I will provide a first patch file for the visitor pattern (at first, only your visitor interface).

          What do you think?

          Show
          Dimitri Unruh added a comment - Adrian, if we don't need backward compatibility, I would like to do some package refactoring. I will provide a patch for the screen package as an example. Also I will provide a first patch file for the visitor pattern (at first, only your visitor interface). What do you think?
          Hide
          Adrian Crum added a comment -

          Once all of the behavior has been moved to external classes, we could continue the process (in a separate Jira issue) to turn the screen widget models into immutable data structures. That will improve performance and memory use, and it can help cut down on the instances of thread-unsafe code being introduced into the project (a recurring problem in this project).

          Show
          Adrian Crum added a comment - Once all of the behavior has been moved to external classes, we could continue the process (in a separate Jira issue) to turn the screen widget models into immutable data structures. That will improve performance and memory use, and it can help cut down on the instances of thread-unsafe code being introduced into the project (a recurring problem in this project).
          Hide
          Adrian Crum added a comment -

          By the way, the existing patches are POC type work - they should not be used directly. Instead, use them as a guideline. One of the things I tried to do in those patches was maintain backward compatibility, but recent discussions on the dev mailing list have opened up the possibility to make newer versions of OFBiz less backwards compatible - if there is a significant improvement involved.

          Show
          Adrian Crum added a comment - By the way, the existing patches are POC type work - they should not be used directly. Instead, use them as a guideline. One of the things I tried to do in those patches was maintain backward compatibility, but recent discussions on the dev mailing list have opened up the possibility to make newer versions of OFBiz less backwards compatible - if there is a significant improvement involved.
          Hide
          Adrian Crum added a comment -

          Cool! Branch has been created:

          https://svn.apache.org/repos/asf/ofbiz/branches/20111115ScreenWidgetRedesign

          Check out the branch to a local copy, make your changes, and post patches here. I will review them and commit them to the branch.

          Show
          Adrian Crum added a comment - Cool! Branch has been created: https://svn.apache.org/repos/asf/ofbiz/branches/20111115ScreenWidgetRedesign Check out the branch to a local copy, make your changes, and post patches here. I will review them and commit them to the branch.
          Hide
          Dimitri Unruh added a comment -

          I would like to help and work on it

          Show
          Dimitri Unruh added a comment - I would like to help and work on it
          Hide
          Adrian Crum added a comment -

          The first thing we need to do is get a team assembled who are willing to work on this. It is a sizable effort.

          Second, we should create a branch for the screen widget refactoring. In addition to the visitor pattern, there have been some other very good ideas on screen widget changes - so those could be discussed and included in the branch too.

          Finally, when the work in the branch is completed, we will ask the community to review it and vote on merging it back to the trunk.

          Show
          Adrian Crum added a comment - The first thing we need to do is get a team assembled who are willing to work on this. It is a sizable effort. Second, we should create a branch for the screen widget refactoring. In addition to the visitor pattern, there have been some other very good ideas on screen widget changes - so those could be discussed and included in the branch too. Finally, when the work in the branch is completed, we will ask the community to review it and vote on merging it back to the trunk.
          Hide
          Dimitri Unruh added a comment -

          Adrian,

          what should we do with this? Vote?
          This is really nice

          Show
          Dimitri Unruh added a comment - Adrian, what should we do with this? Vote? This is really nice
          Hide
          Adrian Crum added a comment -

          Updated patch to latest trunk revision.

          Show
          Adrian Crum added a comment - Updated patch to latest trunk revision.
          Hide
          Adrian Crum added a comment -

          Updated patch. I implemented a visitor renderer by copying the MacroScreenRenderer code.

          Making the change wasn't too hard. The visitor pattern actually makes a lot of things easier from the rendering viewpoint. The difficulties I ran into were due to maintaining backward-compatibility.

          Show
          Adrian Crum added a comment - Updated patch. I implemented a visitor renderer by copying the MacroScreenRenderer code. Making the change wasn't too hard. The visitor pattern actually makes a lot of things easier from the rendering viewpoint. The difficulties I ran into were due to maintaining backward-compatibility.
          Hide
          Adrian Crum added a comment -

          One thing I would like to clarify: the patch only includes screen sub-widgets, but the goal would be apply the visitor pattern to all screen widget model classes - like Marc mentioned.

          An abstract class for renderers that navigates the model structure probably won't work - because each rendering format will require its own sequence of events, or its own mode of navigation. You can't determine in advance how a particular rendering format will need to navigate the structure. The existing widget models try to do this form of prediction and it has caused problems in the past.

          I'm currently working on porting the macro renderer over to the visitor interface to see what kind of problems we might encounter.

          Show
          Adrian Crum added a comment - One thing I would like to clarify: the patch only includes screen sub-widgets, but the goal would be apply the visitor pattern to all screen widget model classes - like Marc mentioned. An abstract class for renderers that navigates the model structure probably won't work - because each rendering format will require its own sequence of events, or its own mode of navigation. You can't determine in advance how a particular rendering format will need to navigate the structure. The existing widget models try to do this form of prediction and it has caused problems in the past. I'm currently working on porting the macro renderer over to the visitor interface to see what kind of problems we might encounter.
          Hide
          Marc Morin added a comment -

          We did a similar visitor pattern to the entire presentment (Screen, Form, Menu) space and the controller. This path, mainly separation of concern, is very powerful, and allows easy extension to the system.

          The typical implementation path that has been taken, is to develop a series of abstract visitors that have a few base behaviors, navigation of the components, and throwing exceptions for each component. This enables a specific visitors to extend the base navigation class, to get full model navigation and add any specific behavior with a surprisingly small amount of code.

          We've done "Presentment" visitors for walking, validation (examining location/name references looking for dangling screen/from/controller references, etc.. used in junit test cases - fail build if broken links), security auto-notation (propagates service security declarations, to screens model objects, and will automatically remove buttons, links to areas in the application that the user doesn't have rights to access.) We have a partial dump (pretty-print) for the PresentmentModel as well.

          Externalizing the creation of the model objects, is crucial to getting the full power of this type of change. The model objects should have nothing but their attributes with their getter/setter methods (and the accept).

          Show
          Marc Morin added a comment - We did a similar visitor pattern to the entire presentment (Screen, Form, Menu) space and the controller. This path, mainly separation of concern, is very powerful, and allows easy extension to the system. The typical implementation path that has been taken, is to develop a series of abstract visitors that have a few base behaviors, navigation of the components, and throwing exceptions for each component. This enables a specific visitors to extend the base navigation class, to get full model navigation and add any specific behavior with a surprisingly small amount of code. We've done "Presentment" visitors for walking, validation (examining location/name references looking for dangling screen/from/controller references, etc.. used in junit test cases - fail build if broken links), security auto-notation (propagates service security declarations, to screens model objects, and will automatically remove buttons, links to areas in the application that the user doesn't have rights to access.) We have a partial dump (pretty-print) for the PresentmentModel as well. Externalizing the creation of the model objects, is crucial to getting the full power of this type of change. The model objects should have nothing but their attributes with their getter/setter methods (and the accept).
          Hide
          Adrian Crum added a comment -

          The attached patch is a beginning step - it has the visitor interface for the screen sub-widgets, and it adds the necessary accept method to the sub-widgets. There is no interface implementation.

          The screen widget model classes already use a pattern similar to visitor called double dispatch. Essentially, the ScreenStringRenderer implementation calls the renderWidgetString method of each widget, and that widget method calls the renderXxxx method in the ScreenStringRenderer. With the visitor pattern, the renderWidgetString method is called accept, and the renderXxxx method is called visit. So, it's basically the same design but with a slight difference.

          In the current code, the visitor (ScreenStringRenderer) passes a number of arguments to the model widget renderWidgetString method. In the attached patch, the visitor (ScreenWidgetVisitor) passes only itself as an argument to the model widget accept method. The reason for this difference is flexibility and expandability. Set up this way, the visitor could be anything - a renderer, an artifact info gatherer, a pretty printer, etc. There is no implementation implied in the design.

          Each renderer would implement ScreenWidgetVisitor. The code inside each model widget renderWidgetString method would be moved to the corresponding visit method in the renderer. Likewise, the artifact-info-gatherer would implement ScreenWidgetVisitor. The existing model widget artifact-info-gathering code would be moved to the corresponding visit method in the artifact-info-gatherer.

          Once those changes are made, the model widgets will contain only object construction code. Even that code could be moved to a builder class that implements ScreenWidgetVisitor.

          The benefit of this change will be improved separation of concerns. Right now the screen widget model classes try to be everything to everybody. After the change they will be little more than data structures that various behavior-specific objects will refer to. Rendering code is not mixed in with object construction code or with artifact-gathering code. The API is simplified.

          A change like this could have serious downstream consequences for OFBiz users who have modified the screen widget model classes. One approach we could take is to put the visit methods in ScreenStringRenderer (instead of having a separate interface) and then deprecate the existing renderXxxx methods. The downside is code maintainance - bug fixes and new features will have to be applied to both patterns.

          If you believe the change is worthwhile, then please vote on this issue.

          Show
          Adrian Crum added a comment - The attached patch is a beginning step - it has the visitor interface for the screen sub-widgets, and it adds the necessary accept method to the sub-widgets. There is no interface implementation. The screen widget model classes already use a pattern similar to visitor called double dispatch. Essentially, the ScreenStringRenderer implementation calls the renderWidgetString method of each widget, and that widget method calls the renderXxxx method in the ScreenStringRenderer. With the visitor pattern, the renderWidgetString method is called accept, and the renderXxxx method is called visit. So, it's basically the same design but with a slight difference. In the current code, the visitor (ScreenStringRenderer) passes a number of arguments to the model widget renderWidgetString method. In the attached patch, the visitor (ScreenWidgetVisitor) passes only itself as an argument to the model widget accept method. The reason for this difference is flexibility and expandability. Set up this way, the visitor could be anything - a renderer, an artifact info gatherer, a pretty printer, etc. There is no implementation implied in the design. Each renderer would implement ScreenWidgetVisitor. The code inside each model widget renderWidgetString method would be moved to the corresponding visit method in the renderer. Likewise, the artifact-info-gatherer would implement ScreenWidgetVisitor. The existing model widget artifact-info-gathering code would be moved to the corresponding visit method in the artifact-info-gatherer. Once those changes are made, the model widgets will contain only object construction code. Even that code could be moved to a builder class that implements ScreenWidgetVisitor. The benefit of this change will be improved separation of concerns. Right now the screen widget model classes try to be everything to everybody. After the change they will be little more than data structures that various behavior-specific objects will refer to. Rendering code is not mixed in with object construction code or with artifact-gathering code. The API is simplified. A change like this could have serious downstream consequences for OFBiz users who have modified the screen widget model classes. One approach we could take is to put the visit methods in ScreenStringRenderer (instead of having a separate interface) and then deprecate the existing renderXxxx methods. The downside is code maintainance - bug fixes and new features will have to be applied to both patterns. If you believe the change is worthwhile, then please vote on this issue.

            People

            • Assignee:
              Unassigned
              Reporter:
              Adrian Crum
            • Votes:
              2 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:

                Development