> On Aug. 23, 2012, 1:12 a.m., Mona Chitnis wrote: > > 1. @author tags in public class javadoc can be removed. I think the new > > pre-commit build yells out -1 if it encounters them > > > > 2. initPropertyTable() from the derived NodeWidget classes, can be defined > > upstream in super class NodeWidget and overridden in derived class when > > behavior is different from default. Same way for 'generateXml()' instead of > > being abstract in super class. > > > > 3. Along with #2, NodeWidget Derived class constructors can be reduced to a > > single liner like e.g. > > > > super(gen, "oozie-EmailActionWidget"); > > > > with superclass constructor doing the string assignment. (correct me if > > wrong) > > > > 4. *Nitpicky* Maintain consistency in the constructor argument - public > > EmailActionWidget(OozieWorkflowGenerator gen). At one place it is 'g' and > > others 'gen'. > > > > Kudos to managing such a huge amount of new code! Great work!
Thank you for review and comments, Mona. I found all of them good points, so accommodated them. soon to upload modified patch after final test. >1. @author tags yes modified >2. initPropertyTable agree. fixed. one limitation of GWT is that we cannot use Reflection. thus, cannot do something like "child class passing corresponding property table class as argument, and parent class instantiates it using newInstance()" so child class has to pass some indicator (string/enum type/instance), and in some place, we have to provide big if-block to map the indicator to corresponding property table class. to this end, I added new factory class(PropertyTableFactory) taking care of the mapping and instantiating right property table class. also made it singleton. >3. Along with #2, NodeWidget Derived class constructors can be reduced to a >single liner like e.g. modified as instructed. >4. *Nitpicky* Maintain consistency in the constructor argument fixed >Kudos to managing such a huge amount of new code! Great work! Thanks! - Ryota ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6435/#review10654 ----------------------------------------------------------- On Aug. 22, 2012, 6:23 p.m., Ryota Egashira wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6435/ > ----------------------------------------------------------- > > (Updated Aug. 22, 2012, 6:23 p.m.) > > > Review request for oozie. > > > Description > ------- > > https://issues.apache.org/jira/browse/OOZIE-944 > Please note this is a big patch including 71 files total. > 38 java files, 9 config files (maven related files/GWT related XML/readme, > etc), 24 images files (since this is UI tool. should be excluded for review) > > > This addresses bug OOZIE-944. > https://issues.apache.org/jira/browse/OOZIE-944 > > > Diffs > ----- > > trunk/workflowgenerator/README.txt PRE-CREATION > trunk/workflowgenerator/pom.xml PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/OozieDiagramController.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/OozieWorkflowGenerator.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/Property.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/PropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/action/EmailPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/action/FSPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/action/JavaPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/action/MapReducePropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/action/PigPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/action/PipesPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/action/SSHPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/action/ShellPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/action/StreamingPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/action/SubWFPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/control/DecisionPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/control/EndPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/control/ForkPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/control/JoinPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/control/KillPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/control/StartPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/property/control/WrkflowPropertyTable.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/NodeWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/action/EmailActionWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/action/FSActionWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/action/JavaActionWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/action/MapReduceActionWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/action/PigActionWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/action/PipesActionWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/action/SSHActionWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/action/ShellActionWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/action/StreamingActionWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/action/SubWFActionWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/control/DecisionNodeWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/control/EndNodeWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/control/ForkNodeWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/control/JoinNodeWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/control/KillNodeWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/client/widget/control/StartNodeWidget.java > PRE-CREATION > > trunk/workflowgenerator/src/main/java/org/apache/oozie/tools/workflowgenerator/workflowgenerator.gwt.xml > PRE-CREATION > trunk/workflowgenerator/src/main/resources/img/action.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/add-btn.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/decision.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/del-btn.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/distcp-action.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/email-action.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/end.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/fork-shape.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/fork.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/fs-action.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/hive-action.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/java-action.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/join-shape.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/join.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/kill.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/mr-action.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/pig-action.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/pipes-action.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/shell-action.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/ssh-action.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/start-shape.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/start.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/streaming-action.png UNKNOWN > trunk/workflowgenerator/src/main/resources/img/subwf-action.png UNKNOWN > > trunk/workflowgenerator/src/main/resources/org/apache/oozie/tools/workflowgenerator/workflowgenerator.gwt.xml > PRE-CREATION > trunk/workflowgenerator/src/main/webapp/WEB-INF/web.xml PRE-CREATION > trunk/workflowgenerator/src/main/webapp/workflowgenerator.css PRE-CREATION > trunk/workflowgenerator/src/main/webapp/workflowgenerator.html PRE-CREATION > trunk/workflowgenerator/workflowgeneratorTest-dev.launch PRE-CREATION > trunk/workflowgenerator/workflowgeneratorTest-prod.launch PRE-CREATION > > Diff: https://reviews.apache.org/r/6435/diff/ > > > Testing > ------- > > locally run and check UI components on browser > > > Thanks, > > Ryota Egashira > >
