Vojtech Szocs has posted comments on this change. Change subject: webadmin: Fix server-side sort issue ......................................................................
Patch Set 5: (1 comment) http://gerrit.ovirt.org/#/c/28557/5/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/AbstractActionTable.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/table/AbstractActionTable.java: Line 239: table.enableColumnWidthPersistence(clientStorage, dataProvider.getModel()); Line 240: } Line 241: Line 242: // When UiCommon models are (re)initialized, register search string change listener Line 243: eventBus.addHandler(UiCommonInitEvent.getType(), new UiCommonInitHandler() { > Do these tables exist for the entire login session of the user? I'm not sur > Do these tables exist for the entire login session of the user? AbstractActionTable instances (like SimpleActionTable) are initialized once, typically as part of main or sub tab View initialization. The associated View is typically a singleton, scoped to the entire GWT application. In consequence, life-time of an AbstractActionTable instance is assumed to be application-wide singleton as well. Take a look at AbstractActionPanel#registerSelectionChangeHandler, which does following: // this is because associated View might be constructed lazily after login (UiCommonInitEvent) addSelectionChangeListener(itemSelectionChangeHandler); // this is because on login, all models get re-initialized (model instances change on each login) eventBus.addHandler(UiCommonInitEvent.getType(), new UiCommonInitHandler() { @Override public void onUiCommonInit(UiCommonInitEvent event) { addSelectionChangeListener(itemSelectionChangeHandler); } }); > If they indeed don't, then this handler should probably be removed sometime, > so that events aren't relayed to destroyed objects. It most likely should be > added as part of onLoad()/onAttach() and removed as part of > onUnload/onDetach(). As for intercepting widget's attach to DOM, GWT Javadoc says that onLoad should be always preferred to onAttach. Widgets such as AbstractActionTable get attached & detached quite frequently, imagine switching between different main or sub tabs. > If they do, then this logic should probably be moved somewhere else - it > sounds suboptimal to send these events to ALL tabs (rather than just the > visible ones), especially considering the performance issues we already have > with events. Probably somewhere in tabs' presenters (onReveal(), onHide()). I don't think that dispatching events to a lot of listeners is too much of an overhead; look at SimpleEventBus.map representation and its usage. Actually I think that GWT Widget onLoad/onUnload + UiCommon Event addListener/removeListener is bigger overhead than letting SimpleEventBus keep references to GWT Event handlers. Since AbstractActionTable relies on backing SearchableListModel (via SearchableTableModelProvider), logical scope of AbstractActionTable is the one of the model, which is one login session. Line 244: @Override Line 245: public void onUiCommonInit(UiCommonInitEvent event) { Line 246: addModelSearchStringChangeListener(dataProvider.getModel()); Line 247: } -- To view, visit http://gerrit.ovirt.org/28557 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7ce39a4e2b6323e6e0276a3a462a3f9da005344b Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
