----- Original Message ----- > From: "Vojtech Szocs" <vsz...@redhat.com> > To: "Tomas Jelinek" <tjeli...@redhat.com> > Cc: "engine-devel" <engine-devel@ovirt.org>, "Daniel Erez" > <de...@redhat.com>, "Gilad Chaplik" <gchap...@redhat.com>, > "Tal Nisan" <tni...@redhat.com>, "Alona Kaplan" <alkap...@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 > - 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. > > [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" <tjeli...@redhat.com> > > To: "engine-devel" <engine-devel@ovirt.org> > > Cc: "Vojtech Szocs" <vsz...@redhat.com>, "Daniel Erez" <de...@redhat.com>, > > "Gilad Chaplik" <gchap...@redhat.com>, > > "Tal Nisan" <tni...@redhat.com>, "Alona Kaplan" <alkap...@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" <tjeli...@redhat.com> > > > To: "engine-devel" <engine-devel@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@ovirt.org > > > http://lists.ovirt.org/mailman/listinfo/engine-devel > > > > > > _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel
