anmolbabu has posted comments on this change. Change subject: webadmin : Handle multi-tab validation error ......................................................................
Patch Set 1: (2 comments) https://gerrit.ovirt.org/#/c/40680/1/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotCreatePopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotCreatePopupView.java: Line 264: Line 265: @Override Line 266: public void setMessage(String msg) { Line 267: super.setMessage(msg); Line 268: errorMsgLabel.setText(msg); > Do we still need this? Done. But even if I use the below approach it would also be setting the message to the message label in here. both tabs have specific labels as Shubhendu opines that it is better to show errors in a label apart from showing them on hover of invalidated editors. Line 269: } Line 270: Line 271: @Override Line 272: public void handleValidationErrors(GlusterVolumeSnapshotModel object) { Line 270: Line 271: @Override Line 272: public void handleValidationErrors(GlusterVolumeSnapshotModel object) { Line 273: StringBuilder generalTabErrorBuilder = new StringBuilder(); Line 274: if (!snapshotNameEditor.isValid()){ > Why do we handle this as special one? 1. Validation logic not generic for all widgets in the sense that, we have the IValidation implementations defined for snapshotNameEditor only So, here we can simply do editor.validate() which will take the responsibility of doing the following : a. Mark the widget as invalid(make widgets container red border(default as in super implementations of all widgets) or as defined in the widget) b. Put a hover text for that specific widget as contained in <corresponding_entity_model>.getInvalidityReasons() but all widgets don't have such a validation the reason being consider i. snapshotDescriptionEditor, its an optional field ii. clusterNameEditor and volumeNameEditor auto- populated labels. Validity of i and ii above will always be true as they are never marked invalid iii. daysOfMonthEditor, executionTimeEditor, daysOfWeekEditor The validity/invalidity of these would depend on their own state along with the state of other editors like recurrenceEditor. So here I feel it might not be nice to introduce cross editor dependencies (in our IValidation implementations) and may be it is better to handle it by ourselves. 2. we don't have tab wise groupings of widgets. I feel it would be nice to have(but this would be only residing here in this class but I would be extremely happy if this grouping logic can be generic and framework level instead of doing it in here and also I am not really sure if its really required only to check validity of widgets) a tab- wise grouping of widgets, so that we can just iterate over each group and handle the tab group switching generically (only tab wise specific but iteration on a group would be generic). So, the only way is to specifically check each widgets validity Line 275: appendErrors(Collections.singletonList(constants.volumeSnapshotNamePrefixLabel()), generalTabErrorBuilder); Line 276: appendErrors(object.getSnapshotName().getInvalidityReasons(), generalTabErrorBuilder); Line 277: } Line 278: -- To view, visit https://gerrit.ovirt.org/40680 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I88c05aa369758ce174b0dc85f83a253e608d6c0e Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
