Alexander Wels has posted comments on this change. Change subject: userportal,webadmin: synchronize grid refresh ......................................................................
Patch Set 4: (3 comments) http://gerrit.ovirt.org/#/c/23605/4/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/refresh/AbstractRefreshManager.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/refresh/AbstractRefreshManager.java: Line 96: Line 97: private void updateController() { Line 98: this.controller = modelProvider.getModel(); Line 99: Line 100: updateTimer(); > Do we really need to call updateTimer in updateController method? Yes but since none of them are active, nothing will happen. On the other hand we do need to configure the handlers for each timer, so that when someone changes the refresh rate, all the timer are updated. Line 101: } Line 102: Line 103: private void updateTimer() { Line 104: final GridTimer modelTimer = getModelTimer(); http://gerrit.ovirt.org/#/c/23605/4/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/GridTimer.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/GridTimer.java: Line 241: public boolean isActive() { Line 242: return active; Line 243: } Line 244: Line 245: public String getValue() { > Hm maybe worth renaming this method to something more meaningful, like getT Done Line 246: logger.fine((isActive() ? "Refresh Status: Active(" : "Inactive(") + (isPaused() ? "paused)" : "running)") + ":" //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ //$NON-NLS-5$ Line 247: + " Rate: " + rateCycle[currentRate] + "(" + getRefreshRate() / 1000 + " sec)"); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$); } Line 248: return ConstantsManager.getInstance().getMessages().refreshInterval(getRefreshRate() / 1000); Line 249: } http://gerrit.ovirt.org/#/c/23605/4/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 377: protected void replaceTimerChangeListener() { Line 378: if (timerChangeHandler == null) { Line 379: addTimerChangeListener(); Line 380: } else { Line 381: removeTimerChangeHandler(); > Small nitpick: rename removeTimerChangeHandler to removeTimerChangeListener Well really I should name them all *handler, and not listener, so I will do that. Line 382: addTimerChangeListener(); Line 383: } Line 384: } Line 385: -- To view, visit http://gerrit.ovirt.org/23605 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8742785cb9b3d890c39859586b03cd53c64b31e5 Gerrit-PatchSet: 4 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: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com> Gerrit-Reviewer: Leonardo Bianconi <leonardo.bianc...@eldorado.org.br> Gerrit-Reviewer: Vojtech Szocs <vsz...@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
