Tomer Saban has posted comments on this change. Change subject: webadmin: Added place holder for watchdog model. ......................................................................
Patch Set 7: (11 comments) 'Done' replies were fixed on next commit. http://gerrit.ovirt.org/#/c/36688/7/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 1240: Line 1241: @Override Line 1242: public String getDisplayStringNullSafe(String data) { Line 1243: if (data == null || data.trim().isEmpty()) { Line 1244: data = ui_constants.NoWatchdog(); > please use commonApplicationConstants. no need to use costants from other l Using CommonApplicationConstants requires changes in another places in order to keep using the same constant from CommonApplicationConstants in other places where it's going to create recursive dependency in other files which should be avoided(CommonApplicationConstants can't be used from UnitVmModel.java. See the reply there). Line 1245: } Line 1246: Line 1247: return typeAheadNameTemplateNullSafe(data); Line 1248: } http://gerrit.ovirt.org/#/c/36688/7/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/AbstractVmPopupWidget.ui.xml File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/AbstractVmPopupWidget.ui.xml: Line 645 Line 646 Line 647 Line 648 Line 649 > unrealted Done http://gerrit.ovirt.org/#/c/36688/7/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/templates/TemplateListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/templates/TemplateListModel.java: Line 675 Line 676 Line 677 Line 678 Line 679 > unrelated Done http://gerrit.ovirt.org/#/c/36688/7/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 432: } Line 433: Line 434: private NotChangableForVmInPoolListModel<String> watchdogModel; Line 435: Line 436: public ListModel<String> getWatchdogModelEditor() { > why added Editor to getter and setter. Removed Line 437: return watchdogModel; Line 438: } Line 439: Line 440: private void setWatchdogModelEditor(NotChangableForVmInPoolListModel<String> watchdogModel) { Line 1856: if (sender == getWatchdogModelEditor()) Line 1857: { Line 1858: watchdogModelEditor_SelectedItemChanged(sender, args); Line 1859: } Line 1860: else if (sender == getVmInitEnabled()) > can you call watchdogModelEditor_SelectedItemChanged only once? (and rename Done Line 1861: { Line 1862: vmInitEnabledChanged(); Line 1863: } Line 1864: else if (sender == getMemSize()) Line 1891: if (getProvisioningClone_IsSelected().getEntity()) { Line 1892: getProvisioning().setEntity(true); Line 1893: } Line 1894: } else if (sender == getWatchdogModelEditor()) { Line 1895: WatchdogModel_EntityChanged(sender, args); > method starts with lowercase char Done Line 1896: } else if (sender == getIsHighlyAvailable()) { Line 1897: behavior.updateMigrationAvailability(); Line 1898: } else if (sender == getOverrideMigrationDowntime()) { Line 1899: overrideMigrationDowntimeChanged(); Line 1930: Line 1931: private void WatchdogModel_EntityChanged(Object sender, EventArgs args) { Line 1932: if (getWatchdogModelEditor().getEntity() == null) { Line 1933: getWatchdogAction().setIsChangable(false); Line 1934: getWatchdogAction().setSelectedItem(null); //$NON-NLS-1$ > remove //non-nls1... Done Line 1935: } else { Line 1936: getWatchdogAction().setIsChangable(true); Line 1937: } Line 1938: } Line 2022: } Line 2023: } Line 2024: Line 2025: private void watchdogModelEditor_SelectedItemChanged(Object sender, EventArgs args) { Line 2026: final UIConstants constants = ConstantsManager.getInstance().getConstants(); > remove Needed for getting NoWatchdog constant. It can't be taken from CommonApplicationConstants because it will create recursive dependency which should be avoided. Using CommonApplicationConstants is problematic. Line 2027: Line 2028: if (getWatchdogModelEditor().getSelectedItem() == null || Line 2029: ((String)getWatchdogModelEditor().getSelectedItem()).equals(constants.NoWatchdog())) { Line 2030: getWatchdogAction().setIsChangable(false); Line 2025: private void watchdogModelEditor_SelectedItemChanged(Object sender, EventArgs args) { Line 2026: final UIConstants constants = ConstantsManager.getInstance().getConstants(); Line 2027: Line 2028: if (getWatchdogModelEditor().getSelectedItem() == null || Line 2029: ((String)getWatchdogModelEditor().getSelectedItem()).equals(constants.NoWatchdog())) { > you should expect null. if not there a bug in the infrastructure (I believe Doesn't work when I remove the part that checks equality with the no watchdog. Maybe there really is a bug in the infrastructure(It actually being called 3 times and in the last one it the selected item is "<No Watchdog>". That's why it should be left as is. Line 2030: getWatchdogAction().setIsChangable(false); Line 2031: getWatchdogAction().setSelectedItem(null); Line 2032: } else { Line 2033: getWatchdogAction().setIsChangable(true); Line 2309: for (VmWatchdogType vmWatchdogType : vmWatchdogTypes) { Line 2310: watchDogModelsStrings.add(vmWatchdogType.toString()); Line 2311: } Line 2312: Line 2313: watchDogModelsStrings.add(0, constants.NoWatchdog()); > should be 'add(0, null)'. When I removed it, It also removed it from the list(We said <No Watchdog> should appear and not empty option. Therefore, I left it as is. Line 2314: getWatchdogModelEditor().setItems(watchDogModelsStrings); Line 2315: Line 2316: String old_watchdog_selected_string = getWatchdogModelEditor().getSelectedItem(); Line 2317: http://gerrit.ovirt.org/#/c/36688/7/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java: Line 2506: @DefaultStringValue("This field is not a valid Guid (use 0-9,A-F format: 00000000-0000-0000-0000-000000000000)") Line 2507: String invalidGuidMsg(); Line 2508: Line 2509: @DefaultStringValue("<No Watchdog>") Line 2510: String NoWatchdog(); > move to commonApplictaionConstants Needed here (See reply at AbstractVmPopupWidget for more information). Line 2511: -- To view, visit http://gerrit.ovirt.org/36688 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ice4111935a0ec3bb29237660702e536adbc34d14 Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tomer Saban <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Tomer Saban <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
