Greg Sheremeta has posted comments on this change. Change subject: webadmin : CheckBoxGroup, DaysOfMonthSelector and DateTimeBox Widgets ......................................................................
Patch Set 8: (46 comments) http://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 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 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? 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" 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 dumb getters 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? 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? 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? 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" 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 appears to be missing. Or, if there's a reason fireEvents isn't used, you can combine this method with the method above. 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 List<String> values = Arrays.asList(value.split(",")); 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 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. 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. 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) ... private static final int NUM_COLUMNS = 4; Line 262: public int getTabIndex() { Line 263: return tabIndex; Line 264: } Line 265: Line 266: public void setAccessKey(char key) { remove? 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? Line 270: } Line 271: Line 272: public void setTabIndex(int index) { Line 273: tabIndex = index; http://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 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 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? 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? 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? 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 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 Line 179: addMinuteSelectionListener(); Line 180: Line 181: amPmBox = initStringValueBox(new ArrayList<String>(Arrays.asList(MORNING, EVENING))); Line 182: addAmPmSelectionListener(); http://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 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? 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 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 ? what is L? 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? 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 Line 199: Line 200: } http://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 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 Line 65: } Line 66: Line 67: @Override Line 68: public TakesValueEditor<Date> asEditor() { http://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 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? 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? Line 43: } Line 44: http://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 Line 7: Line 8: private TakesConstrainedValueEditor<String> editor; Line 9: Line 10: public ListModelCheckBoxGroup() { http://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 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? 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? Line 34: } Line 35: Line 36: @Override Line 37: public WidgetWithLabelEditor<String, ListModelCheckBoxGroupEditor> asEditor() { http://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 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 Line 57: } Line 58: Line 59: @Override Line 60: public TakesValueEditor<String> asEditor() { http://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? 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? Line 39: } Line 40: http://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: What you are doing here makes sense. It's almost Observer pattern. Have you considered using Observer more formally? That would involve this Set making calls to CheckBoxGroup. Just a thought. 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 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" "Listenable" has some ambiguity, and I don't think it's a real word :) 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. Line 7: Line 8: private static final long serialVersionUID = 1L; Line 9: Line 10: public abstract void onAdd(T item); -- To view, visit http://gerrit.ovirt.org/37302 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I38daa0d2c151eb0e34603488496a8a1ea4719c87 Gerrit-PatchSet: 8 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Greg Sheremeta <[email protected]> Gerrit-Reviewer: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: anmolbabu <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
