Alexander Wels has posted comments on this change. Change subject: webadmin : Introduce volume profile pdf Export option ......................................................................
Patch Set 6: (3 comments) http://gerrit.ovirt.org/#/c/28453/6/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/VolumeProfileStatisticsModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/VolumeProfileStatisticsModel.java: Line 292: if (!isBrickProfileSelected) { Line 293: url += ";nfsStatistics=true";//$NON-NLS-1$ Line 294: } Line 295: return url + ".pdf";//$NON-NLS-1$ Line 296: } I personally would write this like this: private String formProfileUrl(String clusterId, String volumeId, boolean isBrickProfileSelected) { String stats = isBrickProfileSelected ? ";nfsStatistics=true" : ""; //$NON-NLS-1 $NON-NLS-2$ return String.format("/ovirt-engine/api/clusters/%s/glustervolumes/%s/profilestatistics.pdf%s", clusterId, volumeId, stats); //$NON-NLS-1$ } Line 297: Line 298: public String getProfileExportUrl() { Line 299: return profileExportUrl; Line 300: } http://gerrit.ovirt.org/#/c/28453/6/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/VolumeProfileStatisticsPopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/VolumeProfileStatisticsPopupView.java: Line 33: import com.google.gwt.uibinder.client.UiBinder; Line 34: import com.google.gwt.uibinder.client.UiField; Line 35: import com.google.gwt.user.client.ui.Anchor; Line 36: import com.google.gwt.user.client.ui.Label; Line 37: import com.google.gwt.user.client.ui.VerticalPanel; Please don't use vertical panels. Use flow panels instead. I realize the rest of the popup uses Vertical and Horizontal panels, so you might be stuck with it, but in general you want to avoid them as much as possible as the actual generated HTML is a table, where you really only need a div. Line 38: import com.google.inject.Inject; Line 39: Line 40: public class VolumeProfileStatisticsPopupView extends AbstractModelBoundPopupView<VolumeProfileStatisticsModel> implements VolumeProfileStatisticsPopupPresenterWidget.ViewDef { Line 41: Line 129: Line 130: @UiField Line 131: @Ignore Line 132: @WithElementId Line 133: VerticalPanel nfsProfileAnchorContainer; Instead of exposing the containers, and dynamically adding the anchors, I would define 2 anchors in the ui binder (ui.xml) and have them both hidden. Then add them here instead of the panels. Then in the 'initAnchors' I would set the value of the anchor to the appropriate URL and show the one we are interested in. Line 134: Line 135: ApplicationConstants constants; Line 136: ApplicationResources resources; Line 137: -- To view, visit http://gerrit.ovirt.org/28453 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9791208b327dbe3e3b4dacfb568f08e042e8b8c9 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@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
