Alexander Wels has posted comments on this change. Change subject: userportal, webadmin: Number field reporting wrong error ......................................................................
Patch Set 6: (5 comments) http://gerrit.ovirt.org/#/c/37244/6/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/editor/UiCommonEditorVisitor.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/editor/UiCommonEditorVisitor.java: Line 64: @Override Line 65: public void onValueChange(ValueChangeEvent<T> event) { Line 66: // Set value in model Line 67: if (ctx.canSetInModel()) { Line 68: boolean editorValid = true; > this looks like a big change. Are we going from always setting values into We are going from always setting it to only setting it if the EDITOR is not marked invalid. This is to prevent the following scenario. 1. I enter an invalid value, click ok but some other field is invalid. 2. I change the value to be valid, click ok but some other field is invalid. 3. I change the value to be invalid again, if I always write the value (in this case it would be null because the editor cannot parse the value), then it will fire a change event, and it will wipe out what I entered it the editor. Which is a bad user experience since in step 1 it didn't wipe out the value. Line 69: if (event.getSource() instanceof EntityModelTextBox) { Line 70: editorValid = ((EntityModelTextBox<?>)event.getSource()).isStateValid(); Line 71: } Line 72: if (editorValid) { Line 92: public void onKeyPress(KeyPressEvent event) { Line 93: if (KeyCodes.KEY_ENTER == event.getNativeEvent().getKeyCode()) { Line 94: // Set value in model Line 95: if (ctx.canSetInModel()) { Line 96: boolean editorValid = true; > same as above same answer, just handling the enter key instead of the change event. Line 97: if (editor instanceof EntityModelTextBox) { Line 98: editorValid = ((EntityModelTextBox<?>)editor).isStateValid(); Line 99: } Line 100: if (editorValid) { Line 228: editor.markAsValid(); Line 229: } else { Line 230: //The validator will set the entities to be valid before running checks Line 231: //So there is no possibility to go from one error to another without this Line 232: //updating the error message. > sorry, don't quite understand your comment here The entity has its own validation that gets called, as part of that validation it always marks the entity valid before potentially marking it as invalid. This causes this method to run which then messes with the state of the editor even if we won't want it to mess with (for instance if the editor marked itself invalid). I updated the message to hopefully be clearer. Line 233: if (editor.isValid()) { Line 234: editor.markAsInvalid(model.getInvalidityReasons()); Line 235: } Line 236: } http://gerrit.ovirt.org/#/c/37244/6/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/AbstractValidatedWidgetWithLabel.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/AbstractValidatedWidgetWithLabel.java: Line 56: String contentWidget(); Line 57: } Line 58: Line 59: //We need to store the valid state of the editor so that when the model validator Line 60: //runs and the editor is not valid (due to a parsing error). The editor doesn't get > "error). The " --> error), the Stop nitpicking my English. Line 61: //reset by the model. Line 62: private boolean editorStateValid = true; Line 63: Line 64: private final W contentWidget; http://gerrit.ovirt.org/#/c/37244/6/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/generic/EntityModelTextBox.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/generic/EntityModelTextBox.java: Line 29: } Line 30: Line 31: /** Line 32: * Return the parsed value, or null if the field is empty or parsing fails. If the parsing fails Line 33: * fire a parsing failed event, so interested parties can handle it. > suggest tweaking this comment a bit. Looks like the EditorStateUpdateEvent Done Line 34: */ Line 35: @Override Line 36: public T getValue() { Line 37: T value = null; -- To view, visit http://gerrit.ovirt.org/37244 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3702cb370528a0baf6a176fa89cd736596a10ff2 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Greg Sheremeta <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[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
