Tomas Jelinek has posted comments on this change.
Change subject: frontend: Select an empty profile does not work
......................................................................
Patch Set 4:
(6 comments)
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/BaseListModelSuggestBox.java
Line 159: public HandlerRegistration addValueChangeHandler(final
ValueChangeHandler<T> handler) {
Line 160: return asSuggestBox().addValueChangeHandler(new
ValueChangeHandler<String>() {
Line 161: @Override
Line 162: public void onValueChange(ValueChangeEvent<String> event)
{
Line 163: try {
The throwing of the exception is the contract of the asEntity method defined
and documented here, therefore it is correct to handle it here. The fact that
not all the clients support the "illegal input" is not a reason to not provide
a generic solution in the upper class.
Line 164: T value = asEntity(event.getValue());
Line 165: handler.onValueChange(new
ValueChangeEvent<T>(value) {});
Line 166: } catch (IllegalArgumentException e) {
Line 167: // ignore - the user entered an incorrect string.
Just do not notify the listeners
Line 161: @Override
Line 162: public void onValueChange(ValueChangeEvent<String> event)
{
Line 163: try {
Line 164: T value = asEntity(event.getValue());
Line 165: handler.onValueChange(new
ValueChangeEvent<T>(value) {});
I see your point (and I've seen it also that time). But there was no way to get
to agreement because it was a philosophical difference so I have decided to go
with a third opinion of a frontend maintainer who did agree with that solution
(and gave +2) so I have merged it.
The main difference is that I believe that the upper class can provide generic
logic even not all the children need it as soon as the children are not forced
to use it. Especially if the other possibility is code duplication or more
complex code just to make sure that the ListModelSuggestBox can not use this
feature now or in the future.
Line 166: } catch (IllegalArgumentException e) {
Line 167: // ignore - the user entered an incorrect string.
Just do not notify the listeners
Line 168: }
Line 169: }
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListModelTypeAheadListBox.java
Line 223: }
Line 224:
Line 225: @Override
Line 226: protected T asEntity(String provided) {
Line 227: if (provided == null) {
The "provided" is the value taken from the text box itself which should never
be null (can be in some hacky cases if someone breaks the SuggestBox's TextBox
on purpose or some JS hack but normally not). I can remove this check entirely
but I like to be defensive and not fail in case of some strange errors.
Line 228: throw new IllegalArgumentException("Only non-null
arguments are accepted"); //$NON-NLS-1$
Line 229: }
Line 230:
Line 231: for (T data : acceptableValues) {
Line 224:
Line 225: @Override
Line 226: protected T asEntity(String provided) {
Line 227: if (provided == null) {
Line 228: throw new IllegalArgumentException("Only non-null
arguments are accepted"); //$NON-NLS-1$
Please be careful not to confuse the "provided" string which is the string
rendered in the text box and is not null (see ValueBoxBase.setText) with the
acceptableValues which can contain null. And this null from acceptableValues is
than rendered as some not null string. See my comments in the loop.
Line 229: }
Line 230:
Line 231: for (T data : acceptableValues) {
Line 232: String expected = renderer.getReplacementString(data);
Line 230:
Line 231: for (T data : acceptableValues) {
Line 232: String expected = renderer.getReplacementString(data);
Line 233: if (expected == null) {
Line 234: continue;
Did you mean
if (expected == null) {
return null;
}
?
But neither is correct.
- The (provided == null) => should never happen
- The (expected == null) => The whole idea of this widget is that the
renderer.getReplacementString is EXACTLY the same string as the one in the text
box.
As the provided == null is essentially an incorrect state than the
renderer.getReplacementString(data) can never return null. If it does, I can
either fail or
ignore it. As I don't want to fail completely so I just ignore it. I certainly
can not throw
IllegalArgumentException because it is not an illegal argument,
and I can certainly not return null because it says nothing about the fact if
the provided String is the same as the given entity would be rendered by this
renderer.
Line 235: }
Line 236:
Line 237: if (expected.equals(provided)) {
Line 238: return data;
Line 233: if (expected == null) {
Line 234: continue;
Line 235: }
Line 236:
Line 237: if (expected.equals(provided)) {
no relevant in the context of my previous answer.
Maybe can be rewritten to:
if (expected != null && expected.equals(provided)) ...
Line 238: return data;
Line 239: }
Line 240: }
Line 241:
--
To view, visit http://gerrit.ovirt.org/18791
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I199d4ce14b55b3f552cf6138b7c3c5d21d620ccc
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Tomas Jelinek <tjeli...@redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Lior Vernia <lver...@redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
