Vojtech Szocs has posted comments on this change. Change subject: webadmin: qos parent sub tab ......................................................................
Patch Set 2: Code-Review+2 (4 comments) This patch is very simple, which is a good thing. I have some inline comments but nothing that would prevent this patch from being merged as-is. I wrote them just to clarify some technical details. Giving +2 score, feel free to merge (but please make sure to test the special "reveal-tab-container-directly" case I mentioned in one of my comments). http://gerrit.ovirt.org/#/c/30655/2/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/tab/HyperlinkTab.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/tab/HyperlinkTab.java: Line 24: initWidget(WidgetUiBinder.uiBinder.createAndBindUi(this)); Line 25: } Line 26: Line 27: @Override Line 28: public void setAlign(Align align) { Hm, maybe add some Java comment to explain no-op implementation, for example: // This tab widget doesn't support horizontal alignment Line 29: } Line 30: Line 31: @Override Line 32: public void activate() { Line 29: } Line 30: Line 31: @Override Line 32: public void activate() { Line 33: label.getElement().getStyle().setFontWeight(FontWeight.BOLD); An alternative way to style elements is following: public interface Style extends CssResource { String active(); String inactive(); } @UiField public Style style; // inside activate() method label.getElement().replaceClassName(style.inactiveLeft(), style.activeLeft()); // inside deactivate() method label.getElement().replaceClassName(style.activeLeft(), style.inactiveLeft()); and in HyperlinkTab.ui.xml template: <ui:style type="org.ovirt.engine.ui.common.widget.tab.HyperlinkTab.Style"> .active { font-weight: bold; } .inactive { font-weight: normal; } </ui:style> The above approach is more complex but allows you to centralize CSS styling into ui.xml template (instead of putting styling code in Java class). However, for this very simple case, I am OK with current change :-) so above approach is just for information. Line 34: } Line 35: Line 36: @Override Line 37: public void deactivate() { http://gerrit.ovirt.org/#/c/30655/2/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/tab/datacenter/DataCenterQosSubTabPanelPresenter.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/tab/datacenter/DataCenterQosSubTabPanelPresenter.java: Line 31: AbstractSubTabPanelPresenter<DataCenterQosSubTabPanelPresenter.ViewDef, DataCenterQosSubTabPanelPresenter.ProxyDef> { Line 32: Line 33: @ProxyCodeSplit Line 34: @NameToken(ApplicationPlaces.dataCenterQosSubTabPlace) Line 35: public interface ProxyDef extends TabContentProxyPlace<DataCenterQosSubTabPanelPresenter> { In GWTP framework, specific-tab presenters (like MainTabDataCenterPresenter) are contained within tab-container presenters (like MainTabPanelPresenter). It usually doesn't make sense for tab-container presenter to have a Place associated, because what you typically want is to reveal specific-tab presenter (which reveals itself into parent tab-container presenter, etc. until the root presenter is reached). In other words, when you reveal only tab-container presenter, any presenters under it will not be revealed, which is generally not what we want. In general, we always want to have *some* specific-tab presenter revealed, by default the first tab, for example. Due to bottom-up presenter reveal process, when you reveal specific-tab presenter, it also reveals its tab-container presenter, all the way up to root presenter. If you look in MainTabPanelPresenter (for example), you will see: @ProxyCodeSplit public interface ProxyDef extends Proxy<MainTabPanelPresenter> { } Note that it just extends Proxy, not TabContentProxyPlace. You simply reveal MainTabPanelPresenter by revealing any (specific-tab) presenter "below". In general, only "leaf" presenters should have Place associated. Tab-container presenter is not a "leaf" presenter, it is an "intermediate" between specific-tab presenter and root presenter. OK so much for the GWTP (boring) theory :-D I am putting my teacher glasses down now. I am OK with current change if it works and you are happy with it. I see no real harm in DataCenterQosSubTabPanelPresenter itself having a Place associated, but then you need to work around an issue where the user can reveal DataCenterQosSubTabPanelPresenter directly -- try loading URL like this: WebAdmin.html#<ApplicationPlaces.dataCenterQosSubTabPlace> If loading above URL also reveals specific StorageQoS & NetworkQoS tabs underneath, you are OK. Since I don't want to block this patch from proceeding, I can try to improve it in future if needed. With your approval, of course :-) Line 36: } Line 37: Line 38: public interface ViewDef extends AbstractSubTabPanelPresenter.ViewDef, DynamicTabPanel { Line 39: } Line 62: return new ModelBoundTabData(applicationConstants.dataCenterQosSubTabLabel(), 2, modelProvider); Line 63: } Line 64: Line 65: @Override Line 66: public void setInSlot(Object slot, PresenterWidget<?> content) { Overall, this override is quite creative yet simple, nice job. Line 67: super.setInSlot(slot, content); Line 68: if (content instanceof SubTabDataCenterStorageQosPresenter Line 69: || content instanceof SubTabDataCenterNetworkQoSPresenter) { Line 70: lastPresenter = (Presenter<?, ?>) content; -- To view, visit http://gerrit.ovirt.org/30655 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6d5e2de2d979bdaab498922acd6a580bc89dc19a Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@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
