Lior Vernia has posted comments on this change.
Change subject: webadmin: network profiles
......................................................................
Patch Set 23: (14 inline comments)
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/profiles/EditVnicProfileModel.java
Line 12: private VnicProfile profile;
Line 13:
Line 14: public EditVnicProfileModel(EntityModel sourceModel, Version
dcCompatibilityVersion, VnicProfile profile) {
Line 15: super(sourceModel, dcCompatibilityVersion);
Line 16:
setTitle(ConstantsManager.getInstance().getConstants().vnicProfileTitle());
Same title as in New, you can remove both and put once in the base class.
Line 17: setHashName("edit_vnic_profile"); //$NON-NLS-1$
Line 18:
Line 19: this.profile = profile;
Line 20:
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/profiles/VnicProfileModel.java
Line 227: }
Line 228: }
Line 229: }));
Line 230: } else {
Line 231: getCustomPropertySheet().setIsChangable(false);
Maybe make it not available instead of not changeable when it's not supported?
Line 232: }
Line 233: }
Line 234:
Line 235: public boolean validate()
Line 235: public boolean validate()
Line 236: {
Line 237: getName().validateEntity(new IValidation[] { new
NotEmptyValidation(), new I18NNameValidation() });
Line 238:
Line 239: return getName().getIsValid() &&
getCustomPropertySheet().validate();
You may prefer to first do getCustomPropertySheet().validate() and only then
return getCustomPropertySheet().getIsValid(), so both validation errors, if
they exist, will show at the same time.
Line 240: }
Line 241:
Line 242: protected abstract VnicProfile getProfile();
Line 243:
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/RemoveVnicProfileModel.java
Line 19: public class RemoveVnicProfileModel extends ConfirmationModel {
Line 20:
Line 21: private final List<VnicProfile> profiles;
Line 22: private final boolean fullMsg;
Line 23: private final ListModel sourceListModel;
If you wanna implement immediate refresh as proposed below, this should be
SearchableListModel.
Line 24:
Line 25: public RemoveVnicProfileModel(ListModel sourceListModel,
List<VnicProfile> profiles, boolean isFullMsg) {
Line 26:
setTitle(ConstantsManager.getInstance().getConstants().removeVnicProfileTitle());
Line 27: setHashName("remove_vnic_prfoile"); //$NON-NLS-1$
Line 68: }
Line 69:
Line 70: startProgress(null);
Line 71:
Line 72: Frontend.RunMultipleAction(getActionType(), list,
Consider using RunMultipleActions() - notice the extra "s" - which only returns
after actual execution and not after canDoAction(), and then to execute the
getSearchCommand() of the sourceListModel, to refresh the display immediately.
Line 73: new IFrontendMultipleActionAsyncCallback() {
Line 74: @Override
Line 75: public void
executed(FrontendMultipleActionAsyncResult result) {
Line 76:
Line 90: protected VdcActionParametersBase
getRemoveVnicProfileParams(VnicProfile profile) {
Line 91: return new VnicProfileParameters(profile);
Line 92: }
Line 93:
Line 94: protected VdcActionType getActionType() {
I don't think that's needed here, there's no derived classes with different
action type.
Line 95: return VdcActionType.RemoveVnicProfile;
Line 96: }
Line 97:
Line 98: public List<VnicProfile> getProfiles() {
....................................................
File
frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java
Line 279:
Line 280: @DefaultStringValue("Remove Network Interface(s)")
Line 281: String removeNetworkInterfacesTitle();
Line 282:
Line 283: @DefaultStringValue("Remove VM Interfaces Profile(s)")
Interfaces --> Interface
Line 284: String removeVnicProfileTitle();
Line 285:
Line 286: @DefaultStringValue("Copy Template")
Line 287: String copyTemplateTitle();
Line 2048: String cloudInitAttachmentTypeBase64();
Line 2049:
Line 2050: @DefaultStringValue("Content must be Base64")
Line 2051: String cloudInitBase64Message();
Line 2052:
Whitespace.
Line 2053: @DefaultStringValue("VNIC Profile")
Line 2054: String vnicProfileTitle();
Line 2049:
Line 2050: @DefaultStringValue("Content must be Base64")
Line 2051: String cloudInitBase64Message();
Line 2052:
Line 2053: @DefaultStringValue("VNIC Profile")
"VM Interface Profile"? For consistency?
Line 2054: String vnicProfileTitle();
....................................................
File
frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIMessages.java
Line 229:
Line 230: @DefaultMessage("Vnic {0} from VM {1}")
Line 231: String vnicFromVm(String vnic, String vm);
Line 232:
Line 233: @DefaultMessage("Vnic Profile {0} from Network {1}")
If you're gonna use VNIC (instead of "VM Interface", which I think is okay for
the sake of not being too verbose), then it should either be all capital
letters as an acronym, or something like vNIC (which is more of the VMware
style).
Line 234: String vnicProfileFromNetwork(String vnicProfile, String network);
Line 235:
Line 236: @DefaultMessage("Vnic {0} from Template {1}")
Line 237: String vnicFromTemplate(String vnic, String template);
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/gin/uicommon/NetworkModule.java
Line 111: @Provides
Line 112: @Singleton
Line 113: public SearchableDetailModelProvider<VnicProfile,
NetworkListModel, NetworkProfileListModel>
getNetworkProfileListProvider(EventBus eventBus,
Line 114: Provider<DefaultConfirmationPopupPresenterWidget>
defaultConfirmPopupProvider,
Line 115: final Provider<VnicProfilePopupPresenterWidget>
newNetworkPopupProvider,
The variable names here are confusing...
Line 116: final Provider<VnicProfilePopupPresenterWidget>
editNetworkPopupProvider,
Line 117: final Provider<RemoveConfirmationPopupPresenterWidget>
removeConfirmPopupProvider) {
Line 118: return new SearchableDetailTabModelProvider<VnicProfile,
NetworkListModel, NetworkProfileListModel>(eventBus,
Line 119: defaultConfirmPopupProvider,
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/profile/VnicProfilePopupView.java
Line 112:
Line 113: initCustomPropertySheet(profile);
Line 114: }
Line 115:
Line 116: private void initCustomPropertySheet(final VnicProfileModel
profile) {
Consider moving this handler code to the presenter's onBind() method as is
customary.
Line 117:
profile.getCustomPropertySheet().getKeyValueLines().getItemsChangedEvent().addListener(new
IEventListener() {
Line 118:
Line 119: @Override
Line 120: public void eventRaised(Event ev, Object sender,
EventArgs args) {
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/profile/VnicProfilePopupView.ui.xml
Line 37: <d:content>
Line 38: <g:FlowPanel>
Line 39: <e:ListModelListBoxEditor
ui:field="networkEditor" />
Line 40: <e:EntityModelTextBoxEditor
ui:field="nameEditor" />
Line 41: <g:SimplePanel width="100%">
Please fix indentation here and in the rows below.
Line 42:
<e:EntityModelCheckBoxEditor ui:field="portMirroringEditor" />
Line 43: </g:SimplePanel>
Line 44: <g:FlowPanel>
Line 45: <k:KeyValueWidget
ui:field="customPropertiesSheetEditor"/>
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/network/ProfileInfoPanel.java
Line 8: import com.google.gwt.dom.client.Style.Unit;
Line 9: import com.google.gwt.user.client.ui.ScrollPanel;
Line 10: import com.google.gwt.user.client.ui.TabLayoutPanel;
Line 11:
Line 12: public class ProfileInfoPanel extends TabLayoutPanel {
As far as I can see this class is no longer used.
Line 13:
Line 14: private
PermissionWithInheritedPermissionListModelTable<PermissionListModel>
permissionsTable;
Line 15: private ApplicationConstants constants;
Line 16:
--
To view, visit http://gerrit.ovirt.org/16924
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icedb35f0e9277663a3b4525574c3b0dbbd2086db
Gerrit-PatchSet: 23
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alona Kaplan <alkap...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Lior Vernia <lver...@redhat.com>
Gerrit-Reviewer: Moti Asayag <masa...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
