Martin Mucha has posted comments on this change. Change subject: webadmin: reverted removal of selectedItemsChanged method. ......................................................................
Patch Set 1: (2 comments) http://gerrit.ovirt.org/#/c/36899/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SearchableListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SearchableListModel.java: Line 614: protected void selectedItemsChanged() { Line 615: /* Line 616: NOTICE, that there's lot of methods overriding this one, looking 'just like' this one, which seems wrong. Line 617: But notice that all those 'updateActionAvailability()' are private, and does not extends each other. Line 618: Making #updateActionAvailability() protected seems not very straightforward, therefore it's not fixed. > This is not the place to suggest refactoring, either send a patch or forget actually I'm not suggesting refactoring, I'm discouraging one providing information why I'm stating that. This is also important, because there may be more clever guy capable of doing proper fix and he needs to know why he should not do that (actually now I think I provided too few information). Discussed problem is on more levels than just two. Problem is not with this class and it's parent as you mention in proposed shorter comment, but with methods overriding this one; those are actually making (otherwise simple) refactoring hard. Line 619: * */ Line 620: super.selectedItemsChanged(); Line 621: updateActionAvailability(); Line 622: } http://gerrit.ovirt.org/#/c/36899/1/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/macpool/SharedMacPoolListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/macpool/SharedMacPoolListModel.java: Line 79: removeAllowed = false; Line 80: } else { Line 81: for (MacPool macPool : (Iterable<MacPool>) getSelectedItems()) { Line 82: if (macPool.isDefaultPool()) { Line 83: removeAllowed = false; > Isn't directly related to this patch, but maybe add a "break;" line here wh I had lots of complaints about unrelated changes, I'll do it in separate patch. But even if there's thousands of mac pools, which is I believe not very likely, this 'break' is optimization in grade of microseconds at best. To verify it, I wrote some test (mainly to learn/verify what I know) and even on my crappy cpu and 10000 records (with making sure JIT kicks in) this is marginal optimization: break present: 737 ns (nano seconds) no break: 19596 ns sure, it's two grades better with break, but ... Line 84: } Line 85: } Line 86: } Line 87: getRemoveCommand().setIsExecutionAllowed(removeAllowed); -- To view, visit http://gerrit.ovirt.org/36899 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I473566cf0755519eb7fc922c435ae8a2ee545779 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@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
