Alexander Wels has posted comments on this change. Change subject: webadmin : CheckBoxGroup, DaysOfMonthSelector and DateTimeBox Widgets ......................................................................
Patch Set 10: (13 comments) https://gerrit.ovirt.org/#/c/37302/10/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 35: So I have several issues with this class in no particular order: 1. Why are we using a flex table to essentially lay out X number of check boxes. Wouldn't it make much more sense to simply 'float: left' all the check boxes that we add and have the width of the panel determine how many check boxes show up on the same row. 2. We are using the labels of the check boxes as the values returned from this widget. What happens if we translate the widget to a different language, then the values will change as well. When building the check boxes we need to pass in label/value pairs. One is what we show the user and the other is the internal representation of that label. That way the values don't change based on locale. 3. I am trying to understand the need to use an ObservableOrderedSet. I don't see any use for it, and it is needlessly complicating things. I think it would be much simpler to have the following datastructure: private final Map<String, CheckBox> checkBoxes = new HashMap<>(); The KEY of the map will be the VALUE, when you create the checkbox, you pass the localized value so that is what will be displayed. The checkboxes will keep in their internal state if they are selected or not. When one calls getValue() you can simply iterate over the map like this: public String getValue() { String result = ""; for (Map.Entry<String, CheckBox> entry : checkBoxes.entrySet()) { if (entry.getValue().isChecked()) { if (!result.isEmpty()) { result += ","; //$NON-NLS-1$ } result += entry.getKey(); } } return value; } public void setValue(String key) { if (checkBoxes.get(key) != null) { checkBoxes.get(key).setChecked(); } } public void setAcceptableValues(List<Pair<String, String>> values) { for (Pair<String, String> valueLabelPair: values) { CheckBox checkBox = buildCheckBox(valueLabelPair.second()); wrapperPanel.add(checkBox); checkBoxes.put(valueLabelPair.first(), checkBox); } } Line 84: Can't you just call setEnabled(false) here instead of duplicating essentially the same code? Line 111: fPanel.add(newCheckBox); Line 112: panels.put(checkBoxLabel, fPanel); Line 113: } Line 114: Line 115: private CheckBox buildCheckbox(String checkBoxLabel) { Before calling SafeHtmlUtils.fromTrustedString, you have to actually make sure the string passed in safe. Line 116: final CheckBox newCheckBox = new CheckBox(SafeHtmlUtils.fromTrustedString(checkBoxLabel)); Line 117: newCheckBox.setValue(false); Line 118: newCheckBox.setStyleName("checkBoxWidth");//$NON-NLS-1$ Line 119: newCheckBox.addValueChangeHandler(new ValueChangeHandler<Boolean>() { Line 118: newCheckBox.setStyleName("checkBoxWidth");//$NON-NLS-1$ Line 119: newCheckBox.addValueChangeHandler(new ValueChangeHandler<Boolean>() { Line 120: @Override Line 121: public void onValueChange(ValueChangeEvent<Boolean> event) { Line 122: if (newCheckBox.getValue()) { If you take my suggestions above, you can notify the same listeners to your ObservableSortedSet here. Line 123: selectedItems.add(newCheckBox.getText()); Line 124: } else if (selectedItems.contains(newCheckBox.getText())) { Line 125: selectedItems.remove(newCheckBox.getText()); Line 126: } Line 278: The better way to loop over a map: for (Map.Entry<String, CheckBox> entry : checkBoxes.entrySet()) { entry.getValue().setEnabled(false); } https://gerrit.ovirt.org/#/c/37302/10/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 18: import com.google.gwt.user.client.ui.FlowPanel; Line 19: import com.google.gwt.user.client.ui.HasValue; Line 20: import com.google.gwt.user.client.ui.ValueListBox; Line 21: import com.google.gwt.user.datepicker.client.DateBox; Line 22: I don't quite understand the point of this widget. I don't have the screen shot of what you are trying to replicate anymore can you share it with me again? Line 23: /** Line 24: * The DateTimeBox allows the user to select a date and a time.<br> Line 25: * It currently utilises the gwt DateBox for the date component and has the following 3 dropdowns<br> Line 26: * 1. Hours component of time.<br> Line 24: * The DateTimeBox allows the user to select a date and a time.<br> Line 25: * It currently utilises the gwt DateBox for the date component and has the following 3 dropdowns<br> Line 26: * 1. Hours component of time.<br> Line 27: * 2. Minutes component of time.<br> Line 28: * 3. Am/Pm (This widget hence assumes 12hr format on display to user and 24 hour format while returning the selected This is a flawed concept, many locales don't have the AM/PM notion at all, so assuming it should be displayed like that is wrong. Line 29: * date object)<br> Line 30: */ Line 31: Line 32: public class DateTimeBox extends Composite implements TakesValue<Date>, HasValue<Date> { Line 27: * 2. Minutes component of time.<br> Line 28: * 3. Am/Pm (This widget hence assumes 12hr format on display to user and 24 hour format while returning the selected Line 29: * date object)<br> Line 30: */ Line 31: Why not use the GWT bootstrap date time picker which does basically everything we want here already. http://gwtbootstrap3.github.io/gwtbootstrap3-demo/#dateTimePicker Line 32: public class DateTimeBox extends Composite implements TakesValue<Date>, HasValue<Date> { Line 33: Line 34: private static final int HOUR_CONSTANT = 12; Line 35: private static final int MINUTE_MAX = 59; https://gerrit.ovirt.org/#/c/37302/10/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 41: Line 42: @Override Line 43: public void markAsInvalid(List<String> validationHints) { Line 44: super.markAsInvalid(validationHints); Line 45: getValidatedWidgetStyle().setBorderStyle(BorderStyle.SOLID); The parent already sets the border style to solid, so no need to do it here again. Line 46: } Line 47: https://gerrit.ovirt.org/#/c/37302/10/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 33: Line 34: @Override Line 35: public void markAsInvalid(List<String> validationHints) { Line 36: super.markAsInvalid(validationHints); Line 37: getValidatedWidgetStyle().setBorderStyle(BorderStyle.SOLID); The parent already sets the border style to solid, so no need to do it here again. Line 38: } Line 39: Line 40: @Override Line 41: public WidgetWithLabelEditor<String, ListModelCheckBoxGroupEditor> asEditor() { https://gerrit.ovirt.org/#/c/37302/10/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 38: Line 39: @Override Line 40: public void markAsInvalid(List<String> validationHints) { Line 41: super.markAsInvalid(validationHints); Line 42: getValidatedWidgetStyle().setBorderStyle(BorderStyle.SOLID); The parent already sets the border style to solid, so no need to do it here again. Line 43: } Line 44: https://gerrit.ovirt.org/#/c/37302/10/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ObservableOrderedSet.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ObservableOrderedSet.java: Line 6: /** Line 7: * This provides a watchable {@link LinkedHashSet}. Line 8: */ Line 9: Line 10: public abstract class ObservableOrderedSet<T> extends LinkedHashSet<T> { This whole class makes no sense to me. What is the purpose of it? Line 11: Line 12: private static final long serialVersionUID = 1L; Line 13: Line 14: public abstract void onAdd(T item); https://gerrit.ovirt.org/#/c/37302/10/frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/webadmin/public/WebAdmin.css File frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/webadmin/public/WebAdmin.css: Line 114: /* right click menu spacing */ Line 115: .gwt-MenuBar .gwt-MenuItem { Line 116: padding: 0 10px !important; Line 117: } Line 118: These should be in their own css file as style classes here are more global than for a specific widget. Line 119: .selectedFlexTableCell { Line 120: background-color : #E6E6FA; Line 121: width: 25px; Line 122: } -- 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: 10 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
