Vojtech Szocs has posted comments on this change. Change subject: frontend: refactoring: Generify list models ......................................................................
Patch Set 26: (27 comments) Excellent refactor patch, Martin! Just a few small (mostly generic type signature related) comments, looks great otherwise. https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/presenter/AbstractSubTabPresenter.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/presenter/AbstractSubTabPresenter.java: Line 40: * View type. Line 41: * @param <P> Line 42: * Proxy type. Line 43: */ Line 44: public abstract class AbstractSubTabPresenter<T, M extends ListWithDetailsModel, D extends HasEntity, V extends AbstractSubTabPresenter.ViewDef<T>, P extends TabContentProxyPlace<?>> Why not: D extends HasEntity<?> to avoid raw type warning? Line 45: extends AbstractTabPresenter<V, P> { Line 46: Line 47: // TODO(vszocs) use HasActionTable<I> instead of raw type HasActionTable, this will Line 48: // require adding new type parameter to presenter (do later as part of refactoring) https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/model/DetailModelProvider.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/model/DetailModelProvider.java: Line 12: * Main model type. Line 13: * @param <D> Line 14: * Detail model type. Line 15: */ Line 16: public interface DetailModelProvider<M extends ListWithDetailsModel, D extends HasEntity> extends ModelProvider<D> { Why not: D extends HasEntity<?> to avoid raw type warning? Line 17: Line 18: /** Line 19: * Notifies main model that the corresponding sub tab has been selected. Line 20: */ https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/model/ModelProvider.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/model/ModelProvider.java: Line 7: * Line 8: * @param <M> Line 9: * Model type. Line 10: */ Line 11: public interface ModelProvider<M extends HasEntity> { Why not: M extends HasEntity<?> to avoid raw type warning? Line 12: Line 13: /** Line 14: * Returns the model instance. Line 15: */ https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/model/TabModelProvider.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/uicommon/model/TabModelProvider.java: Line 20: /** Line 21: * Basic {@link ModelProvider} implementation Line 22: * @param <M> Model type. Line 23: */ Line 24: public abstract class TabModelProvider<M extends HasEntity> implements ModelProvider<M>, ModelBoundPopupResolver<M>, HasHandlers { Why not: M extends HasEntity<?> to avoid raw type warning? Line 25: Line 26: private final EventBus eventBus; Line 27: private final ModelBoundPopupHandler<M> popupHandler; Line 28: private boolean modelInitialized = false; https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/view/AbstractSubTabFormView.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/view/AbstractSubTabFormView.java: Line 15: * Main model type. Line 16: * @param <D> Line 17: * Detail model type. Line 18: */ Line 19: public abstract class AbstractSubTabFormView<T, M extends ListWithDetailsModel, D extends HasEntity> extends AbstractView implements AbstractSubTabPresenter.ViewDef<T> { Why not: D extends HasEntity<?> to avoid raw type warning? Line 20: Line 21: private final DetailModelProvider<M, D> modelProvider; Line 22: Line 23: public AbstractSubTabFormView(DetailModelProvider<M, D> modelProvider) { https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/tab/ModelBoundTab.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/tab/ModelBoundTab.java: Line 23: setAccessible(tabData.getModelProvider().getModel().getIsAvailable()); Line 24: registerModelEventListeners(tabData.getModelProvider()); Line 25: } Line 26: Line 27: void registerModelEventListeners(final ModelProvider<? extends HasEntity> modelProvider) { Why not: ? extends HasEntity<?> to avoid raw type warning? Line 28: modelProvider.getModel().getPropertyChangedEvent().addListener(new IEventListener<PropertyChangedEventArgs>() { Line 29: @Override Line 30: public void eventRaised(Event<? extends PropertyChangedEventArgs> ev, Object sender, PropertyChangedEventArgs args) { Line 31: // Update tab accessibility when 'IsAvailable' property changes https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/tab/ModelBoundTabData.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/tab/ModelBoundTabData.java: Line 11: * tab widgets. Line 12: */ Line 13: public class ModelBoundTabData extends TabDataBasic { Line 14: Line 15: private final ModelProvider<? extends HasEntity> modelProvider; Why not: ? extends HasEntity<?> to avoid raw type warning? Line 16: private final Align align; Line 17: Line 18: public ModelBoundTabData(String label, float priority, Line 19: ModelProvider<? extends HasEntity> modelProvider) { Line 15: private final ModelProvider<? extends HasEntity> modelProvider; Line 16: private final Align align; Line 17: Line 18: public ModelBoundTabData(String label, float priority, Line 19: ModelProvider<? extends HasEntity> modelProvider) { Why not: ? extends HasEntity<?> to avoid raw type warning? Line 20: this(label, priority, modelProvider, Align.LEFT); Line 21: } Line 22: Line 23: public ModelBoundTabData(String label, float priority, Line 20: this(label, priority, modelProvider, Align.LEFT); Line 21: } Line 22: Line 23: public ModelBoundTabData(String label, float priority, Line 24: ModelProvider<? extends HasEntity> modelProvider, Why not: ? extends HasEntity<?> to avoid raw type warning? Line 25: Align align) { Line 26: super(label, priority); Line 27: this.modelProvider = modelProvider; Line 28: this.align = align; Line 27: this.modelProvider = modelProvider; Line 28: this.align = align; Line 29: } Line 30: Line 31: public ModelProvider<? extends HasEntity> getModelProvider() { Why not: ? extends HasEntity<?> to avoid raw type warning? Line 32: return modelProvider; Line 33: } Line 34: Line 35: public Align getAlign() { https://gerrit.ovirt.org/#/c/32907/26/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 293: } Line 294: }); Line 295: } Line 296: Line 297: void addModelSearchStringChangeListener(final SearchableListModel<Object, ?> model) { Object is redundant here, could be: SearchableListModel<?, ?> Line 298: if (model.supportsServerSideSorting()) { Line 299: model.getPropertyChangedEvent().addListener(new IEventListener<PropertyChangedEventArgs>() { Line 300: @Override Line 301: public void eventRaised(org.ovirt.engine.ui.uicompat.Event<? extends PropertyChangedEventArgs> ev, Object sender, PropertyChangedEventArgs args) { Line 466: tableWithHeader.addColumnSortHandler(new ColumnSortEvent.Handler() { Line 467: @SuppressWarnings("unchecked") Line 468: @Override Line 469: public void onColumnSort(ColumnSortEvent event) { Line 470: SearchableListModel<Object, ?> model = getDataProvider().getModel(); Object is redundant here, could be: SearchableListModel<?, ?> Line 471: Column<?, ?> column = event.getColumn(); Line 472: Line 473: if (column instanceof AbstractSortableColumn) { Line 474: AbstractSortableColumn<T, ?> sortedColumn = (AbstractSortableColumn<T, ?>) column; Line 487: // Otherwise, fall back to client-side sorting Line 488: else { Line 489: Comparator<? super T> comparator = sortedColumn.getComparator(); Line 490: if (comparator != null) { Line 491: ((SearchableListModel<Object, T>) model).setComparator(comparator, event.isSortAscending()); Object is redundant here, could be: SearchableListModel<?, T> Line 492: sortApplied = true; Line 493: } Line 494: } Line 495: https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/HasEntity.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/HasEntity.java: Line 3: import org.ovirt.engine.ui.uicompat.Event; Line 4: import org.ovirt.engine.ui.uicompat.EventArgs; Line 5: import org.ovirt.engine.ui.uicompat.EventDefinition; Line 6: Line 7: public interface HasEntity<T> extends IModel { Can you please add a very brief Javadoc to describe the purpose of this interface? Something like: "Interface implemented by models that work with a dedicated entity instance." Line 8: Line 9: EventDefinition entityChangedEventDefinition = new EventDefinition("EntityChanged", HasEntity.class); //$NON-NLS-1$; Line 10: Line 11: Event<EventArgs> getEntityChangedEvent(); https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/IModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/IModel.java: Line 3: import org.ovirt.engine.ui.uicommonweb.UICommand; Line 4: import org.ovirt.engine.ui.uicommonweb.models.common.ProgressModel; Line 5: import org.ovirt.engine.ui.uicompat.IProvidePropertyChangedEvent; Line 6: Line 7: public interface IModel extends IProvidePropertyChangedEvent { Can you please add a very brief Javadoc to describe the purpose of this interface? Something like: "Common interface implemented by model objects." Line 8: void setWindow(Model value); Line 9: Model getWindow(); Line 10: Line 11: Model getConfirmWindow(); https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/MainModelSelectionChangeEvent.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/MainModelSelectionChangeEvent.java: Line 6: import com.google.web.bindery.event.shared.HandlerRegistration; Line 7: Line 8: public class MainModelSelectionChangeEvent extends GwtEvent<MainModelSelectionChangeEvent.MainModelSelectionChangeHandler> { Line 9: Line 10: SearchableListModel<Object, ? extends EntityModel<?>> mainModel; Object is redundant here, could be: SearchableListModel<?, ? extends EntityModel<?>> Line 11: Line 12: protected MainModelSelectionChangeEvent() { Line 13: // Possibly for serialization. Line 14: } Line 12: protected MainModelSelectionChangeEvent() { Line 13: // Possibly for serialization. Line 14: } Line 15: Line 16: public MainModelSelectionChangeEvent(SearchableListModel<Object, ? extends EntityModel<?>> mainModel) { Object is redundant here, could be: SearchableListModel<?, ? extends EntityModel<?>> Line 17: this.mainModel = mainModel; Line 18: } Line 19: Line 20: public static void fire(HasHandlers source, SearchableListModel<Object, ? extends EntityModel<?>> mainModel) { Line 16: public MainModelSelectionChangeEvent(SearchableListModel<Object, ? extends EntityModel<?>> mainModel) { Line 17: this.mainModel = mainModel; Line 18: } Line 19: Line 20: public static void fire(HasHandlers source, SearchableListModel<Object, ? extends EntityModel<?>> mainModel) { Object is redundant here, could be: SearchableListModel<?, ? extends EntityModel<?>> Line 21: MainModelSelectionChangeEvent eventInstance = new MainModelSelectionChangeEvent(mainModel); Line 22: source.fireEvent(eventInstance); Line 23: } Line 24: Line 49: protected void dispatch(MainModelSelectionChangeHandler handler) { Line 50: handler.onMainModelSelectionChange(this); Line 51: } Line 52: Line 53: public SearchableListModel<Object, ? extends EntityModel<?>> getMainModel() { Object is redundant here, could be: SearchableListModel<?, ? extends EntityModel<?>> Line 54: return mainModel; Line 55: } Line 56: Line 57: @Override https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SystemTreeModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SystemTreeModel.java: Line 35: import org.ovirt.engine.ui.uicompat.FrontendMultipleQueryAsyncResult; Line 36: import org.ovirt.engine.ui.uicompat.IFrontendMultipleQueryAsyncCallback; Line 37: import org.ovirt.engine.ui.uicompat.PropertyChangedEventArgs; Line 38: Line 39: public class SystemTreeModel extends SearchableListModel<Object, SystemTreeItemModel> implements IFrontendMultipleQueryAsyncCallback { Shouldn't this be: SearchableListModel<Void, SystemTreeItemModel> ? Line 40: Line 41: public static final EventDefinition resetRequestedEventDefinition; Line 42: public static final EventDefinition beforeItemsChangedEventDefinition; Line 43: private Event<EventArgs> privateResetRequestedEvent; https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/section/main/presenter/tab/extended/SideTabExtendedVirtualMachinePresenter.java File frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/section/main/presenter/tab/extended/SideTabExtendedVirtualMachinePresenter.java: Line 76: */ Line 77: @Override Line 78: protected String createRequestToken() { Line 79: String requestToken = super.createRequestToken(); Line 80: HasEntity model = modelProvider.getModel().getActiveDetailModel(); Why not: HasEntity<?> to avoid raw type warning? Line 81: if (model instanceof PoolGeneralModel || Line 82: model instanceof PoolInterfaceListModel || Line 83: model instanceof PoolDiskListModel Line 84: ) { https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/section/main/presenter/tab/extended/template/AbstractSubTabExtendedTemplatePresenter.java File frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/section/main/presenter/tab/extended/template/AbstractSubTabExtendedTemplatePresenter.java: Line 14: import com.gwtplatform.mvp.client.proxy.PlaceManager; Line 15: import com.gwtplatform.mvp.shared.proxy.PlaceRequest; Line 16: import com.gwtplatform.mvp.client.proxy.TabContentProxyPlace; Line 17: Line 18: public abstract class AbstractSubTabExtendedTemplatePresenter<D extends HasEntity, V extends AbstractSubTabPresenter.ViewDef<VmTemplate>, P extends TabContentProxyPlace<?>> Why not: D extends HasEntity<?> to avoid raw type warning? Line 19: extends AbstractSubTabPresenter<VmTemplate, UserPortalTemplateListModel, D, V, P> { Line 20: Line 21: public AbstractSubTabExtendedTemplatePresenter(EventBus eventBus, V view, P proxy, Line 22: PlaceManager placeManager, DetailModelProvider<UserPortalTemplateListModel, D> modelProvider) { https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/section/main/presenter/tab/extended/vm/AbstractSubTabExtendedVmPresenter.java File frontend/webadmin/modules/userportal-gwtp/src/main/java/org/ovirt/engine/ui/userportal/section/main/presenter/tab/extended/vm/AbstractSubTabExtendedVmPresenter.java: Line 14: import com.gwtplatform.mvp.client.proxy.PlaceManager; Line 15: import com.gwtplatform.mvp.shared.proxy.PlaceRequest; Line 16: import com.gwtplatform.mvp.client.proxy.TabContentProxyPlace; Line 17: Line 18: public abstract class AbstractSubTabExtendedVmPresenter<D extends HasEntity, V Why not: D extends HasEntity<?> to avoid raw type warning? Line 19: extends AbstractSubTabPresenter.ViewDef<UserPortalItemModel>, P extends TabContentProxyPlace<?>> Line 20: extends AbstractSubTabPresenter<UserPortalItemModel, UserPortalListModel, D, V, P> { Line 21: Line 22: public AbstractSubTabExtendedVmPresenter(EventBus eventBus, V view, P proxy, https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/storage/backup/ImportCloneDialogPopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/storage/backup/ImportCloneDialogPopupView.java: Line 99: } Line 100: Line 101: @Override Line 102: public void edit(ImportCloneModel object) { Line 103: if (((ImportEntityData<Object>) object.getEntity()).getEntity() instanceof VM) { Object is redundant here, could be: ImportEntityData<?> Line 104: dialogLabelEditor.setText(messages.sameVmNameExists(((ImportVmData) object.getEntity()).getVm().getName())); Line 105: cloneEditor.setLabel(constants.cloneImportVmDetails()); Line 106: suffixEditor.setLabel(constants.cloneImportSuffixVm()); Line 107: } else { https://gerrit.ovirt.org/#/c/32907/26/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/storage/backup/ImportVmFromExportDomainPopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/storage/backup/ImportVmFromExportDomainPopupView.java: Line 186: } Line 187: Line 188: protected void subTabLayoutPanelSelectionChanged(Integer selectedItem) { Line 189: if (importModel != null) { Line 190: importModel.setActiveDetailModel((HasEntity) importModel.getDetailModels().get(selectedItem)); Why not: HasEntity<?> to avoid raw type warning? Line 191: } Line 192: } Line 193: Line 194: protected void initGeneralSubTabView() { Line 681: Line 682: @Override Line 683: public void onSelectionChange(SelectionChangeEvent event) { Line 684: if (!firstSelection) { Line 685: object.setActiveDetailModel((HasEntity) object.getDetailModels().get(0)); Why not: HasEntity<?> to avoid raw type warning? Line 686: setGeneralViewSelection(((ImportEntityData<Object>) object.getSelectedItem()).getEntity()); Line 687: firstSelection = true; Line 688: } Line 689: splitLayoutPanel.clear(); Line 682: @Override Line 683: public void onSelectionChange(SelectionChangeEvent event) { Line 684: if (!firstSelection) { Line 685: object.setActiveDetailModel((HasEntity) object.getDetailModels().get(0)); Line 686: setGeneralViewSelection(((ImportEntityData<Object>) object.getSelectedItem()).getEntity()); Object is redundant here, could be: ImportEntityData<?> Line 687: firstSelection = true; Line 688: } Line 689: splitLayoutPanel.clear(); Line 690: splitLayoutPanel.addSouth(subTabLayoutPanel, 230); -- To view, visit https://gerrit.ovirt.org/32907 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I00f1671a5839fe31b49ad0d1c99bad84e5f0c7c4 Gerrit-PatchSet: 26 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Betak <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Frank Kobzik <[email protected]> Gerrit-Reviewer: Greg Sheremeta <[email protected]> Gerrit-Reviewer: Jakub Niedermertl <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: Martin Betak <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: [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
