anmolbabu has posted comments on this change. Change subject: webadmin : CheckBoxGroup, DaysOfMonthSelector and DateTimeBox Widgets ......................................................................
Patch Set 8: (45 comments) https://gerrit.ovirt.org/#/c/37302/8/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/CheckBoxGroup.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/CheckBoxGroup.java: Line 26: import com.google.gwt.user.client.ui.FlexTable; Line 27: import com.google.gwt.user.client.ui.FlowPanel; Line 28: import com.google.gwt.user.client.ui.HasConstrainedValue; Line 29: Line 30: public class CheckBoxGroup extends Composite implements TakesValue<String>, HasConstrainedValue<String> { > Javadoc please Done Line 31: private final Map<String, CheckBox> checkBoxes = new LinkedHashMap<>(); Line 32: Line 33: Line 34: private final Map<String, FlowPanel> panels = new LinkedHashMap<String, FlowPanel>(); Line 31: private final Map<String, CheckBox> checkBoxes = new LinkedHashMap<>(); Line 32: Line 33: Line 34: private final Map<String, FlowPanel> panels = new LinkedHashMap<String, FlowPanel>(); Line 35: private final FlexTable weekDaysTable = new FlexTable(); > change weekDaysTable to something more generic Done Line 36: private final FlowPanel wrapperPanel = new FlowPanel(); Line 37: int currentCount = 0; Line 38: Line 39: private boolean enabled = true; Line 73: } Line 74: Line 75: @Override Line 76: public void onRemoveAll() { Line 77: Iterator<Entry<String, CheckBox>> iterator = checkBoxes.entrySet().iterator(); > did you mean to leave out populateSelectedItemsString() here? nope I missed it Line 78: while (iterator.hasNext()) { Line 79: Entry<String, CheckBox> mapEntry = iterator.next(); Line 80: mapEntry.getValue().setValue(false); Line 81: } Line 90: } Line 91: }; Line 92: } Line 93: Line 94: public void addCheckBoxOptions(String checkBoxLabel) { > please rename "addCheckBox" Done Line 95: if (checkBoxLabel == null) { Line 96: throw new IllegalArgumentException("null value is not permited"); //$NON-NLS-1$ Line 97: } Line 98: if (checkBoxes.containsKey(checkBoxLabel)) { Line 105: fPanel.add(newCheckBox); Line 106: panels.put(checkBoxLabel, fPanel); Line 107: } Line 108: Line 109: private CheckBox getNewCheckBox(String checkBoxLabel) { > consider renaming "buildCheckbox" -- "get" in Java is usually reserved for Done Line 110: final CheckBox newCheckBox = new CheckBox(SafeHtmlUtils.fromTrustedString(checkBoxLabel)); Line 111: newCheckBox.setValue(false); Line 112: newCheckBox.setWidth("30px");//$NON-NLS-1$ Line 113: newCheckBox.addValueChangeHandler(new ValueChangeHandler<Boolean>() { Line 110: final > do we know this is a trusted String? Done Line 108: Line 109: private CheckBox getNewCheckBox(String checkBoxLabel) { Line 110: final CheckBox newCheckBox = new CheckBox(SafeHtmlUtils.fromTrustedString(checkBoxLabel)); Line 111: newCheckBox.setValue(false); Line 112: newCheckBox.setWidth("30px");//$NON-NLS-1$ > can this be done with a CSS class instead? Done Line 113: newCheckBox.addValueChangeHandler(new ValueChangeHandler<Boolean>() { Line 114: @Override Line 115: public void onValueChange(ValueChangeEvent<Boolean> event) { Line 116: if (newCheckBox.getValue()) { Line 119: selectedItems.remove(newCheckBox.getText()); Line 120: } Line 121: } Line 122: }); Line 123: newCheckBox.getElement().getStyle().setMarginRight(30, Unit.PX); > can this be done with a CSS class instead? Done Line 124: return newCheckBox; Line 125: } Line 126: Line 127: private void populateSelectedItemsString() { Line 123: newCheckBox.getElement().getStyle().setMarginRight(30, Unit.PX); Line 124: return newCheckBox; Line 125: } Line 126: Line 127: private void populateSelectedItemsString() { > suggest renaming "updateValue" Done Line 128: selectedItemsString = null; Line 129: for(String item : selectedItems) { Line 130: if(selectedItemsString == null) { Line 131: selectedItemsString = item.toString(); Line 185: setValue(value, false); Line 186: } Line 187: Line 188: @Override Line 189: public void setValue(String value, boolean fireEvents) { > if fireEvents is true, you're supposed to fire a ValueChangeEvent. That app Done Line 190: boolean newValue = false; Line 191: if (value == null) { Line 192: return; Line 193: } Line 190: boolean newValue = false; Line 191: if (value == null) { Line 192: return; Line 193: } Line 194: List<String> values = getItemsToSelect(value); > change to Done Line 195: if (selectedItemsString == null || selectedItems.size() == values.size() && selectedItems.containsAll(values)) { Line 196: return; Line 197: } else { Line 198: for (String item : values) { Line 191: if (value == null) { Line 192: return; Line 193: } Line 194: List<String> values = getItemsToSelect(value); Line 195: if (selectedItemsString == null || selectedItems.size() == values.size() && selectedItems.containsAll(values)) { > please use extra parentheses to make this a little clearer Done Line 196: return; Line 197: } else { Line 198: for (String item : values) { Line 199: if (!checkBoxes.containsKey(item)) { Line 206: } Line 207: } Line 208: } Line 209: Line 210: private List<String> getItemsToSelect(String value) { > not needed. delete. Done Line 211: List<String> values = new ArrayList<>(); Line 212: if (value != null) { Line 213: for (String item : value.split(",")) {//$NON-NLS-1$ Line 214: values.add(item); Line 217: return values; Line 218: } Line 219: Line 220: @Override Line 221: public void setAcceptableValues(Collection<String> values) { > javadoc? not quite sure what this method is doing. Done Line 222: panels.clear(); Line 223: wrapperPanel.clear(); Line 224: if (values != null) { Line 225: for (final String value : values) { Line 234: 4 > if (column == NUM_COLUMNS) Done Line 262: public int getTabIndex() { Line 263: return tabIndex; Line 264: } Line 265: Line 266: public void setAccessKey(char key) { > remove? This is required to be overriden by classes extending EditorWidget(precisely speaking Focusable). And this is required for my model bound version of CheckBoxGroup which is ListModelCheckBoxGroup extends CheckBoxGroup. I'll move this to ListModelCheckBoxGroup extends CheckBoxGroup. This is actually the shortcut key used to make selections/deselections from the widget(if I understand it correctly).But not sure what that can be and how it can be used for this widget.So it remained a unseful method here. Please provide your suggestions. Line 267: } Line 268: Line 269: public void setFocus(boolean focused) { Line 270: } Line 265: Line 266: public void setAccessKey(char key) { Line 267: } Line 268: Line 269: public void setFocus(boolean focused) { > remove? This is required to be overriden by classes extending EditorWidget(precisely speaking Focusable). And this is required for my model bound version of CheckBoxGroup which is ListModelCheckBoxGroup extends CheckBoxGroup. I'll move this to ListModelCheckBoxGroup extends CheckBoxGroup. Kindly provide your suggestions Line 270: } Line 271: Line 272: public void setTabIndex(int index) { Line 273: tabIndex = index; https://gerrit.ovirt.org/#/c/37302/8/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/DateTimeBox.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/DateTimeBox.java: Line 19: import com.google.gwt.user.client.ui.HasConstrainedValue; Line 20: import com.google.gwt.user.client.ui.ValueListBox; Line 21: import com.google.gwt.user.datepicker.client.DateBox; Line 22: Line 23: public class DateTimeBox extends Composite implements TakesValue<Date>, HasConstrainedValue<Date> { > Javadoc please Done Line 24: Line 25: private static final int HOUR_CONSTANT = 12; Line 26: private static final int MINUTE_MAX = 59; Line 27: private static final String MORNING = "AM";//$NON-NLS-1$ Line 99: return addHandler(handler, ValueChangeEvent.getType()); Line 100: } Line 101: Line 102: @Override Line 103: public void setAcceptableValues(Collection<Date> values) { > missing implementation? or put a comment that all values are acceptable Nope. This is not of use to me here as it can get only operate on date and any date.So, removing it from my code. Line 104: Line 105: } Line 106: Line 107: @Override Line 127: } Line 128: Line 129: private void placeDateBox() { Line 130: containerTable.setWidget(constantRowIndex, currentColumnMax, dateBox); Line 131: containerTable.getColumnFormatter().setWidth(currentColumnMax, dateRequired ? "100px" : "0px");//$NON-NLS-1$//$NON-NLS-2$ > can we use CSS? Done Line 132: currentColumnMax++; Line 133: } Line 134: Line 135: private void initDateBox() { Line 134: Line 135: private void initDateBox() { Line 136: dateBox = new DateBox(); Line 137: dateBox.getDatePicker().addStyleName("dateBoxPopup");//$NON-NLS-1$ Line 138: dateBox.setWidth(dateRequired ? "90px" : "0px");//$NON-NLS-1$//$NON-NLS-2$ > can we use CSS? Done Line 139: dateBox.setFormat(new DateBox.DefaultFormat(DateTimeFormat.getFormat("dd-MMM-yyyy")));//$NON-NLS-1$ Line 140: dateBox.addValueChangeHandler(new ValueChangeHandler<Date>() { Line 141: @Override Line 142: public void onValueChange(ValueChangeEvent<Date> event) { Line 135: private void initDateBox() { Line 136: dateBox = new DateBox(); Line 137: dateBox.getDatePicker().addStyleName("dateBoxPopup");//$NON-NLS-1$ Line 138: dateBox.setWidth(dateRequired ? "90px" : "0px");//$NON-NLS-1$//$NON-NLS-2$ Line 139: dateBox.setFormat(new DateBox.DefaultFormat(DateTimeFormat.getFormat("dd-MMM-yyyy")));//$NON-NLS-1$ > use a constant? I'll contain this in a variable so that the flexibility of setting the user defined format can be provided. Line 140: dateBox.addValueChangeHandler(new ValueChangeHandler<Date>() { Line 141: @Override Line 142: public void onValueChange(ValueChangeEvent<Date> event) { Line 143: selectedDate = event.getValue(); Line 171: currentColumnMax++; Line 172: } Line 173: Line 174: private void initTimeBoxes() { Line 175: hoursBox = initIntegerValueBox(formStepValues(hourSteps, 1, 12)); > you have HOUR_CONSTANT defined about, but not using it Done Line 176: addHourSelectionListener(); Line 177: Line 178: minutesBox = initIntegerValueBox(formStepValues(minuteSteps, 0, 59)); Line 179: addMinuteSelectionListener(); Line 174: private void initTimeBoxes() { Line 175: hoursBox = initIntegerValueBox(formStepValues(hourSteps, 1, 12)); Line 176: addHourSelectionListener(); Line 177: Line 178: minutesBox = initIntegerValueBox(formStepValues(minuteSteps, 0, 59)); > you have MINUTE_MAX defined about, but not using it Done Line 179: addMinuteSelectionListener(); Line 180: Line 181: amPmBox = initStringValueBox(new ArrayList<String>(Arrays.asList(MORNING, EVENING))); Line 182: addAmPmSelectionListener(); https://gerrit.ovirt.org/#/c/37302/8/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/DaysOfMonthSelector.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/DaysOfMonthSelector.java: Line 21: import com.google.gwt.user.client.ui.HTMLTable.Cell; Line 22: import com.google.gwt.user.client.ui.HasConstrainedValue; Line 23: import com.google.gwt.user.client.ui.Label; Line 24: Line 25: public class DaysOfMonthSelector extends Composite implements TakesValue<String>, HasConstrainedValue<String> { > Javadoc please Done Line 26: Line 27: private static final int DAYS_IN_WEEK = 7; Line 28: Line 29: private static final UIConstants constants = ConstantsManager.getInstance().getConstants(); Line 39: Line 40: public DaysOfMonthSelector() { Line 41: selectedItems = initSelectionList(); Line 42: initWidget(wrapperPanel); Line 43: daysOfMonth.setBorderWidth(2); > can we use CSS? Done Line 44: showDaysOfMonth(); Line 45: daysOfMonth.addClickHandler(new ClickHandler() { Line 46: @Override Line 47: public void onClick(ClickEvent event) { Line 121: } Line 122: Line 123: private int getRowForTheDay(int date) { Line 124: int row = date / DAYS_IN_WEEK; Line 125: return date % 7 == 0 ? row - 1 : row; > DAYS_IN_WEEK Done Line 126: } Line 127: Line 128: private int getColumnForTheDay(int date) { Line 129: int probableColumn = date % DAYS_IN_WEEK; Line 173: } Line 174: Line 175: @Override Line 176: public void setValue(String value) { Line 177: setValue(value != null ? value.replace("L", "32") : value, false);//$NON-NLS-1$//$NON-NLS-2$\ > not using LAST_DAY ? 'L' is returned instead of 32. Basically, some months are of 31 days and some have 30 and feb has 27 days. So, if a event needs to happen on last day of every month then this comes in to be of use. For instance in case of GlusterVolumeSnapshots, if a snaap is to be scheduled to be taken once every last day of month, that's where this comes into play. The quartz accepts an expression 'L' for indicating last day of month Line 178: } Line 179: Line 180: @Override Line 181: public String getValue() { Line 178: } Line 179: Line 180: @Override Line 181: public String getValue() { Line 182: return selectedItemsString != null ? selectedItemsString.replace("32", "L") : selectedItemsString;//$NON-NLS-1$//$NON-NLS-2$ > not using LAST_DAY? what is L? Same as above.I'll replace 32 and use LAST_DAY in both places Line 183: } Line 184: Line 185: private void setSelectedValue() { Line 186: selectedItemsString = null; Line 194: } Line 195: ValueChangeEvent.fire(this, getValue()); Line 196: } Line 197: Line 198: public void setEnabled(boolean enabled) { > delete, or comment why not implmented Done Line 199: Line 200: } https://gerrit.ovirt.org/#/c/37302/8/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/EntityModelDateTimeBox.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/EntityModelDateTimeBox.java: Line 11: import com.google.gwt.event.dom.client.KeyUpHandler; Line 12: import com.google.gwt.event.shared.HandlerRegistration; Line 13: import com.google.gwt.user.client.ui.HasConstrainedValue; Line 14: Line 15: public class EntityModelDateTimeBox extends DateTimeBox implements EditorWidget<Date, TakesValueEditor<Date>>, HasConstrainedValue<Date>{ > Javadoc please Done Line 16: Line 17: private TakesConstrainedValueEditor<Date> editor; Line 18: Line 19: private boolean enabled; Line 60: public void setFocus(boolean focused) { Line 61: } Line 62: Line 63: @Override Line 64: public void setTabIndex(int index) { > please store the tab index and return it from the getter Done Line 65: } Line 66: Line 67: @Override Line 68: public TakesValueEditor<Date> asEditor() { https://gerrit.ovirt.org/#/c/37302/8/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/EntityModelDateTimeBoxEditor.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/EntityModelDateTimeBoxEditor.java: Line 7: Line 8: import com.google.gwt.dom.client.Style.BorderStyle; Line 9: import com.google.gwt.editor.client.IsEditor; Line 10: Line 11: public class EntityModelDateTimeBoxEditor extends AbstractValidatedWidgetWithLabel<Date, EntityModelDateTimeBox> implements IsEditor<WidgetWithLabelEditor<Date, EntityModelDateTimeBoxEditor>>{ > Javadoc please Done Line 12: Line 13: private final WidgetWithLabelEditor<Date, EntityModelDateTimeBoxEditor> editor; Line 14: Line 15: public EntityModelDateTimeBoxEditor() { Line 32: Line 33: @Override Line 34: public void markAsValid() { Line 35: super.markAsValid(); Line 36: getValidatedWidgetStyle().setBorderStyle(BorderStyle.NONE); > can we use CSS? If masrkAsInvalid is done and then markAsValid then not sure if it takes care of this Line 37: } Line 38: Line 39: @Override Line 40: public void markAsInvalid(List<String> validationHints) { Line 38: Line 39: @Override Line 40: public void markAsInvalid(List<String> validationHints) { Line 41: super.markAsInvalid(validationHints); Line 42: getValidatedWidgetStyle().setBorderStyle(BorderStyle.SOLID); > can we use CSS? Same as above Line 43: } Line 44: https://gerrit.ovirt.org/#/c/37302/8/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListModelCheckBoxGroup.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListModelCheckBoxGroup.java: Line 2: Line 3: import com.google.gwt.editor.client.adapters.TakesValueEditor; Line 4: import com.google.gwt.user.client.ui.HasConstrainedValue; Line 5: Line 6: public class ListModelCheckBoxGroup extends CheckBoxGroup implements EditorWidget<String, TakesValueEditor<String>>, HasConstrainedValue<String> { > Javadoc please Done Line 7: Line 8: private TakesConstrainedValueEditor<String> editor; Line 9: Line 10: public ListModelCheckBoxGroup() { https://gerrit.ovirt.org/#/c/37302/8/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListModelCheckBoxGroupEditor.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListModelCheckBoxGroupEditor.java: Line 7: Line 8: import com.google.gwt.dom.client.Style.BorderStyle; Line 9: import com.google.gwt.editor.client.IsEditor; Line 10: Line 11: public class ListModelCheckBoxGroupEditor extends AbstractValidatedWidgetWithLabel<String, ListModelCheckBoxGroup> implements IsEditor<WidgetWithLabelEditor<String, ListModelCheckBoxGroupEditor>>{ > Javadoc please Done Line 12: Line 13: private final WidgetWithLabelEditor<String, ListModelCheckBoxGroupEditor> editor; Line 14: Line 15: public ListModelCheckBoxGroupEditor() { Line 23: Line 24: @Override Line 25: public void markAsValid() { Line 26: super.markAsValid(); Line 27: getValidatedWidgetStyle().setBorderStyle(BorderStyle.NONE); > can we use CSS? Same as before Line 28: } Line 29: Line 30: @Override Line 31: public void markAsInvalid(List<String> validationHints) { Line 29: Line 30: @Override Line 31: public void markAsInvalid(List<String> validationHints) { Line 32: super.markAsInvalid(validationHints); Line 33: getValidatedWidgetStyle().setBorderStyle(BorderStyle.SOLID); > can we use CSS? Same as before Line 34: } Line 35: Line 36: @Override Line 37: public WidgetWithLabelEditor<String, ListModelCheckBoxGroupEditor> asEditor() { https://gerrit.ovirt.org/#/c/37302/8/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListModelDaysOfMonthSelector.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListModelDaysOfMonthSelector.java: Line 9: import com.google.gwt.event.dom.client.KeyUpHandler; Line 10: import com.google.gwt.event.shared.HandlerRegistration; Line 11: import com.google.gwt.user.client.ui.HasConstrainedValue; Line 12: Line 13: public class ListModelDaysOfMonthSelector extends DaysOfMonthSelector implements EditorWidget<String, TakesValueEditor<String>>, HasConstrainedValue<String> { > Javadoc please Done Line 14: Line 15: private TakesConstrainedValueEditor<String> editor; Line 16: Line 17: boolean enabled; Line 52: } Line 53: Line 54: @Override Line 55: public void setTabIndex(int index) { Line 56: > please save the value and return from getter Done Line 57: } Line 58: Line 59: @Override Line 60: public TakesValueEditor<String> asEditor() { https://gerrit.ovirt.org/#/c/37302/8/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListModelDaysOfMonthSelectorEditor.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListModelDaysOfMonthSelectorEditor.java: Line 28: Line 29: @Override Line 30: public void markAsValid() { Line 31: super.markAsValid(); Line 32: getValidatedWidgetStyle().setBorderStyle(BorderStyle.NONE); > can we use CSS? Same as before Line 33: } Line 34: Line 35: @Override Line 36: public void markAsInvalid(List<String> validationHints) { Line 34: Line 35: @Override Line 36: public void markAsInvalid(List<String> validationHints) { Line 37: super.markAsInvalid(validationHints); Line 38: getValidatedWidgetStyle().setBorderStyle(BorderStyle.SOLID); > can we use CSS? Same as before Line 39: } Line 40: https://gerrit.ovirt.org/#/c/37302/8/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListenableOrderedSet.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListenableOrderedSet.java: Line 1: package org.ovirt.engine.ui.common.widget.editor; Line 2: Line 3: import java.util.Collection; Line 4: import java.util.LinkedHashSet; Line 5: > I think this should be renamed "ObservableOrderedSet" Done Line 6: public abstract class ListenableOrderedSet<T> extends LinkedHashSet<T> { Line 7: Line 8: private static final long serialVersionUID = 1L; Line 9: Line 2: Line 3: import java.util.Collection; Line 4: import java.util.LinkedHashSet; Line 5: Line 6: public abstract class ListenableOrderedSet<T> extends LinkedHashSet<T> { > Javadoc please. Done Line 7: Line 8: private static final long serialVersionUID = 1L; Line 9: Line 10: public abstract void onAdd(T item); -- To view, visit https://gerrit.ovirt.org/37302 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I38daa0d2c151eb0e34603488496a8a1ea4719c87 Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
