Alona Kaplan has posted comments on this change.
Change subject: foreman integration - showing foreman hosts in new host dialog
......................................................................
Patch Set 1: (14 inline comments)
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/HostListModel.java
Line 652: }
Line 653: };
Line 654: AsyncDataProvider.GetDataCenterList(_asyncQuery);
Line 655:
Line 656: hostModel.getExternalHostProviderEnabled().setEntity(false);
All the initialization code of ExternalHost resides in the HostModel. I would
move also this code to HostModel to avoid confusion.
Line 657: hostModel.getHostName().setIsEmpty(true);
Line 658: }
Line 659:
Line 660: private void GoToEventsTab()
Line 653: };
Line 654: AsyncDataProvider.GetDataCenterList(_asyncQuery);
Line 655:
Line 656: hostModel.getExternalHostProviderEnabled().setEntity(false);
Line 657: hostModel.getHostName().setIsEmpty(true);
Why do you need to use empty property? You have to initialize it manually. I
think it is better to check hostModel.getHostName().getItems != null to check
if the list is empty.
Line 658: }
Line 659:
Line 660: private void GoToEventsTab()
Line 661: {
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/HostModel.java
Line 144: }
Line 145:
Line 146: private ListModel privateHostName;
Line 147:
Line 148: public ListModel getHostName()
"HostName" is confusing here. Maybe change to "ExternalHostName"
Line 149: {
Line 150: return privateHostName;
Line 151: }
Line 152:
Line 668: public void eventRaised(Event ev, Object sender,
EventArgs args) {
Line 669: UpdateExternalHostModels();
Line 670: }
Line 671: };
Line 672: getExternalHostProviderEnabled().setEntity(true);
In HostListModel.New() you set the value by default to false.
So, as I understand, the code here is redundant.
Line 673:
getExternalHostProviderEnabled().getEntityChangedEvent().addListener(externalHostsListener);
Line 674: } else {
Line 675: getExternalHostProviderEnabled().setIsAvailable(false);
Line 676: getHostName().setIsAvailable(false);
Line 1062: getHostsQuery.asyncCallback = new INewAsyncCallback() {
Line 1063: @Override
Line 1064: public void OnSuccess(Object model, Object result)
Line 1065: {
Line 1066: HostModel hostListModel = (HostModel) model;
You don't need to pass the model as a parameter, you can use HostModel.this.
And it is not a hostListModel is is a hostModel.
Line 1067:
Line 1068: ArrayList<VDS> hosts = (ArrayList<VDS>) result;
Line 1069: ListModel hostNameListModel =
hostListModel.getHostName();
Line 1070: hostNameListModel.setItems(hosts);
Line 1067:
Line 1068: ArrayList<VDS> hosts = (ArrayList<VDS>) result;
Line 1069: ListModel hostNameListModel =
hostListModel.getHostName();
Line 1070: hostNameListModel.setItems(hosts);
Line 1071: hostNameListModel.setSelectedItem(null);
Do you want to have the "null" value in the external host list permanently or
just as a default selection?
If you want to have it in the list you have to add it to the items.
Line 1072: hostNameListModel.setIsSelectable(true);
Line 1073: hostNameListModel.setIsChangable(true);
Line 1074: hostNameListModel.setIsEmpty(false);
Line 1075: getName().setEntity("");
Line 1068: ArrayList<VDS> hosts = (ArrayList<VDS>) result;
Line 1069: ListModel hostNameListModel =
hostListModel.getHostName();
Line 1070: hostNameListModel.setItems(hosts);
Line 1071: hostNameListModel.setSelectedItem(null);
Line 1072: hostNameListModel.setIsSelectable(true);
Redundant- it is selectable by default
Line 1073: hostNameListModel.setIsChangable(true);
Line 1074: hostNameListModel.setIsEmpty(false);
Line 1075: getName().setEntity("");
Line 1076: getHost().setEntity("");
Line 1070: hostNameListModel.setItems(hosts);
Line 1071: hostNameListModel.setSelectedItem(null);
Line 1072: hostNameListModel.setIsSelectable(true);
Line 1073: hostNameListModel.setIsChangable(true);
Line 1074: hostNameListModel.setIsEmpty(false);
See the comment about empty property in HostListModel.
Line 1075: getName().setEntity("");
Line 1076: getHost().setEntity("");
Line 1077: }
Line 1078: };
Line 1071: hostNameListModel.setSelectedItem(null);
Line 1072: hostNameListModel.setIsSelectable(true);
Line 1073: hostNameListModel.setIsChangable(true);
Line 1074: hostNameListModel.setIsEmpty(false);
Line 1075: getName().setEntity("");
Redundant- will happen in HostName_SelectedItemChanged.
Line 1076: getHost().setEntity("");
Line 1077: }
Line 1078: };
Line 1079:
AsyncDataProvider.GetExternalProviderHostList(getHostsQuery);
Line 1072: hostNameListModel.setIsSelectable(true);
Line 1073: hostNameListModel.setIsChangable(true);
Line 1074: hostNameListModel.setIsEmpty(false);
Line 1075: getName().setEntity("");
Line 1076: getHost().setEntity("");
same
Line 1077: }
Line 1078: };
Line 1079:
AsyncDataProvider.GetExternalProviderHostList(getHostsQuery);
Line 1080: } else {
Line 1077: }
Line 1078: };
Line 1079:
AsyncDataProvider.GetExternalProviderHostList(getHostsQuery);
Line 1080: } else {
Line 1081: getHostName().setIsSelectable(enabled);
setting isChangable is enough.
Line 1082: getHostName().setIsChangable(enabled);
Line 1083: getHostName().setSelectedItem(null);
Line 1084: getName().setEntity("");
Line 1085: getHost().setEntity("");
Line 1080: } else {
Line 1081: getHostName().setIsSelectable(enabled);
Line 1082: getHostName().setIsChangable(enabled);
Line 1083: getHostName().setSelectedItem(null);
Line 1084: getName().setEntity("");
Redundant- will happen in HostName_SelectedItemChanged.
Line 1085: getHost().setEntity("");
Line 1086: }
Line 1087: }
Line 1088:
Line 1081: getHostName().setIsSelectable(enabled);
Line 1082: getHostName().setIsChangable(enabled);
Line 1083: getHostName().setSelectedItem(null);
Line 1084: getName().setEntity("");
Line 1085: getHost().setEntity("");
same
Line 1086: }
Line 1087: }
Line 1088:
Line 1089: private void UpdatePmModels()
Line 1118: public void OnSuccess(Object model, Object
returnValue) {
Line 1119:
Line 1120: List<String> pmOptions = (ArrayList<String>)
returnValue;
Line 1121:
Line 1122: if (pmOptions != null) {
How is it related to your patch?
Line 1123:
getPmPort().setIsAvailable(pmOptions.contains(PmPortKey));
Line 1124:
getPmSlot().setIsAvailable(pmOptions.contains(PmSlotKey));
Line 1125:
getPmSecure().setIsAvailable(pmOptions.contains(PmSecureKey));
Line 1126: }
--
To view, visit http://gerrit.ovirt.org/14582
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I30ea180e477a8f0714c4dc97ec55f29be515723a
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Alona Kaplan <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Oved Ourfali <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
