Alexander Wels has posted comments on this change. Change subject: webadmin: System tree refresh ......................................................................
Patch Set 2: (4 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? the comparison should only be done if there are no children or if we don't care about the children. getChildren().size() > 0 && deepCompare Ensures that if deepCompare is false or if there are no children we will end up here. 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 Because the only reason the hashCode is here is because check style complains if it doesn't exist. I really don't care that much about the hash code. 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 Yes I think I can do with a root.equals(match, deepCompare=false). I will change it. 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 I checked what that would do, and I didn't see anything strange when I tested this. On the other hand THAT line is what is causing the tree to automatically select the root node when refreshing. Once I removed it everything started to work normally. -- 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 <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Greg Sheremeta <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[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
