Alona Kaplan has posted comments on this change. Change subject: webadmin: removed caching from EnumTranslator. ......................................................................
Patch Set 2: (20 comments) Well done! :) Just few small comments. Please note the comment in EnumTranslator, it can cause bugs. http://gerrit.ovirt.org/#/c/26048/2/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/renderer/VolumeTransportTypeRenderer.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/renderer/VolumeTransportTypeRenderer.java: Line 16: } Line 17: Line 18: @Override Line 19: public String render(Set<TransportType> transportTypes) { Line 20: Translator transportTypeTranslator = EnumTranslator.getInstance(); Please use EnumTranslator.getInstance() directrly. No need for a variable. Line 21: StringBuilder transportTypesBuilder = new StringBuilder(); Line 22: Iterator<TransportType> iterator = transportTypes.iterator(); Line 23: while (iterator.hasNext()) { Line 24: TransportType transportType = iterator.next(); http://gerrit.ovirt.org/#/c/26048/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Linq.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/Linq.java: Line 1289: private static final long serialVersionUID = -7917198421355959306L; Line 1290: Line 1291: @Override Line 1292: public int compare(ProviderType type1, ProviderType type2) { Line 1293: final EnumTranslator enumTranslator = EnumTranslator.getInstance(); Please use EnumTranslator.getInstance() directrly. No need for a variable. Line 1294: return LexoNumericComparator.comp(enumTranslator.get(type1), enumTranslator.get(type2)); Line 1295: } Line 1296: } Line 1297: http://gerrit.ovirt.org/#/c/26048/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/pools/PoolGeneralModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/pools/PoolGeneralModel.java: Line 439: Line 440: poolGeneralModel.setDefinedMemory(getvm().getVmMemSizeMb() + " MB"); //$NON-NLS-1$ Line 441: poolGeneralModel.setMinAllocatedMemory(getvm().getMinAllocatedMem() + " MB"); //$NON-NLS-1$ Line 442: Line 443: Translator translator = EnumTranslator.getInstance(); Please use EnumTranslator.getInstance() directrly. No need for a variable. Line 444: poolGeneralModel.setDefaultDisplayType(translator.get(getvm().getDefaultDisplayType())); Line 445: poolGeneralModel.setOrigin(translator.get(getvm().getOrigin())); Line 446: poolGeneralModel.setUsbPolicy(translator.get(getvm().getUsbPolicy())); Line 447: http://gerrit.ovirt.org/#/c/26048/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/providers/NeutronPluginTranslator.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/providers/NeutronPluginTranslator.java: Line 14: Line 15: private static final List<String> displayStrings; Line 16: private static final Map<String, OpenstackNetworkPluginType> pluginForDisplay; Line 17: Line 18: private NeutronPluginTranslator() { } It is not in the scope of this patch. Line 19: Line 20: static { Line 21: pluginForDisplay = new HashMap<String, OpenstackNetworkPluginType>(); Line 22: displayStrings = new ArrayList<String>(); http://gerrit.ovirt.org/#/c/26048/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/templates/TemplateGeneralModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/templates/TemplateGeneralModel.java: Line 354: template.getCpuPerSocket())); Line 355: Line 356: setOS(AsyncDataProvider.getOsName(template.getOsId())); Line 357: Line 358: Translator translator = EnumTranslator.getInstance(); Please use EnumTranslator.getInstance() directrly. No need for a variable. Line 359: setDefaultDisplayType(translator.get(template.getDefaultDisplayType())); Line 360: Line 361: setOrigin(translator.get(template.getOrigin())); Line 362: http://gerrit.ovirt.org/#/c/26048/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/users/UserEventNotifierListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/users/UserEventNotifierListModel.java: Line 110: ApplicationModeHelper.getModeSpecificEventNotificationTypeList(); Line 111: Map<EventNotificationEntity, HashSet<AuditLogType>> availableEvents = Line 112: AsyncDataProvider.getAvailableNotificationEvents(); Line 113: Line 114: Translator translator = EnumTranslator.getInstance(); Please use EnumTranslator.getInstance() directrly. No need for a variable. Line 115: Line 116: ArrayList<SelectionTreeNodeModel> list = new ArrayList<SelectionTreeNodeModel>(); Line 117: Line 118: ArrayList<event_subscriber> items = http://gerrit.ovirt.org/#/c/26048/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 2029: watchDogModels.add(EnumTranslator.getInstance().get(vmWatchdogType)); Line 2030: } Line 2031: Line 2032: watchDogModels.add(0, null); Line 2033: String oldWatchdogSelected = getWatchdogModel().getSelectedItem(); Please don't include in the patch things that are not related to the subject of the patch. Line 2034: getWatchdogModel().setItems(watchDogModels); Line 2035: Line 2036: if (watchDogModels.contains(oldWatchdogSelected)) { Line 2037: getWatchdogModel().setSelectedItem(oldWatchdogSelected); http://gerrit.ovirt.org/#/c/26048/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmGeneralModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmGeneralModel.java: Line 522: } Line 523: } Line 524: Line 525: @Override Line 526: protected void entityPropertyChanged(Object sender, PropertyChangedEventArgs e) { Same here. Not in the scope of the patch. You can create a new cleanup patch for all these small changes. Line 527: super.entityPropertyChanged(sender, e); Line 528: Line 529: updateProperties(); Line 530: } Line 528: Line 529: updateProperties(); Line 530: } Line 531: Line 532: private void updateProperties() { same Line 533: VM vm = (VM) getEntity(); Line 534: Line 535: setName(vm.getName()); Line 536: setDescription(vm.getVmDescription()); Line 580: setVmId(vm.getId().toString()); Line 581: setFqdn(vm.getVmFQDN()); Line 582: Line 583: setHasAlert(vm.getVmPauseStatus() != VmPauseStatus.NONE && vm.getVmPauseStatus() != VmPauseStatus.NOERR); Line 584: if (getHasAlert()) { same Line 585: setAlert(translator.get(vm.getVmPauseStatus())); Line 586: } else { Line 587: setAlert(null); Line 588: } Line 582: Line 583: setHasAlert(vm.getVmPauseStatus() != VmPauseStatus.NONE && vm.getVmPauseStatus() != VmPauseStatus.NOERR); Line 584: if (getHasAlert()) { Line 585: setAlert(translator.get(vm.getVmPauseStatus())); Line 586: } else { same Line 587: setAlert(null); Line 588: } Line 589: Line 590: setHasDefaultHost(vm.getDedicatedVmForVds() != null); Line 587: setAlert(null); Line 588: } Line 589: Line 590: setHasDefaultHost(vm.getDedicatedVmForVds() != null); Line 591: if (getHasDefaultHost()) { same Line 592: Frontend.getInstance().runQuery(VdcQueryType.Search, new SearchParameters("Host: cluster = " + vm.getVdsGroupName() //$NON-NLS-1$ Line 593: + " sortby name", SearchType.VDS), new AsyncQuery(this, //$NON-NLS-1$ Line 594: new INewAsyncCallback() { Line 595: @Override Line 600: if (localVm == null) Line 601: { Line 602: return; Line 603: } Line 604: ArrayList<VDS> hosts = ((VdcQueryReturnValue) returnValue).getReturnValue(); same Line 605: for (VDS host : hosts) Line 606: { Line 607: if (localVm.getDedicatedVmForVds() != null Line 608: && host.getId().equals(localVm.getDedicatedVmForVds())) Line 613: } Line 614: Line 615: } Line 616: })); Line 617: } else { same Line 618: setDefaultHost(ConstantsManager.getInstance().getConstants().anyHostInCluster()); Line 619: } Line 620: } Line 621: http://gerrit.ovirt.org/#/c/26048/2/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/EnumTranslator.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/EnumTranslator.java: Line 20: Line 21: @Override Line 22: public String get(Enum<?> key) { Line 23: if(key == null) { Line 24: throw new IllegalArgumentException(); This can be problematic. createAndTranslate(..) returned constants.notAvailableLabel() for null object. while get(...) returned null. You have to make sure no one of the usages relies on this behaviour. Line 25: } Line 26: Line 27: try { Line 28: //FIXME: hack: due to java restriction for method names with chars that are not letters, digits, and underscores, replace . with 0 http://gerrit.ovirt.org/#/c/26048/2/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/MainTabStorageView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/MainTabStorageView.java: Line 100 Line 101 Line 102 Line 103 Line 104 Not in the scope of the patch. Line 109 Line 110 Line 111 Line 112 Line 113 same http://gerrit.ovirt.org/#/c/26048/2/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/gluster/SubTabVolumeGeneralView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/gluster/SubTabVolumeGeneralView.java: Line 83: translateVolumeType((GlusterVolumeEntity) model.getEntity()); Line 84: } Line 85: } Line 86: Line 87: private void translateVolumeType(GlusterVolumeEntity volumeEntity) { Why did you move the method? Line 88: Translator translator = EnumTranslator.getInstance(); Line 89: if (translator.containsKey(volumeEntity.getVolumeType())) { Line 90: getDetailModel().setVolumeTypeSilently(translator.get(volumeEntity.getVolumeType())); Line 91: } Line 84: } Line 85: } Line 86: Line 87: private void translateVolumeType(GlusterVolumeEntity volumeEntity) { Line 88: Translator translator = EnumTranslator.getInstance(); No need for a variable. Use EnumTranslator.getInstance() directly. Line 89: if (translator.containsKey(volumeEntity.getVolumeType())) { Line 90: getDetailModel().setVolumeTypeSilently(translator.get(volumeEntity.getVolumeType())); Line 91: } Line 92: } http://gerrit.ovirt.org/#/c/26048/2/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/table/column/VmStatusCell.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/table/column/VmStatusCell.java: Line 131: AbstractImagePrototype imagePrototype = AbstractImagePrototype.create(alertImageResource); Line 132: String html = imagePrototype.getHTML(); Line 133: Line 134: // Append tooltip Line 135: Translator translator = EnumTranslator.getInstance(); No need for a variable. Use EnumTranslator.getInstance() directly. Line 136: String toolTip = translator.get(vm.getVmPauseStatus()); Line 137: html = html.replaceFirst("img", "img " + "title='" + toolTip + "' "); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$ Line 138: Line 139: return SafeHtmlUtils.fromTrustedString(html); -- To view, visit http://gerrit.ovirt.org/26048 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3df424d8e7a35ed249b2760da79deba7db31b785 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@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
