[Engine-devel] Introducing generics to UiCommon

Vojtech Szocs vszocs at redhat.com
Mon Oct 7 10:17:30 UTC 2013


Hi Tomas,

I missed the original mail and just reviewed both patches (+1 from my side).

I think introducing generics to UiCommon is a step in the right direction; as you wrote, using non-generic types (i.e. List instead of List<ElementType>) leads to hidden expectations and type casts that make code harder to read and maintain (and also impacts GwtCommon/WebAdmin/UserPortal code that needs to adapt to such code).

Do we have some general plan to cover UiCommon as a whole?
- patch [1] modifies EntityModel/ListModel, what about other base models? (i.e. SearchableListModel)
- patch [1] adds String version of EntityModelTextBox, what about other editor widgets? [*]
  according to your original mail, these are: EntityModelLabel, EntityModelTextAreaLabel, EntityModelPasswordBox, EntityModelTextArea, ListModelSuggestBox
- any other change necessary? (aside from modifying specific model classes such as DataCenterModel)
[*] IIUC for each editor widget we need: the widget itself, editor, renderer/parser (potential reuse)

What about following approach (just a suggestion):

[separate patch - rebased on patch 1]
- make remaining base models generic too (i.e. SearchableListModel)
- add remaining GUI infra to bind to generic models, i.e. editor widgets and related stuff

[separate patch per specific main tab model]
- make main tab model use generics (including any dialog models referenced from this code)
- make related sub tab models use generics (including any dialog models referenced from this code)

Having said that, I value your effort to improve existing code. This is not an easy task, it will take more patches but I think it's worth it.

Thanks!
Vojtech


----- Original Message -----
> From: "Tomas Jelinek" <tjelinek at redhat.com>
> To: "engine-devel" <engine-devel at ovirt.org>
> Cc: "Vojtech Szocs" <vszocs at redhat.com>, "Daniel Erez" <derez at redhat.com>, "Gilad Chaplik" <gchaplik at redhat.com>,
> "Tal Nisan" <tnisan at redhat.com>, "Alona Kaplan" <alkaplan at redhat.com>
> Sent: Friday, September 27, 2013 1:32:14 PM
> Subject: Re: [Engine-devel]  Introducing generics to UiCommon
> 
> Hey all,
> 
> some time ago I have created a patch which introduces generics to UiCommon
> [1] and one which uses it
> in DataCenterModel [2]. Today I have changed a bit the generic version of the
> EntityModelTextBox to
> be truly generic (since it can edit also e.g. integers) and than a simple
> subclass StringEntityModelTextBox
> which provides the String renderer/parser to simplify the usage. All the
> other details are available in the previous
> mail.
> 
> What do you think about it?
> 
> Tomas
> 
> [1]: http://gerrit.ovirt.org/#/c/17604/
> [2]: http://gerrit.ovirt.org/#/c/17605/
> 
> ----- Original Message -----
> > From: "Tomas Jelinek" <tjelinek at redhat.com>
> > To: "engine-devel" <engine-devel at ovirt.org>
> > Sent: Monday, August 5, 2013 9:41:29 AM
> > Subject: [Engine-devel]  Introducing generics to UiCommon
> > 
> > Hey all,
> > 
> > as we have passed the oVirt feature freeze I would like to celebrate it
> > with
> > a little bit of cleanup :)
> > 
> > A good candidate for this is to introduce generics into uicommonweb
> > project.
> > The fact that it is not generic
> > brings quite some hidden expectations into our code, makes it unreadable
> > and
> > error prone.
> > 
> > Also, the gwt-common and both webadmin and userportal are mostly prepared
> > to
> > be generic but because the uicommonweb is not, we have code like:
> > 
> > new ListModelListBoxEditor<Object>(new NullSafeRenderer<Object>() {
> >             @Override
> >             public String renderNullSafe(Object object)
> >                 return ((Version) object).getValue();
> >             }
> >         });
> > 
> > which is quite ugly and error prone.
> > 
> > So I have prepared two patches, one [1] which introduces the generic
> > infrastructure (and prepares one widget for it, more about this below) and
> > one [2] which uses it and refactors the DataCenterModel
> > to use it (I have chosen this model because it is big enough to show how to
> > do it and what the benefits are but small enough to be quickly
> > review-able).
> > 
> > The infrastructure change:
> > - changes the ListModel and EntityModel to be genreic
> > - adjusts the UiCommonEditorDriverGenerator to work with generics (e.g. to
> > make it aware that ListModel<String> is indeed a ListModel, same for
> > EntityModel)
> > - created a String version of EntityModelTextBox
> > 
> > The reason why the String EntityModelTextBox had to be created is that the
> > EntityModelTextBox is an EditorWidget<Object, ...> so it can work only with
> > EntityModel<Object>. I saw 2 ways how to make this work with
> > EntityModel<String>:
> > 1: Create a String version of this editor inside the .generic sub-package,
> > incrementally replace the usage of the non-generic EntityModelTextBox and
> > when the non-generic will be completely replaced, delete it and move the
> > generic one
> >    out from the generic sub-package
> > 
> > 2: Change the EditorWidget<Object, ...> to EditorWidget<T, ...> and replace
> > each usage of the "EntityModelTextBox someWidget" by
> > "EntityModelTextBox<Object> someWidget" and than incrementally replace the
> > <Object> to <String> as the
> >    underlying models will be refactored. After the last one will be
> >    refactored, change the EditorWidget<T, ...> to EditorWidget<String, ...>
> >    and replace all "EntityModelTextBox<Object> someWidget" by
> >    "EntityModelTextBox someWidget"
> > 
> > I have chosen the first option because:
> > - much less classes touched at once (e.g. much more safe)
> > - the EntityModelTextBox<T> invites to use something like
> > EntityModelTextBox<VM> which is not correct and fails on class cast
> > exceptions
> > 
> > But at the same time I see the disadvantages of this approach (mostly that
> > we
> > have two versions of the same class). Please note that far not all the
> > widgets will need two versions, only the ones editing only Strings which
> > are declared as EditorWidget<Object> which are:
> > - EntityModelLabel
> > - EntityModelTextAreaLabel (used only in couple of places - can be
> > refactored
> > together without the need to have two versions)
> > - EntityModelTextBox (already in the [1])
> > - EntityModelPasswordBox
> > - EntityModelTextArea
> > - ListModelSuggestBox (used only in couple of places - can be refactored
> > together without the need to have two versions)
> > 
> > The rest of the widgets should be already prepared to be used in generic
> > environment.
> > 
> > Please let me know what do you think about it,
> > 
> > have a nice day,
> > Tomas
> > 
> > [1]: http://gerrit.ovirt.org/#/c/17604/
> > [2]: http://gerrit.ovirt.org/#/c/17605/
> > 
> > _______________________________________________
> > Engine-devel mailing list
> > Engine-devel at ovirt.org
> > http://lists.ovirt.org/mailman/listinfo/engine-devel
> > 
> 



More information about the Devel mailing list