Alexander Wels has posted comments on this change. Change subject: webadmin: auto select dialog tab ......................................................................
Patch Set 2: (8 comments) 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! Done 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 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 IMO the view should be the only place that actually contains the mapping. Its the view that manages the tabs, so it should manage the 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. Done 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 If you notice the map is private, and the only access to it is through getTabNameMapping() below. So they can't even 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 Well since this class is NOT a subclass of AbstractTabbedModelBoundPopupView I can't do that otherwise I would have 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 s Well since this class is NOT a subclass of AbstractTabbedModelBoundPopupView I can't do that otherwise I would have 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 Well since this class is NOT a subclass of AbstractTabbedModelBoundPopupView I can't do that otherwise I would have 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 AbstractTa Well since this class is NOT a subclass of AbstractTabbedModelBoundPopupView I can't do that otherwise I would have Line 1873: return tabMap; Line 1874: } -- 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
