Gilad Chaplik has posted comments on this change. Change subject: webadmin: allow to extend KeyValueModel ......................................................................
Patch Set 2: (6 comments) http://gerrit.ovirt.org/#/c/22926/2/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/form/key_value/KeyValueWidget.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/form/key_value/KeyValueWidget.java: Line 82: model.updateKeys(); Line 83: widgets.remove(widget); Line 84: } Line 85: Line 86: public void updateRowWidth(String rowWidth) { > There's something strange about this method, it has to be called before edi nice comment. Done Line 87: this.rowWidth = rowWidth; Line 88: } Line 89: http://gerrit.ovirt.org/#/c/22926/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/key_value/BaseKeyModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/key_value/BaseKeyModel.java: Line 27: protected void init(Set<String> allKeys, Set<String> usedKeys) { Line 28: this.allKeys = new HashSet<String>(allKeys); Line 29: this.usedKeys = new HashSet<String>(usedKeys); Line 30: List<KeyValueLineModel> list = new ArrayList<KeyValueLineModel>(); Line 31: KeyValueLineModel lineModel; > No reason for this to be outside of the loop, you can inline it with create old habits Done. Line 32: disableEvent = true; Line 33: for (String key : usedKeys) { Line 34: lineModel = createNewLineModel(key); Line 35: lineModel.getKeys().setSelectedItem(key); Line 41: } Line 42: Line 43: protected abstract void initLineModel(KeyValueLineModel lineModel, String key); Line 44: Line 45: protected abstract void fillLineModel(KeyValueLineModel lineModel, String key); > The name doesn't explain well what this does, I would name the method setVa Done Line 46: Line 47: public final IEventListener keyChangedListener = new IEventListener() { Line 48: Line 49: @Override Line 80: public boolean isKeyValid(String key) { Line 81: return !(key == null || key.equals(selectKey) || key.equals(noKeys)); Line 82: } Line 83: Line 84: public List<String> getAvailableKeys(String key) { > This can be private. Done Line 85: List<String> list = getAvailableKeys(); Line 86: boolean realKey = isKeyValid(key); Line 87: if (realKey && !list.contains(key)) { Line 88: list.add(0, key); Line 101: Line 102: return list; Line 103: } Line 104: Line 105: public List<String> getAvailableKeys() { > This too. Done Line 106: List<String> list = Line 107: (allKeys == null) ? new LinkedList<String>() : new LinkedList<String>(allKeys); Line 108: list.removeAll(getUsedKeys()); Line 109: if (list.size() > 0) { Line 122: return new ArrayList<String>(usedKeys); Line 123: } Line 124: } Line 125: Line 126: public int possibleKeysCount() { > This can be removed altogether. Done Line 127: return allKeys == null ? 0 : allKeys.size(); Line 128: } Line 129: Line 130: public void updateKeys() { -- To view, visit http://gerrit.ovirt.org/22926 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I92d49a9e3a6099e0e082939bee61ccf484545263 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> 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
