Vojtech Szocs has posted comments on this change.
Change subject: webadmin,userportal: Synchronize refresh.
......................................................................
Patch Set 4:
(12 comments)
Looks good, some small comments inline.
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/model/CommonModelManager.java
Line 20: * Creates new {@link CommonModel} instance and sets up necessary
event listeners.
Line 21: * <p>
Line 22: * Should be called right after successful user authentication,
before redirecting the user to the main section.
Line 23: */
Line 24: public static void init(final EventBus eventBus) {
Small nit-pick: we could also @Inject EventBus into CommonModelManager and make
CommonModelManager implement HasHandlers. That way, this method wouldn't need
any parameters, it would do simply:
WhateverEvent.fire(CommonModelManager.this, ...);
Line 25: commonModel = CommonModel.newInstance();
Line 26:
Line 27: commonModel.getSelectedItemChangedEvent().addListener(new
IEventListener() {
Line 28: @Override
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/model/DataBoundTabModelProvider.java
Line 55: // Add model items change handler
Line 56: getModel().getItemsChangedEvent().addListener(new
IEventListener() {
Line 57: @Override
Line 58: public void eventRaised(Event ev, Object sender, EventArgs
args) {
Line 59: if (getCommonModel() != null && getModel() != null &&
handleItemsChangedEvent()) {
Please see my comment in TabModelProvider.java - we can do this:
if (hasModel() && handleItemsChangedEvent()) { ... }
Line 60: updateData();
Line 61: }
Line 62: }
Line 63: });
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/model/TabModelProvider.java
Line 47: eventBus.addHandler(CleanupModelEvent.getType(), new
CleanupModelEvent.CleanupModelHandler() {
Line 48:
Line 49: @Override
Line 50: public void onCleanupModel(CleanupModelEvent event) {
Line 51: if (getCommonModel() != null && getModel() != null) {
In UserPortal, TabModelProvider#getCommonModel always returns null because
UserPortal's ApplicationInit doesn't call CommonModelManager#init at all -
there's no CommonModel instance in UserPortal.
This could be refactored into something like this:
if (hasModel() && handleItemsChangedEvent()) { ... }
with new method in TabModelProvider:
/**
* Returns {@code true} if the associated model instance is available to this
model provider.
*/
protected boolean hasModel() {
return getCommonModel() != null && getModel() != null;
}
with UserPortal-specific override in UserPortalDataBoundModelProvider:
@Override
protected boolean hasModel() {
// Don't call super, there's no CommonModel in UserPortal
return getModel() != null;
}
Line 52: //Setting eventbus to null will also unregister
the handlers.
Line 53: getModel().setEventBus(null);
Line 54: }
Line 55: }
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/EntityModel.java
Line 177: /**
Line 178: * Get the GWT event bus.
Line 179: * @return The {@code EventBus}, can be null.
Line 180: */
Line 181: public EventBus getEventBus() {
Thinking about it, this method can be protected and final.
Line 182: return eventBus;
Line 183: }
Line 184:
Line 185: /**
Line 185: /**
Line 186: * Set the GWT event bus.
Line 187: * @param eventBus The {@code EventBus}, can be null.
Line 188: */
Line 189: public void setEventBus(EventBus eventBus) {
I think this method can be final with addition of extra "hook" (overridable)
method to register handlers:
public final void setEventBus(EventBus eventBus) {
this.eventBus = eventBus;
if (eventBus != null) {
registerHandlers();
} else {
unregisterHandlers();
}
}
/**
* Override this method to register custom event handlers as necessary.
*/
protected void registerHandlers() {
// No-op, override as necessary
}
Line 190: this.eventBus = eventBus;
Line 191: if (eventBus == null) {
Line 192: unregisterHandlers();
Line 193: }
Line 196: /**
Line 197: * Register a handler.
Line 198: * @param reg The {@code HandlerRegistration} returned from
registering a handler.
Line 199: */
Line 200: public void registerHandler(HandlerRegistration reg) {
I think this should be final.
Line 201: if (reg != null && !handlerRegistrations.contains(reg)) {
Line 202: handlerRegistrations.add(reg);
Line 203: }
Line 204: }
Line 205:
Line 206: /**
Line 207: * Unregister all registered handlers.
Line 208: */
Line 209: public void unregisterHandlers() {
I think this should be final.
Line 210: for (HandlerRegistration reg: handlerRegistrations) {
Line 211: reg.removeHandler(); // can't call unregisterHandler(reg)
as that would modify the list during iteration
Line 212: }
Line 213: handlerRegistrations.clear();
Line 216: /**
Line 217: * Unregister a specific handler using its {@code
HandlerRegistration}.
Line 218: * @param reg The {@code HandlerRegistration} to use to remove
the handler.
Line 219: */
Line 220: public void unregisterHandler(HandlerRegistration reg) {
I think this should be final.
Line 221: if (reg != null) {
Line 222: reg.removeHandler();
Line 223: handlerRegistrations.remove(reg);
Line 224: }
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SearchableListModel.java
Line 885: return getListName();
Line 886: }
Line 887:
Line 888: @Override
Line 889: public void setEventBus(EventBus eventBus) {
See my comment in EntityModel.java - you can override registerHandlers method
instead, without worrying about EventBus being null:
@Override
protected void registerHandlers() {
registerHandler(getEventBus().addHandler(...));
}
Line 890: super.setEventBus(eventBus);
Line 891: if (eventBus != null) {
Line 892: //Register to listen for operation complete events.
Line 893:
registerHandler(getEventBus().addHandler(RefreshActiveModelEvent.getType(),
Line 906: }
Line 907: }
Line 908:
Line 909: @Override
Line 910: public void fireEvent(GwtEvent<?> event) {
I think this method can be moved right into EntityModel.java
Line 911: getEventBus().fireEvent(event);
Line 912: }
Line 913:
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/events/EventListModel.java
Line 235: {
Line 236: return;
Line 237: }
Line 238:
Line 239: if (!source.isEmpty()) {
EventListModel has bunch of subclasses, do we want this behavior for all of
them? (i.e. VmEventListModel that retrieves events for given VM)
Line 240: //We received some new events, tell the active models to
update.
Line 241: RefreshActiveModelEvent.fire(this, false);
Line 242: }
Line 243: List<AuditLog> list = (List<AuditLog>) getItems();
....................................................
File
frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/system/ApplicationInit.java
Line 101: Window.Location.reload();
Line 102: }
Line 103:
Line 104: frontend.clearLoggedInUser();
Line 105: ApplicationInit.super.performLogout();
Small nit-pick: I guess "ApplicationInit.super" isn't really necessary here.
Line 106: connectAutomaticallyManager.resetAlreadyOpened();
Line 107: }
Line 108: };
Line 109:
--
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: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <[email protected]>
Gerrit-Reviewer: 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
