Tomas Jelinek has posted comments on this change.
Change subject: userportal,webadmin: frontend refactor phase 2
......................................................................
Patch Set 4:
(7 comments)
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/Frontend.java
Line 85:
Line 86: /**
Line 87: * The query start event, legacy.
Line 88: */
Line 89: Event queryStartedEvent = new Event(queryStartedEventDefinition);
- why not private static final?
- if legacy, why not marked with @Deprecated ?
Line 90:
Line 91: /**
Line 92: * The query complete event definition, legacy.
Line 93: */
Line 96: /**
Line 97: * The query complete event, legacy.
Line 98: */
Line 99: Event queryCompleteEvent = new Event(queryCompleteEventDefinition);
Line 100:
- why not private static final?
- if legacy, why not marked with @Deprecated ?
Line 101: /**
Line 102: * The {@code frontendFailureEvent} event.
Line 103: */
Line 104: Event frontendFailureEvent = new Event("FrontendFailure",
Frontend.class); //$NON-NLS-1$
Line 100:
Line 101: /**
Line 102: * The {@code frontendFailureEvent} event.
Line 103: */
Line 104: Event frontendFailureEvent = new Event("FrontendFailure",
Frontend.class); //$NON-NLS-1$
- why not private static final?
- if legacy, why not marked with @Deprecated ?
Line 105:
Line 106: /**
Line 107: * The {@code frontendNotLoggedInEvent} event.
Line 108: */
Line 105:
Line 106: /**
Line 107: * The {@code frontendNotLoggedInEvent} event.
Line 108: */
Line 109: Event frontendNotLoggedInEvent = new Event("NotLoggedIn",
Frontend.class); //$NON-NLS-1$
- why not private static final?
- if legacy, why not marked with @Deprecated ?
Line 110:
Line 111: /**
Line 112: * The context the current operation is run against.
Line 113: */
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/VdcOperation.java
Line 9: * @param <T> The type of operation.
Line 10: * @param <P> The parameter type for the operation.
Line 11: */
Line 12: @SuppressWarnings("rawtypes")
Line 13: public class VdcOperation<T, P> implements Serializable {
As this is a pure configuration class I would mark the whole class as final to
avoid any mutability in the children
Line 14: /**
Line 15: * Generated serial version UID.
Line 16: */
Line 17: private static final long serialVersionUID = -6569717023385458462L;
Line 178: @SuppressWarnings("unchecked")
Line 179: VdcOperation<T, P> otherOperation = (VdcOperation<T, P>)
other;
Line 180: result = operationType.equals(otherOperation.getOperation())
Line 181: &&
parameter.equals(otherOperation.getParameter());
Line 182: return result;
- the hashCode depends on the callback and the equals do not?
- you need to have the callback here because otherwise if you had 2 queries
which are same just with different callbacks the second would be ignored.
Line 183: }
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/communication/VdcOperationManager.java
Line 64: // If the user is logged in, or the user is not logged in and
the operation does not allow duplicates (aka
Line 65: // it is a query, and not an action). And the operation is not
already in the queue || the operation is
Line 66: // an action (allows duplicates). Then add this operation to
the queue, and process the queue immediately.
Line 67: if ((loggedIn || isPublic) &&
(!operationQueue.contains(operation)
Line 68: || operation.allowDuplicates()) &&
operationQueue.add(operation)) {
[1] a bit too complex expression in the if. I would assign the subparts like
this:
boolean isAllowedToExecute = loggedIn || isPublic;
boolean canBeAddedToList = !operationQueue.contains(operation) ||
operation.allowDuplicates();
if (isAllowedToExecute && canBeAddedToList && operationQueue.add(operation)) {
...
}
[2] if the user is not allowed to execute the operation this code just silently
ignores it. Shouldn't we notify the user somehow? For example call the onFalure
on the callback?
Line 69: processor.processOperation(this);
Line 70: }
Line 71: }
Line 72:
--
To view, visit http://gerrit.ovirt.org/17356
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id19491b8fd4f30ad3a88790eaad664d679b35e22
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alexander Wels <aw...@redhat.com>
Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Einav Cohen <eco...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com>
Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com>
Gerrit-Reviewer: Vojtech Szocs <vsz...@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
