Lior Vernia has posted comments on this change. Change subject: frontend: refactoring: Generify list models ......................................................................
Patch Set 19: (3 comments) http://gerrit.ovirt.org/#/c/32907/19/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/HasEntity.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/HasEntity.java: Line 3: import org.ovirt.engine.ui.uicompat.Event; Line 4: import org.ovirt.engine.ui.uicompat.EventArgs; Line 5: import org.ovirt.engine.ui.uicompat.EventDefinition; Line 6: Line 7: public interface HasEntity<T> extends IModel { Is there any justification for this interface? Both EntityModel and ListModelWithEntity need to have these methods, but they don't have to inherit from a single interface. It's arguable that "entity" means the same thing in those two implementors... If it's dropped, there'll be no need for this IModel hack either. Line 8: Line 9: EventDefinition entityChangedEventDefinition = new EventDefinition("EntityChanged", HasEntity.class); //$NON-NLS-1$; Line 10: Line 11: Event<EventArgs> getEntityChangedEvent(); http://gerrit.ovirt.org/#/c/32907/19/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.java: Line 11: /** Line 12: * @param <E> {@link org.ovirt.engine.ui.uicommonweb.models.ListModelWithEntity.E} Line 13: * @param <T> {@link org.ovirt.engine.ui.uicommonweb.models.ListModelWithEntity.T} Line 14: */ Line 15: public class SortedListModel<E, T> extends ListModelWithEntity<E, T> { This shouldn't extend ListModelWithEntity - there's no specific functionality referring to EntityModel in SortedListModel. It's clear that the only reason it's here is that Java doesn't allow multiple inheritance. However, it doesn't make sense from an object-oriented standpoint. I would get rid of ListModelWithEntity and have SearchableListModel implement its functionality directly. This will have the added advantage that people won't actually create more ListModelWithEntity subclasses, and instead extend ListModel and add properly-named members if they need them... NetworkItemModel doesn't really need the ListModelWithEntity functionality - LogicalNetworkModel could have a network member, and similarly NetworkInterfaceModel an interface member. Line 16: Line 17: static class SortSensitiveComparator<T> implements Comparator<T> { Line 18: Line 19: private final Comparator<? super T> comparator; http://gerrit.ovirt.org/#/c/32907/19/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/network/NetworkItemModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/hosts/network/NetworkItemModel.java: Line 9: * Line 10: * @param <T> Line 11: * The Item status enum type Line 12: */ Line 13: public abstract class NetworkItemModel<E, T extends Enum<T>> extends ListModelWithEntity<E, LogicalNetworkModel> implements Comparable<NetworkItemModel<E, T>> { So now this extends SortedListModel as well, even though it used to only extend ListModel?... Is this change in functionality intended? Line 14: Line 15: private final HostSetupNetworksModel setupModel; Line 16: Line 17: private String error = null; -- To view, visit http://gerrit.ovirt.org/32907 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I00f1671a5839fe31b49ad0d1c99bad84e5f0c7c4 Gerrit-PatchSet: 19 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Frank Kobzik <fkob...@redhat.com> Gerrit-Reviewer: Greg Sheremeta <gsher...@redhat.com> Gerrit-Reviewer: Jakub Niedermertl <jnied...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
