Vojtech Szocs has posted comments on this change. Change subject: webadmin: auto select dialog tab ......................................................................
Patch Set 2: (17 comments) This is a very nice patch, please find my comments inline. This patch is VERY important as it touches UiCommon models and GWT code that binds to it. After it's merged, I _strongly_ suggest to announce changes done in this patch to all UI maintainers. (So that when development continues on UiCommon layer, new changes will be consistent with this patch.) (I'd even say you could do some quick public session about it, showing code changes and their purpose, because people might not read email announcements.) http://gerrit.ovirt.org/#/c/27781/2/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/presenter/AbstractModelBoundPopupPresenterWidget.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/presenter/AbstractModelBoundPopupPresenterWidget.java: Line 193: // Initialize popup contents from the model Line 194: getView().edit(model); Line 195: getView().updateTabIndexes(); Line 196: Line 197: model.setEventBus((EventBus) getEventBus()); Nice! Dialog models don't have model providers associated, this code ensures that they benefit from EventBus just like other non-dialog models. One small thing - EntityModel.setEventBus has: assert this.eventBus == null : "EventBus is already set"; even though this is very unlikely to happen (typically we receive freshly created model instance here, triggered by "Window" property change) - I suggest to do following: // Re-initialize event bus on the model model.unsetEventBus(); model.setEventBus((EventBus) getEventBus()); Or even better, we could have some hasEventBus method on the model (returning true if model's eventBus != null) and do following: // Initialize event bus on the model if (!model.hasEventBus()) { model.setEventBus((EventBus) getEventBus()); } Line 198: Line 199: } Line 200: Line 201: @Override http://gerrit.ovirt.org/#/c/27781/2/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/utils/ValidationTabSwitchHelper.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/utils/ValidationTabSwitchHelper.java: Line 27: * @param view The {@code View} to update based on state of the model. Line 28: * @return The {@code HandlerRegistration} so the caller can manage the handlers. Line 29: */ Line 30: public static HandlerRegistration registerValidationHandler(final EventBus eventBus, Line 31: final AbstractModelBoundPopupPresenterWidget<?, ?> presenterWidget, final TabbedView view) { Note, presenterWidget and view arguments will be typically short-lived, i.e. they will represent a one-time-use, non-singleton PresenterWidget/View component. I think that current implementation via static helper method is OK, because HandlerRegistration returned by this method should be removed when un-binding corresponding PresenterWidget by GWTP infra. Line 32: return eventBus.addHandler(ValidationCompleteEvent.getType(), Line 33: new ValidationCompleteEventHandler() { Line 34: Line 35: @Override Line 38: if (event.getModel() != null && event.getModel().equals(presenterWidget.getModel())) { Line 39: //Get the invalid tab names from the model. Line 40: Set<TabName> invalidTabs = ((HasValidatedTabs) presenterWidget.getModel()).getInvalidTabs(); Line 41: //Get the tab names to dialog tab widget map from the view. Line 42: Map<TabName, DialogTab> mapping = view.getTabNameMapping(); I really like the concept of letting the view declare tabName-to-DialogTab mapping. Line 43: markTabs(invalidTabs, mapping); Line 44: //Check if the current active tab is invalid, if so don't do anything. Line 45: for (TabName invalidTabName: invalidTabs) { Line 46: if (view.getTabPanel().getActiveTab().equals(mapping.get(invalidTabName))) { Line 61: * @param mapping The TabName to DialogTab mapping. Line 62: */ Line 63: private static void markTabs(Set<TabName> invalidTabs, Map<TabName, DialogTab> mapping) { Line 64: for (Map.Entry<TabName, DialogTab> entry: mapping.entrySet()) { Line 65: if(invalidTabs.contains(entry.getKey())) { Please use space between "if" and "(" for better readability. Line 66: entry.getValue().markAsInvalid(null); Line 67: } else { Line 68: entry.getValue().markAsValid(); Line 69: } http://gerrit.ovirt.org/#/c/27781/2/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/view/popup/AbstractTabbedModelBoundPopupView.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/view/popup/AbstractTabbedModelBoundPopupView.java: Line 29: } Line 30: }); Line 31: } Line 32: Line 33: abstract protected void populateTabMap(); Minor thing, please consider putting "protected" in front of "abstract" for consistency. (I wish Java modifier rules were more strict to promote consistent style.) Also, it would be nice if this class managed the actual mapping instead of subclasses. For example: protected abstract void populateTabMap(Map<TabName, DialogTab> tabMap) { // tabMap managed by AbstractTabbedModelBoundPopupView, initially empty // subclasses just need to update tabMap, no need to create one on their own } Line 34: Line 35: @Override Line 36: public Map<TabName, DialogTab> getTabNameMapping() { Line 37: return tabMap; http://gerrit.ovirt.org/#/c/27781/2/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/AbstractVmPopupWidget.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/AbstractVmPopupWidget.java: Line 768: private final CommonApplicationTemplates applicationTemplates; Line 769: Line 770: private final CommonApplicationConstants constants; Line 771: Line 772: private final Map<TabName, DialogTab> tabMap = new HashMap<TabName, DialogTab>(); Please see my comment in AbstractTabbedModelBoundPopupView.java - we could avoid the need to manage tabMap here. Line 773: Line 774: @SuppressWarnings({ "unchecked", "rawtypes" }) Line 775: public AbstractVmPopupWidget(CommonApplicationConstants constants, Line 776: CommonApplicationResources resources, Line 856: Line 857: driver.initialize(this); Line 858: Line 859: initialize(); Line 860: populateTabMap(); This call could be performed in AbstractTabbedModelBoundPopupView so that subclasses only implement populateTabMap method, without caring about calling this method. Line 861: } Line 862: Line 863: protected void populateTabMap() { Line 864: getTabNameMapping().put(UnitVmModel.UnitVmTabNames.GENERAL_TAB, generalTab); Line 859: initialize(); Line 860: populateTabMap(); Line 861: } Line 862: Line 863: protected void populateTabMap() { Please see my comment in AbstractTabbedModelBoundPopupView.java - we could simply pass tabMap as argument here. Also, shouldn't this method be marked with @Override as it implements abstract method defined in parent View class? Line 864: getTabNameMapping().put(UnitVmModel.UnitVmTabNames.GENERAL_TAB, generalTab); Line 865: getTabNameMapping().put(UnitVmModel.UnitVmTabNames.BOOT_OPTIONS_TAB, this.bootOptionsTab); Line 866: getTabNameMapping().put(UnitVmModel.UnitVmTabNames.CONSOLE_TAB, this.consoleTab); Line 867: getTabNameMapping().put(UnitVmModel.UnitVmTabNames.CUSTOM_PROPERTIES_TAB, this.customPropertiesTab); Line 1868: return mainTabPanel; Line 1869: } Line 1870: Line 1871: @Override Line 1872: public Map<TabName, DialogTab> getTabNameMapping() { Please see my comments above, tabMap could be managed simply via AbstractTabbedModelBoundPopupView so there would be no need to override this method. Line 1873: return tabMap; Line 1874: } http://gerrit.ovirt.org/#/c/27781/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/Model.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/Model.java: Line 31: { Line 32: /** Line 33: * The GWT event bus. Line 34: */ Line 35: private EventBus eventBus; +1 on moving event bus handling right into Model class. Line 36: Line 37: private final List<HandlerRegistration> handlerRegistrations = new ArrayList<HandlerRegistration>(); Line 38: Line 39: public static final String CANCEL_COMMAND = "Cancel"; //$NON-NLS-1$ http://gerrit.ovirt.org/#/c/27781/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/TabName.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/TabName.java: Line 2: Line 3: /** Line 4: * All classes implementing {@code HasValidatedTabs} should create an enum that implements this interface. Line 5: */ Line 6: public interface TabName { Hm, I don't see any getValue method here so I assume the comparison between TabName instances is done simply via reference comparison, relying on the fact that enum instances are singletons. I understand that adding some getValue method here would complicate usage of this interface, but please consider updating Javadoc according to my assumption above (if it's correct, that is). http://gerrit.ovirt.org/#/c/27781/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterModel.java: Line 58: } Line 59: Line 60: private Map<Guid, PolicyUnit> policyUnitMap; Line 61: private ListModel<ClusterPolicy> clusterPolicy; Line 62: private final Set<TabName> invalidTabs = new HashSet<TabName>(); Can't we make this protected final field in some model superclass, so that we don't have to declare it within each specific model? Line 63: Line 64: public ListModel<ClusterPolicy> getClusterPolicy() { Line 65: return clusterPolicy; Line 66: } Line 562: public void setSerialNumberPolicy(SerialNumberPolicyModel serialNumberPolicy) { Line 563: this.serialNumberPolicy = serialNumberPolicy; Line 564: } Line 565: Line 566: public boolean getIsGeneralTabValid() I _strongly_ suggest to do "round two" refactoring, where we get rid of methods like get/setIsXxxTabValid, replacing them with get/setIsTabValid(TabName tab) method. In turn, this will simplify GWT code which uses such "IsTabValid" methods. Line 567: { Line 568: return !invalidTabs.contains(ClusterTabNames.GENERAL_TAB); Line 569: } Line 570: Line 1654: Line 1655: } Line 1656: Line 1657: @Override Line 1658: public Set<TabName> getInvalidTabs() { Please see my comment above, this could be moved to some model superclass as well. Line 1659: return invalidTabs; Line 1660: } http://gerrit.ovirt.org/#/c/27781/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/HostModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/HostModel.java: Line 57: Line 58: public abstract class HostModel extends Model implements HasValidatedTabs Line 59: { Line 60: public enum HostTabNames implements TabName { Line 61: GENERAL_TAB, POWER_MANAGEMENT_TAB, SPM_TAB, CONSOLE_TAB, NETWORK_PROVIDER_TAB > Maybe consolidate this enum in an external enum for all the models. There s It is possible, but I'm OK with current approach as well. It's up to you :-) Line 62: } Line 63: Line 64: public static final int HostNameMaxLength = 255; Line 65: public static final String PmSecureKey = "secure"; //$NON-NLS-1$ http://gerrit.ovirt.org/#/c/27781/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/UnitVmModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/UnitVmModel.java: Line 350: onPropertyChanged(new PropertyChangedEventArgs("IsCPUsAmountValid")); //$NON-NLS-1$ Line 351: } Line 352: } Line 353: Line 354: public boolean getIsGeneralTabValid() { Hm, we could really get rid of all those get/setIsXxxTabValid methods in some "round two" refactoring :-) Line 355: return !invalidTabs.contains(UnitVmTabNames.GENERAL_TAB); Line 356: } Line 357: Line 358: public void setIsGeneralTabValid(boolean value) { http://gerrit.ovirt.org/#/c/27781/2/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/HostPopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/HostPopupView.java: Line 594 Line 595 Line 596 Line 597 Line 598 Excellent! I love to see such code removed (and I think it was me who wrote it) :-) -- To view, visit http://gerrit.ovirt.org/27781 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2e9c5ad0cd3ee7f54606958b1fc8abd2bbd972ed Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Greg Sheremeta <gsher...@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
