Vojtech Szocs has posted comments on this change. Change subject: userportal,webadmin: synchronize grid refresh ......................................................................
Patch Set 3: (8 comments) Patch looks good overall, just some comments for possible improvement. 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: * already started and running * already started but paused Why do we need to start the timer here? (The updateController method is invoked early as part of AbstractRefreshManager constructor.) If we indeed need to start the timer here, maybe we should call GridTimer.resume method instead, which resets GridTimer.paused flag to false and invokes GridTimer.start method. 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 into a separate method, for example: private void updateController() { this.controller = modelProvider.getModel(); } private void updateModelTimer() { // rest of code from original updateController method } If I read this correctly, we need to execute timer-specific code (i.e. updateModelTimer method) on each refresh rate change via GUI (i.e. setCurrentRefreshRate method). 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 GRID_REFRESH_RATE_PREFIX to something like this: // Key used to store refresh rate of all data grids private static final String GRID_REFRESH_RATE_KEY = "GridRefreshRate"; 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 126: private int repetitions; Line 127: Line 128: public GridTimer(String name, final EventBus eventBus) { Line 129: this.name = name; Line 130: assert(eventBus != null); +1 on assert here Line 131: this.eventBus = eventBus; Line 132: } Line 133: Line 134: @Override 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 HasValueChangeHandlers<Integer> instead of current HasValueChangeHandlers<String> ? 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 conversion with ValueChangeEvent<Integer> instead of ValueChangeEvent<String> 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 handler logic, for example: protected void addTimerChangeListener() { timerChangeHandler = ... // no need to return timerChangeHandler field } protected void replaceTimerChangeListener() { if (timerChangeHandler == null) { addTimerChangeListener(); } else { removeTimerChangeHandler(); addTimerChangeListener(); } } This also eliminates potential bug where removeTimerChangeHandler would fail to nullify the timerChangeHandler reference (for whatever reason) and original addTimerChangeListener method would be stuck in infinite recursion loop. Line 386: } Line 387: return timerChangeHandler; Line 388: } Line 389: http://gerrit.ovirt.org/#/c/23605/3/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/events/EventListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/events/EventListModel.java: Line 27 Line 28 Line 29 Line 30 Line 31 +1 on eliminating the extra timer here -- 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
