Vojtech Szocs has posted comments on this change. Change subject: webadmin: auto select dialog tab ......................................................................
Patch Set 2: (4 comments) 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(); > IMO the view should be the only place that actually contains the mapping. I Yup, agreed. 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))) { 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(); > If you notice the map is private, and the only access to it is through getT Yeah, well, I was just referring to a simpler API, i.e. if populateTabMap had tabMap argument, it wouldn't be necessary to have getTabNameMapping method. On the other hand, I understand that getTabNameMapping method here is due to implementing AbstractTabbedModelBoundPopupPresenterWidget.ViewDef which declares it. So my original suggestion is not really valid in broader context. 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>(); > Well since this class is NOT a subclass of AbstractTabbedModelBoundPopupVie I see, thanks for clarification. Line 773: Line 774: @SuppressWarnings({ "unchecked", "rawtypes" }) Line 775: public AbstractVmPopupWidget(CommonApplicationConstants constants, Line 776: CommonApplicationResources resources, 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 { > I decided to turn this into an enum so I don't have to put the enums in eac Sounds good to me. -- 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
