Ramesh N has posted comments on this change. Change subject: webadmin: fix NPE and visibility issues in CheckBoxGroup ......................................................................
Patch Set 1: (4 comments) https://gerrit.ovirt.org/#/c/40798/1/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 145: */ Line 146: @Override Line 147: public void setValue(List<T> value, boolean fireEvents) { Line 148: List<T> selectedItems = getValue(); Line 149: if (value == selectedItems > why is this required? Its required when there is no change to the selection. We will not do anything if there is no change in the selection. Line 150: || (selectedItems != null && selectedItems.equals(value))) { Line 151: return; Line 152: } Line 153: clearAllSelections(); Line 152: } Line 153: clearAllSelections(); Line 154: if(value == null){ Line 155: return; Line 156: } > please place this as first check We have to clear all the selections if the selection is set to null. Thats why I have this check here. So the order is as follows. If there is no change to the selection the do nothing. If selection is set to null, then clear everything and return. If there is a new selection, then clear all the selection and re-select all the entries in given list. Line 157: for (T currentvalue : value) { Line 158: if (checkBoxes.containsKey(currentvalue)) { Line 159: checkBoxes.get(currentvalue).setValue(true); Line 160: } Line 176: * @return void Line 177: */ Line 178: @Override Line 179: public void setAcceptableValues(Collection<List<T>> values) { Line 180: List<T> seletedItems = getValue(); > are you sure getValue will always be non-null? If a looks at the getValue() impl, its always not null. I am not sure what u meant by everything is selected by default? Line 181: wrapperPanel.clear(); Line 182: checkBoxes.clear(); Line 183: if (values.isEmpty()) { Line 184: throw new IllegalArgumentException("Widget has nothing to do");//$NON-NLS-1$ Line 195: private void showCheckBoxes(List<T> seletedItems) { Line 196: for (Entry<T, CheckBox> currentEntry : checkBoxes.entrySet()) { Line 197: wrapperPanel.add(currentEntry.getValue()); Line 198: if (seletedItems.contains(currentEntry.getKey())) { Line 199: currentEntry.getValue().setValue(true, false); > you could simply do currentEntry.getValue().setValue(true) Done Line 200: } Line 201: } Line 202: } Line 203: -- To view, visit https://gerrit.ovirt.org/40798 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia4f1d371d6c6cbecad9642641d762e954016105d Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
