Alexander Wels has posted comments on this change. Change subject: userportal,webadmin: synchronize grid refresh ......................................................................
Patch Set 3: (6 comments) http://gerrit.ovirt.org/#/c/23605/3/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 109: onRefresh(modelTimer.getValue()); Line 110: } Line 111: }); Line 112: if (wasTimerActive) { Line 113: modelTimer.start(); > If GridTimer.active == true then the timer is in one of two possible states Good point, didn't even notice the resume method. it eliminates the need for the variable as well, I can just call resume. Line 114: } Line 115: } Line 116: Line 117: /** Line 165: Line 166: public void setCurrentRefreshRate(int newRefreshRate) { Line 167: saveRefreshRate(newRefreshRate); Line 168: getModelTimer().setRefreshRate(readRefreshRate()); Line 169: updateController(); > Hm, I suggest to extract timer-specific code in updateController method int Done Line 170: } Line 171: Line 172: public int getCurrentRefreshRate() { Line 173: return getModelTimer().getRefreshRate(); Line 173: return getModelTimer().getRefreshRate(); Line 174: } Line 175: Line 176: String getRefreshRateItemKey() { Line 177: return GRID_REFRESH_RATE_PREFIX; > Since the refresh rate is now shared for all grids, I suggest to rename GRI Done Line 178: } Line 179: Line 180: void saveRefreshRate(int newRefreshRate) { Line 181: clientStorage.setLocalItem(getRefreshRateItemKey(), String.valueOf(newRefreshRate)); http://gerrit.ovirt.org/#/c/23605/3/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 202: reset(); Line 203: logger.fine("GridTimer[" + name + "]: Refresh Rate set to: " + interval); //$NON-NLS-1$ //$NON-NLS-2$ Line 204: // set the NORMAL interval Line 205: normalInterval = interval; Line 206: ValueChangeEvent.fire(this, String.valueOf(getRefreshRate())); > Hm, just a nit-picky question: shouldn't GridTimer implement Good point, the reason it was still a string is due to the fireEvent in the stop() method. But that can fire with an integer as well. Line 207: } Line 208: Line 209: public void start() { Line 210: logger.fine("GridTimer[" + name + "].start()"); //$NON-NLS-1$ //$NON-NLS-2$ http://gerrit.ovirt.org/#/c/23605/3/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 362: public void onValueChange(ValueChangeEvent<String> event) { Line 363: int newInterval; Line 364: //Use the new interval, if the value is invalid us the old interval. Line 365: try { Line 366: newInterval = Integer.valueOf(event.getValue()); > Please see my comment in GridTimer.java - we could avoid string-to-integer Done Line 367: } catch (NumberFormatException nfe) { Line 368: newInterval = timer.getRefreshRate(); Line 369: } Line 370: if (timer.isActive()) { Line 381: } Line 382: }); Line 383: } else { Line 384: removeTimerChangeHandler(); //Make sure to remove old handler before adding new one. Line 385: addTimerChangeListener(); > Hm, for better readability I'd prefer to have an extra method for replace h Done Line 386: } Line 387: return timerChangeHandler; Line 388: } Line 389: -- 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: 3 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
