Vojtech Szocs has posted comments on this change. Change subject: userportal,webadmin: synchronize grid refresh ......................................................................
Patch Set 4: (4 comments) Some more small comments for consideration. 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 35: void onManualRefresh(); Line 36: Line 37: } Line 38: Line 39: // Prefix for keys used to store refresh rates of individual data grids I suggest to update comment as well, for example: // Key used to store refresh rate of all data grids Line 40: private static final String GRID_REFRESH_RATE_KEY = "GridRefreshRate"; //$NON-NLS-1$ Line 41: Line 42: private static final Integer DEFAULT_REFRESH_RATE = GridTimer.DEFAULT_NORMAL_RATE; Line 43: private static final Integer OUT_OF_FOCUS_REFRESH_RATE = Integer.valueOf(60000); 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? The original purpose of updateController was to initialize "this.controller" for later interaction. If we call updateTimer here, it means timer.resume will be called for all data grids right after login (updateController call triggered by UiCommonInitHandler). 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 getTimerStatusMessage. 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 for consistency (I know, this is too nitpicky..) :P 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
