Vojtech Szocs has posted comments on this change. Change subject: webadmin: System tree refresh ......................................................................
Patch Set 2: (5 comments) http://gerrit.ovirt.org/#/c/26161/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SystemTreeItemModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SystemTreeItemModel.java: Line 130: return false; Line 131: } Line 132: } else { Line 133: if (otherModel.getEntity() != null && otherModel.getEntity().equals(getEntity()) Line 134: && otherModel.getTitle().equals(getTitle())) { Shouldn't this comparison be done regardless of deepCompare value? (Current implementation skips otherModel.entity + otherModel.title comparison if deepCompare is true.) Line 135: return true; Line 136: } Line 137: return false; Line 138: } Line 139: } Line 140: Line 141: @Override Line 142: public int hashCode() { Line 143: return super.hashCode(); This override just delegates to Object.hashCode, any reason why we don't do model.entity + model.title hash computation here? Line 144: } http://gerrit.ovirt.org/#/c/26161/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SystemTreeModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SystemTreeModel.java: Line 656: public SystemTreeItemModel findNode(SystemTreeItemModel root, SystemTreeItemModel match) { Line 657: SystemTreeItemModel result = null; Line 658: if (root != null && match != null) { Line 659: if (root.getEntity() != null && root.getEntity().equals(match.getEntity()) Line 660: && root.getTitle().equals(match.getTitle())) { Please see my comment in SystemTreeItemModel.java - this equality condition could be done simply via root.equals(match) if the equals method compared entity + title regardless of deepCompare value. Line 661: result = root; //match found. Line 662: } else { Line 663: if (root.getChildren().size() > 0) { Line 664: for (int i = 0; i < root.getChildren().size(); i++) { http://gerrit.ovirt.org/#/c/26161/2/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/tree/SystemTree.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/tree/SystemTree.java: Line 116 Line 117 Line 118 Line 119 Line 120 This could potentially have unexpected side effects since I recall CellTree's keyboard selection policy being buggy. Just to be on safe side, can you please check this change with Lior? Line 36: Line 37: private static final int ALL_LEVELS = Integer.MAX_VALUE; Line 38: private static final int ITEM_LEVEL = 3; Line 39: Line 40: protected final Map<Object, Map<String, Boolean>> nodeStateMap = new HashMap<Object, Map<String, Boolean>>(); Please add some brief comment here to help communicate the purpose of this map. Line 41: Line 42: interface WidgetUiBinder extends UiBinder<Widget, SystemTree> { Line 43: WidgetUiBinder uiBinder = GWT.create(WidgetUiBinder.class); Line 44: } -- To view, visit http://gerrit.ovirt.org/26161 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I062163b1f6d2e5fcfd1539b69a3d0d2cee674654 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <aw...@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: Vojtech Szocs <vsz...@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
