Details

    • Type: Task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.4
    • Security Level: Public (Public: Anyone can view)
    • Labels:
      None
    • Epic Link:
    • Rice Module:
      KRAD
    • Sprint:
      2.4.0-m2 Sprint 2
    • KAI Review Status:
      Not Required
    • KTI Review Status:
      Not Required
    • Code Review Status:
      Not Required
    • Include in Release Notes?:
      Yes

      Description

      • Properties missing from the copy method, all properties should be copied (there is one exception I have found, viewIndex on View should not be copied)
      • Copy properties method does not call super
      • Nested components are copied without calling .copy
      • Some classes missing copy properties method (ex ‘GroupValidationMessages’). Verify all classes that implement Component and LayoutManager have this method implemented
      • Change added setters to protected, move to correct location, and add javadoc (see message I sent earlier)
      • Scope issues, for example the following code:

      if(addLineActions != null) {
      List<Action> addLineActions = Lists.newArrayListWithExpectedSize(getAddLineActions().size());
      for (Action addLineAction : this.addLineActions)

      { addLineActions.add((Action)addLineAction.copy()); }

      }

      collectionGroupCopy.setAddLineActions(addLineActions);

      Notice the assignment is outside the declaration block of the local addLineActions, so it picks up the global addLineActions for the setter which causes no change. I think it is best to use a different variable name for the local variable so there is no confusion, like this:

      if(addLineActions != null) {
      List<Action> addLineActionsCopy = ists.newArrayListWithExpectedSize(getAddLineActions().size());
      for (Action addLineAction : this.addLineActions)

      { addLineActionsCopy.add((Action)addLineAction.copy()); }

      collectionGroupCopy.setAddLineActions(addLineActionsCopy);
      }

      One way I would recommend to review the methods, is right click on the tab for class and select ‘Split Vertically’. The will give you another editor tab for the same class to the right, you can then scroll up to see the fields listed in the class and compare to the statements in copyProperties. A good practice would be to make the setter calls in the same order as the fields are declared, which would make it easier to determine if something is missing.

      That is all we need for 2.3, and I will fill more comfortable with it. Please update first because I have already fixed several issues.

      After 2.3, I would like to do some further cleanup. One issue is our copy method with the generics is not working as planned. One of the goals of using the generics is so you don’t have to cast the type for each setter. For example instead of having to do this:

      groupCopy.setDisclosure((Disclosure) this.disclosure.copy());

      We want to be able to just do this:

      groupCopy.setDisclosure(this.disclosure.copy());

      Other cleanup (post 2.3):

      • Only need one public <T> T copy() { per hierarchy. Since this is in DictionaryBeanBase, not needed in any subclass (Component, LayoutManager, etc).

      • Create Copyable interface in org.kuali.rice.krad.datadictionary. Make DictionaryBeanBase an other objects with the copy method (not subclasses) implement

      • ‘replacement’ property on PropertyReplacer needs to be coped. Could be a component, List of components, Map of components, or Layout manager. Need to check for interface

      • Create helper methods in ComponentUtils for copying a List of Copyables and Map of Copyables so logic does not need to be duplicated everywhere, then change statements in copy properties methods to use the new helper methods

      • For nested objects that we are setting directly (not calling copy), you don’t need the null check before (just can set the value as null). This will shorten our copy methods a bit

      • Copy properties method does not use @see comment consistently, and sometimes method is after validate method

      • Formatting wise I would rather see these lines broken up:

      super.copyProperties(component);
      ContainerBase containerBaseCopy = (ContainerBase) component;
      containerBaseCopy.setDefaultItemPosition(this.defaultItemPosition);

      to

      super.copyProperties(component);
      // blank line
      ContainerBase containerBaseCopy = (ContainerBase) component;
      // blank line
      containerBaseCopy.setDefaultItemPosition(this.defaultItemPosition);

      • Remove use of Google Util lists
      • Changed addded setter methods to use protected method

        Attachments

          Activity

          There are no comments yet on this issue.

            People

            • Assignee:
              matthew.wuertz Matthew Wuertz (Inactive)
              Reporter:
              jkneal Jerry Neal (Inactive)
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 2 days, 4 hours
                2d 4h
                Remaining:
                Remaining Estimate - 2 days, 4 hours
                2d 4h
                Logged:
                Time Spent - Not Specified
                Not Specified