[Engine-devel] Introducing generics to UiCommon
Tomas Jelinek
tjelinek at redhat.com
Fri Oct 11 13:08:20 UTC 2013
----- Original Message -----
> From: "Tomas Jelinek" <tjelinek at redhat.com>
> To: "Vojtech Szocs" <vszocs at redhat.com>
> Cc: "engine-devel" <engine-devel at ovirt.org>
> Sent: Friday, October 11, 2013 10:56:03 AM
> Subject: Re: [Engine-devel] Introducing generics to UiCommon
>
>
>
> ----- Original Message -----
> > From: "Vojtech Szocs" <vszocs at redhat.com>
> > To: "Tomas Jelinek" <tjelinek at redhat.com>
> > Cc: "engine-devel" <engine-devel at ovirt.org>, "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: Monday, October 7, 2013 12:17:30 PM
> > Subject: Re: [Engine-devel] Introducing generics to UiCommon
> >
> > 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
>
> yes, this should be the widgets which needs to be done
Ok, no, I have actually found some more widgets which needs to be made generic:
EntityModelCheckBox
EntityModelCheckBoxOnlyEditor
EntityModelRadioButton
EntityModelSliderWithTextBoxEditor
EntityModelTextBoxOnlyEditor
EntityModelInputWithSlider
I will provide them in a future patch.
>
> > - any other change necessary? (aside from modifying specific model classes
> > such as DataCenterModel)
>
> I would say nothing else.
>
> > [*] IIUC for each editor widget we need: the widget itself, editor,
> > renderer/parser (potential reuse)
>
> yep
>
> >
> > 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
>
> well, my idea was to do the infrastructure as needed while refactoring the
> specific models.
> But ok, I can prepare this infrastructure patch first.
so I have prepared the second part of the infrastructure patch:
http://gerrit.ovirt.org/#/c/20118/
It contains the widgets enumerated above (e.g. EntityModelLabel,
EntityModelTextAreaLabel, EntityModelPasswordBox, EntityModelTextArea,
ListModelSuggestBox) and the SearchableListModel.
>
> >
> > [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)
>
> yes, sounds reasonable.
>
> >
> > 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
> > > >
> > >
> >
> _______________________________________________
> Engine-devel mailing list
> Engine-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/engine-devel
>
More information about the Engine-devel
mailing list