Vojtech Szocs has posted comments on this change.
Change subject: webadmin,userportal: Synchronize refresh.
......................................................................
Patch Set 1:
(20 comments)
....................................................
File frontend/webadmin/modules/frontend/pom.xml
Line 165: <phase>generate-sources</phase>
Line 166: <goals>
Line 167: <goal>add-source</goal>
Line 168: </goals>
Line 169: <configuration>
Hm, following plugin config is used in both webadmin and userportal poms:
<source>${project.build.directory}/${generatedSourcesDirectory}</source>
<source>${project.build.directory}/generated-sources/gwt</source>
I suggest to take this plugin config into parent pom's pluginManagement section
like this:
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>build-helper-maven-plugin</artifactId>
<version>${build-helper-maven-plugin.version}</version>
<configuration>
<sources>
<source>${project.build.directory}/${generatedSourcesDirectory}</source>
<source>${project.build.directory}/generated-sources/gwt</source>
</sources>
</configuration>
</plugin>
If webadmin or userportal pom uses the above mentioned plugin, declared
executions of this plugin can always override global (top-level) plugin
configuration.
Line 170: <sources>
Line 171:
<source>${project.build.directory}/${generatedSourcesDirectory}</source>
Line 172:
<source>${project.build.directory}/generated-sources/gwt</source>
Line 173: </sources>
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/Frontend.java
Line 270: }
Line 271: } finally {
Line 272: raiseQueryCompleteEvent(queryType,
callback.getContext());
Line 273: if (isEventQuery(queryType, parameters) &&
!((List<?>)result.getReturnValue()).isEmpty()) {
Line 274: raiseOperationCompleteEvent(operation);
Do we want this logic for arbitrary query execution via Frontend class? In
other words, shouldn't the EventListModel provider (i.e. events displayed in
bottom part of WebAdmin) drive this logic instead?
(For example, specific sub tab model like "Host / Events" executes the event
query and triggers this logic too.)
Line 275: }
Line 276: }
Line 277: }
Line 278:
Line 623: if (callback != null) {
Line 624: callback.executed(new
FrontendMultipleActionAsyncResult(actionType,
Line 625: parameters, resultObject, state));
Line 626: }
Line 627: raiseOperationCompleteEvent(operationList.get(0));
Shouldn't we call raiseOperationCompleteEvent per each item in operationList?
Is it safe to assume operationList isn't empty?
Line 628: }
Line 629:
Line 630: @Override
Line 631: public void onFailure(final
List<VdcOperation<VdcActionType, VdcActionParametersBase>> operation,
Line 1085: && ((SearchParameters)
parameters).getSearchTypeValue() == SearchType.AuditLog;
Line 1086: }
Line 1087:
Line 1088: private void raiseOperationCompleteEvent(final VdcOperation<?,
?> operation) {
Line 1089: VdcOperationCompleteEvent.fire(this, new
VdcOperationCompleteEvent(operation));
In general, event classes generated via GWTP's @GenEvent annotation have two
types of "fire" method, for example:
public static void fire(HasHandlers source, <any event params> ...);
public static void fire(HasHandlers source, ...Event eventInstance);
Since first one delegates to second one, I suggest to use the first type of
"fire" method, which is also more readable:
VdcOperationCompleteEvent.fire(this, operation);
Line 1090: }
Line 1091:
Line 1092: @Override
Line 1093: public void fireEvent(GwtEvent<?> event) {
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/VdcOperationComplete.java
Line 3: import com.gwtplatform.dispatch.annotation.GenEvent;
Line 4: import com.gwtplatform.dispatch.annotation.Order;
Line 5:
Line 6: /**
Line 7: * Event triggered when a VdcOperationCompletes. The typical use case
is when a {@code VdcAction} completes
... triggered when a VdcOperation completes
Line 8: * and needs to inform models to refresh themselves.
Line 9: */
Line 10: @GenEvent
Line 11: public class VdcOperationComplete {
Line 8: * and needs to inform models to refresh themselves.
Line 9: */
Line 10: @GenEvent
Line 11: public class VdcOperationComplete {
Line 12: @Order(1)
@Order annotation is used to determine order of custom event arguments in
event's "fire" method signature.
Since we have only one argument (operation) we don't really need this
annotation here.
Line 13: VdcOperation<?, ?> operation;
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/model/CommonModelManager.java
Line 27: public static void init(final EventBus eventBus, final CurrentUser
user, final LoginModel loginModel,
Line 28: final FrontendFailureEventListener
frontendFailureEventListener) {
Line 29: if (commonModel != null) {
Line 30: //Unregister handlers
Line 31: commonModel.unregisterHandlers();
We should make best attempt to unregister not only handlers for given model,
but also handlers for associated models. Ideally, unregisterHandlers method
should be called on each model prior to new user login (which triggers
CommonModel recreation in WebAdmin and manual model recreation in UserPortal).
Assuming EventBus is initialized per each model (see my comment in
ListModel.java), we could use EventBus to fire "new user login about to happen
now" event, all existing models could call unregisterHandlers in response to
this event. After that, EventBus reference would be cleared for existing models
and new ones would be created.
Line 32: //Unset the event bus on the old model, so they can be
freed.
Line 33: commonModel.setEventBus(null);
Line 34: }
Line 35: commonModel = CommonModel.newInstance();
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/model/TabModelProvider.java
Line 38
Line 39
Line 40
Line 41
Line 42
This code was the sole reason of hideAndClearPopup method override. With this
code gone, we can also remove the override:
this.popupHandler = new ModelBoundPopupHandler<M>(this, eventBus);
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/CommonModel.java
Line 150: setLoggedInUser(Frontend.getInstance().getLoggedInUser());
Line 151: }
Line 152:
Line 153: @Override
Line 154: public void setEventBus(EventBus eventBus) {
See my comment in ListModel.java - I think this should be a responsibility of
model provider itself, so that each model has EventBus reference properly
initialized.
Line 155: //Assume none of these are null.
Line 156: dataCenterList.setEventBus(eventBus);
Line 157: clusterList.setEventBus(eventBus);
Line 158: hostList.setEventBus(eventBus);
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/ListModel.java
Line 20: {
Line 21: /**
Line 22: * The GWT event bus.
Line 23: */
Line 24: private EventBus eventBus;
Why not push these changes up to EntityModel? i.e. support EventBus throughout
all UiCommon models, if we make an infra change we should make it as consistent
as possible.
Also, why not perform VdcOperationCompleteEvent registration here, providing
protected no-op method for subclasses to extend?
protected void onVdcOperationComplete(VdcOperationCompleteEvent event) {
// No-op, override as necessary
}
(Including support logic for tracking HandlerRegistration(s) for future
removal.)
Line 25:
Line 26: public static EventDefinition selectedItemChangedEventDefinition;
Line 27: private Event privateSelectedItemChangedEvent;
Line 28:
Line 366: /**
Line 367: * Set the GWT event bus.
Line 368: * @param eventBus The {@code EventBus}, can be null.
Line 369: */
Line 370: public void setEventBus(EventBus eventBus) {
Thinking about it, I think the responsibility to set EventBus should be
performed by model provider. If a model doesn't have model provider (i.e.
CommonModel), class for accessing that model (i.e. CommonModelManager) should
take this responsibility.
Right after each model provider gets notified of model change via
TabModelProvider.onCommonModelChange method, it should call model.setEventBus -
this way, assuming all models are backed by model provider, each will have
EventBus reference properly initialized.
Additionally, we could also set EventBus into dialog models (i.e. UnitVmModel)
via ModelBoundPopupHandler - its revealAndAssignPopup method would call:
model.setEventBus(this.getEventBus());
As this could be problematic with regard to EventBus reference cleanup in case
of dialog models, this is just an idea, for now dialog models can live without
EventBus.
Line 371: this.eventBus = eventBus;
Line 372: }
Line 373:
Line 374: public void unregisterHandlers() {
Line 371: this.eventBus = eventBus;
Line 372: }
Line 373:
Line 374: public void unregisterHandlers() {
Line 375:
I suggest to add following comment:
// No-op, override as necessary
Line 376: }
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SearchableListModel.java
Line 54: private static Logger logger =
Logger.getLogger(SearchableListModel.class.getName());
Line 55: private static final String PAGE_STRING_REGEX =
"[\\s]+page[\\s]+[1-9]+[0-9]*[\\s]*$"; //$NON-NLS-1$
Line 56: private static final String PAGE_NUMBER_REGEX = "[1-9]+[0-9]*$";
//$NON-NLS-1$
Line 57:
Line 58: private HandlerRegistration operationCompleteHandlerRegistration;
See my comment in ListModel.java - I'd rather concentrate such logic into
EntityModel.
As for dealing with HandlerRegistration(s) - I'd prefer having some support
logic like this:
private final List<HandlerRegistration> handlerRegistrations = new
ArrayList<HandlerRegistration>();
protected void registerHandler(HandlerRegistration reg) {
if (reg != null) {
handlerRegistrations.add(reg);
}
}
protected void unregisterHandlers() {
for (HandlerRegistration reg : handlerRegistrations) {
reg.removeHandler(); // can't call unregisterHandler(reg) as that
would modify list during iteration
}
handlerRegistrations.clear();
}
protected void unregisterHandler(HandlerRegistration reg) {
if (reg != null) {
reg.removeHandler();
handlerRegistrations.remove(reg);
}
}
private HandlerRegistration operationCompleteHandlerRegistration; // if we
want to track specific handler reg
Line 59:
Line 60: private UICommand privateSearchCommand;
Line 61:
Line 62: public UICommand getSearchCommand()
Line 388: setIsQueryFirstTime(true);
Line 389: syncSearch();
Line 390: setIsQueryFirstTime(false);
Line 391: getTimer().start();
Line 392: if (getEventBus() != null) {
I think we shouldn't care if EventBus == null here. Maybe we should care when
setting EventBus (i.e. assert eventBus != null) but EventBus != null is
essential (must always hold) invariant of this class.
In other words, if EventBus == null here, it should crash rather than silently
ignore the broken EventBus != null invariant.
Line 393: //Make sure to unregister any existing handler,
before adding a new one.
Line 394: unregisterOperationCompleteHandler();
Line 395: //Register to listen for operation complete
events.
Line 396: operationCompleteHandlerRegistration =
getEventBus().addHandler(VdcOperationCompleteEvent.getType(),
Line 390: setIsQueryFirstTime(false);
Line 391: getTimer().start();
Line 392: if (getEventBus() != null) {
Line 393: //Make sure to unregister any existing handler,
before adding a new one.
Line 394: unregisterOperationCompleteHandler();
See my comment above, removing specific handler reg could be done like this:
unregisterHandler(operationCompleteHandlerRegistration);
Line 395: //Register to listen for operation complete
events.
Line 396: operationCompleteHandlerRegistration =
getEventBus().addHandler(VdcOperationCompleteEvent.getType(),
Line 397: new VdcOperationCompleteHandler() {
Line 398: @Override
Line 392: if (getEventBus() != null) {
Line 393: //Make sure to unregister any existing handler,
before adding a new one.
Line 394: unregisterOperationCompleteHandler();
Line 395: //Register to listen for operation complete
events.
Line 396: operationCompleteHandlerRegistration =
getEventBus().addHandler(VdcOperationCompleteEvent.getType(),
See my comment above, adding specific handler reg could be done like this:
operationCompleteHandlerRegistration = ... current code ...
registerHandler(operationCompleteHandlerRegistration);
Line 397: new VdcOperationCompleteHandler() {
Line 398: @Override
Line 399: public void
onVdcOperationComplete(VdcOperationCompleteEvent event) {
Line 400: //This is an operation this model is
interested in, refresh.
Line 790:
Line 791: public void stopRefresh()
Line 792: {
Line 793: getTimer().stop();
Line 794: unregisterOperationCompleteHandler();
See my comment above, removing specific handler reg could be done like this:
unregisterHandler(operationCompleteHandlerRegistration);
Line 795: }
Line 796:
Line 797: @Override
Line 798: public void unregisterHandlers() {
Line 795: }
Line 796:
Line 797: @Override
Line 798: public void unregisterHandlers() {
Line 799: unregisterOperationCompleteHandler();
See my comment above, implementation of this method could operate on
List<HandlerRegistration> to ensure that all handlers are removed, assuming
they have been added to the list before.
Line 800: }
Line 801:
Line 802: private void unregisterOperationCompleteHandler() {
Line 803: if (operationCompleteHandlerRegistration != null) {
Line 798: public void unregisterHandlers() {
Line 799: unregisterOperationCompleteHandler();
Line 800: }
Line 801:
Line 802: private void unregisterOperationCompleteHandler() {
See my comment above, no need for this method, we could use unregisterHandler
method instead.
Line 803: if (operationCompleteHandlerRegistration != null) {
Line 804: operationCompleteHandlerRegistration.removeHandler();
Line 805: }
Line 806: }
....................................................
File
frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/uicommon/model/UserPortalDataBoundModelProvider.java
Line 67: @Override
Line 68: public void onUserPortalModelInit(UserPortalModelInitEvent event) {
Line 69: if (this.model != null) {
Line 70: //Clear the event bus from original model if it still
exists.
Line 71: this.model.unregisterHandlers();
See my comment in CommonModelManager.java
Line 72: this.model.setEventBus(null);
Line 73: }
Line 74: this.model = createModel();
Line 75: this.model.setEventBus(getEventBus());
--
To view, visit http://gerrit.ovirt.org/21057
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa59712f8f74426b0ca6a132664167f42fb8d7b2
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <[email protected]>
Gerrit-Reviewer: Einav Cohen <[email protected]>
Gerrit-Reviewer: Vojtech Szocs <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
