Vojtech Szocs has posted comments on this change.

Change subject: userportal,webadmin: synchronize grid refresh
......................................................................


Patch Set 2:

(4 comments)

http://gerrit.ovirt.org/#/c/23605/2/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 164:         return getModelTimer().getRefreshRate();
Line 165:     }
Line 166: 
Line 167:     String getRefreshRateItemKey() {
Line 168:         return GRID_REFRESH_RATE_PREFIX; //$NON-NLS-1$
NON-NLS comment can be removed.
Line 169:     }
Line 170: 
Line 171:     void saveRefreshRate(int newRefreshRate) {
Line 172:         clientStorage.setLocalItem(getRefreshRateItemKey(), 
String.valueOf(newRefreshRate));


http://gerrit.ovirt.org/#/c/23605/2/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 321
Line 322
Line 323
Line 324
Line 325
This line sets the default SearchableListModel refresh rate, why remove it?


Line 320:                     
logger.fine(SearchableListModel.this.getClass().getName() + ": Executing 
search"); //$NON-NLS-1$
Line 321:                     syncSearch();
Line 322:                     gettimer().stop();
Line 323:                     
gettimer().setRefreshRate(gettimer().getRefreshRate());
Line 324:                     gettimer().start();
I'd suggest to extract these lines into a separate protected method to improve 
readability and remove code duplication in EventListModel.

For example:

 protected void updateRefreshRate(GridTimer timer) {
     ...
 }

Also, this line seems a bit weird:

 gettimer().setRefreshRate(gettimer().getRefreshRate());

Why do we need to do that?

AbstractRefreshManager is responsible for controlling refresh rate of a 
GridController, so any of the following will yield call to 
GridTimer#setRefreshRate:
* user changing refresh rate via drop-down in GUI = 
AbstractRefreshManager#setCurrentRefreshRate
* application window losing/gaining focus = 
AbstractRefreshManager#onWindowFocusChange
Line 325:                 }
Line 326: 
Line 327:             });
Line 328:         }


http://gerrit.ovirt.org/#/c/23605/2/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 113:             public void execute() {
Line 114:                 getRefreshCommand().execute();
Line 115:                 timer.stop();
Line 116:                 timer.setRefreshRate(timer.getRefreshRate());
Line 117:                 timer.start();
Please see my comment in SearchableListModel.java - we could remove code 
duplication.
Line 118:             }
Line 119:         };
Line 120: 
Line 121:         
timer.setRefreshRate(getConfigurator().getPollingTimerInterval());


-- 
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: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: 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

Reply via email to