Alexander Wels has posted comments on this change. Change subject: userportal, webadmin: Number field reporting wrong error ......................................................................
Patch Set 8: (3 comments) https://gerrit.ovirt.org/#/c/37244/8/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 114: } Line 115: Line 116: public AbstractValidatedWidgetWithLabel(W contentWidget) { Line 117: this(contentWidget, new VisibilityRenderer.SimpleVisibilityRenderer()); Line 118: addStateUpdateHandler(); > I missed this in my first review - addStateUpdateHandler() is called due to Done Line 119: } Line 120: Line 121: @Override Line 122: protected void initWidget(Widget wrapperWidget) { Line 298: Line 299: @Override Line 300: public void markAsValid() { Line 301: if (editorStateValid) { Line 302: super.markAsValid(); > Just wondering, why put only super.markAsValid() inside "if (editorStateVal Because mark as valid can be called even if the lowest level check (aka is it an integer/number) fails. This returns a null to the model, and the validation is run, which in turn can call markAsValid(). However due to the integer failure it is not valid. So we don't want to change the validity identification in that particular case. Line 303: } Line 304: labelTooltip.setText(labelConfiguredTooltip); Line 305: labelTooltip.reconfigure(); Line 306: contentWidgetContainerTooltip.setText(contentWidgetContainerConfiguredTooltip); https://gerrit.ovirt.org/#/c/37244/8/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 34: * Return the parsed value, or null if the field is empty or parsing fails. If the validity of the box changes Line 35: * fire an {@code EditorStateUpdateEvent}, so interested parties can handle it. Line 36: */ Line 37: @Override Line 38: public T getValue() { > Hm, I'd suggest a small improvement here, up to you: It looks like the behavior of that is indentical (except that it lets ParseException bubble out of the method, which gets eaten somewhere else anyway). But IMO, my version is somewhat easier to understand the intent of the code, so I will leave it as is. Line 39: T value = null; Line 40: boolean originalValidState = isValid; Line 41: try { Line 42: value = getValueOrThrow(); -- To view, visit https://gerrit.ovirt.org/37244 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3702cb370528a0baf6a176fa89cd736596a10ff2 Gerrit-PatchSet: 8 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: Jenkins CI Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
