[Engine-devel] Sorting in tabs

Mon Jul 8 12:31:54 UTC 2013

Hi Lior,

I'm revisiting this thread and posting some of my ideas on the sorting issue.

Page-able data widgets like tables with paging buttons, should use server-side sorting: clicking on table column header updates the search string ("sortby <attribute>") which causes CommonModel.search() to be called and new data presented in the given table.

Non-page-able data widgets like tables without paging buttons, i.e. tables bound to SearchableListModel whose "Search{Next|Previous}PageCommand" isn't available, can use client-side sorting. However, instead of manipulating UiCommon model data (items) directly and possibly breaking some hidden expectations within UiCommon code (as Tomas mentioned earlier), I'd rather do client-side sorting on model provider level.

Model providers represent a layer between GUI and UiCommon: GUI <--> model provider <--> UiCommon model

Model providers are adapters between GUI and UiCommon models, listening to important events fired by models and updating GUI accordingly & providing API (facade) for GUI to trigger operations on models. For client-side sorting, I think it's better to implement this feature on model provider level, i.e. inside DataBoundTabModelProvider.updateData() method:

  List<T> items = (List<T>) getModel().getItems();
  // possibly use comparator to sort items

As for GWT's native support for table sorting: this is already in GWT 2.3, see AbstractCellTable.addColumnSortHandler() method. It's just a handler that keeps track of current sort options, i.e. "column2 asc & column 1 desc & column 3 asc". In other words, GWT provides a mechanism to notify our code what are current sort options, but it's up to us to re-render data based on these sort options, i.e. using Comparator on UiCommon model items.

Vojtech

----- Original Message -----
> From: "Tomas Jelinek" <tjelinek at redhat.com>
> To: "Einav Cohen" <ecohen at redhat.com>
> Cc: "Lior Vernia" <lvernia at redhat.com>, "Vojtech Szocs" <vszocs at redhat.com>, "Eli Mesika" <emesika at redhat.com>,
> engine-devel at ovirt.org, "Alexander Wels" <awels at redhat.com>, "Daniel Erez" <derez at redhat.com>, "Gilad Chaplik"
> <gchaplik at redhat.com>, "Alona Kaplan" <alkaplan at redhat.com>
> Sent: Friday, June 28, 2013 9:00:47 AM
> Subject: Re: [Engine-devel] Sorting in tabs
> 
> 
> 
> ----- Original Message -----
> > From: "Tomas Jelinek" <tjelinek at redhat.com>
> > To: "Einav Cohen" <ecohen at redhat.com>
> > Cc: "Lior Vernia" <lvernia at redhat.com>, "Vojtech Szocs"
> > <vszocs at redhat.com>, "Eli Mesika" <emesika at redhat.com>,
> > engine-devel at ovirt.org, "Alexander Wels" <awels at redhat.com>, "Daniel Erez"
> > <derez at redhat.com>, "Gilad Chaplik"
> > <gchaplik at redhat.com>, "Alona Kaplan" <alkaplan at redhat.com>
> > Sent: Friday, June 28, 2013 8:45:17 AM
> > Subject: Re: [Engine-devel] Sorting in tabs
> > 
> > 
> > 
> > ----- Original Message -----
> > > From: "Einav Cohen" <ecohen at redhat.com>
> > > To: "Lior Vernia" <lvernia at redhat.com>
> > > Cc: "Vojtech Szocs" <vszocs at redhat.com>, "Eli Mesika"
> > > <emesika at redhat.com>,
> > > engine-devel at ovirt.org, "Alexander Wels"
> > > <awels at redhat.com>, "Daniel Erez" <derez at redhat.com>, "Gilad Chaplik"
> > > <gchaplik at redhat.com>, "Alona Kaplan"
> > > <alkaplan at redhat.com>, "Tomas Jelinek" <tjelinek at redhat.com>
> > > Sent: Thursday, June 27, 2013 6:40:56 PM
> > > Subject: Re: [Engine-devel] Sorting in tabs
> > > 
> > > > ----- Original Message -----
> > > > From: "Lior Vernia" <lvernia at redhat.com>
> > > > Sent: Thursday, June 27, 2013 12:15:14 PM
> > > > 
> > > > 
> > > > 
> > > > On 27/06/13 18:15, Einav Cohen wrote:
> > > > >> ----- Original Message -----
> > > > >> From: "Lior Vernia" <lvernia at redhat.com>
> > > > >> Sent: Thursday, June 27, 2013 10:38:04 AM
> > > > >>
> > > > >>
> > > > >>
> > > > >> On 27/06/13 16:42, Einav Cohen wrote:
> > > > >>>> ----- Original Message -----
> > > > >>>> From: "Lior Vernia" <lvernia at redhat.com>
> > > > >>>> Sent: Thursday, June 27, 2013 8:53:59 AM
> > > > >>>>
> > > > >>>>
> > > > >>>>
> > > > >>>> On 27/06/13 15:37, Einav Cohen wrote:
> > > > >>>>>> ----- Original Message -----
> > > > >>>>>> From: "Eli Mesika" <emesika at redhat.com>
> > > > >>>>>> Sent: Thursday, June 27, 2013 6:46:58 AM
> > > > >>>>>>
> > > > >>>>>>
> > > > >>>>>> ----- Original Message -----
> > > > >>>>>>> From: "Lior Vernia" <lvernia at redhat.com>
> > > > >>>>>>> To: engine-devel at ovirt.org
> > > > >>>>>>> Sent: Thursday, June 27, 2013 10:12:33 AM
> > > > >>>>>>> Subject: [Engine-devel] Sorting in tabs
> > > > >>>>>>>
> > > > >>>>>>> Hello everyone (UI peeps in particular),
> > > > >>>>>>>
> > > > >>>>>>> I've pushed (not yet merged) a patch that would enable us to
> > > > >>>>>>> keep
> > > > >>>>>>> items
> > > > >>>>>>> in tabs (main/sub) sorted at all times by setting a comparator
> > > > >>>>>>> in
> > > > >>>>>>> SearchableListModel:
> > > > >>>>>>
> > > > >>>>>> But tabs includes only 100 records and supports paging , how you
> > > > >>>>>> deal
> > > > >>>>>> with
> > > > >>>>>> that ???
> > > > >>>>>
> > > > >>>>> if this is in the GUI level, then I assume that the comparator is
> > > > >>>>> simply
> > > > >>>>> comparing the
> > > > >>>>> items within the current page, and not "globally".
> > > > >>>>> so the sorting doesn't affect the set of items that is displayed
> > > > >>>>> in
> > > > >>>>> the
> > > > >>>>> page (it would
> > > > >>>>> be the same as before the sorting) - just their order.
> > > > >>>>
> > > > >>>> Yes, if I understand correctly how the paging works, Einav is
> > > > >>>> correct
> > > > >>>> -
> > > > >>>> only the items passed to the UI are sorted.
> > > > >>>>
> > > > >>>>> also: @Lior - what happens when the search query contains a "sort
> > > > >>>>> by"
> > > > >>>>> part?
> > > > >>>>> there is a chance that the behaivor would be unexpected in this
> > > > >>>>> case;
> > > > >>>>
> > > > >>>> Yes, I thought about this case, and it may result in a confusing
> > > > >>>> user
> > > > >>>> experience if developers aren't careful. Together with the issue
> > > > >>>> of
> > > > >>>> paging, this probably makes this sorting mechanism a better
> > > > >>>> candidate
> > > > >>>> for use within subtabs rather than main tabs.
> > > > >>>
> > > > >>> note that at some point, I think that we would want to introduce
> > > > >>> paging
> > > > >>> also to search-
> > > > >>> based sub-tabs - it will be useful especially for sub-tabs that
> > > > >>> potentially
> > > > >>> display a
> > > > >>> large number of results (e.g. Disks sub-tab in Storage main tab).
> > > > >>> In addition, at some point, we would want to get rid of the paging
> > > > >>> UI
> > > > >>> as
> > > > >>> it
> > > > >>> is now
> > > > >>> (i.e. "next"/"prev" buttons at the top panel) and move to paging
> > > > >>> triggered
> > > > >>> by scroll
> > > > >>> (i.e. have a very long grid, dynamically loaded as you continue to
> > > > >>> scroll
> > > > >>> -
> > > > >>> similar
> > > > >>> to the behavior of some e-mail web-clients, for example). In this
> > > > >>> case,
> > > > >>> sorting on
> > > > >>> the client side will make no sense at all (i.e. from the user
> > > > >>> perspective,
> > > > >>> only a
> > > > >>> portion of a very large grid will be sorted, the other portions
> > > > >>> won't
> > > > >>> be).
> > > > >>>
> > > > >>> So for now - yes, I think it makes sense to introduce your
> > > > >>> mechanism
> > > > >>> to
> > > > >>> all
> > > > >>> sub-tabs,
> > > > >>> however in the long-term - we would probably want the search-based
> > > > >>> sub-tabs
> > > > >>> (which
> > > > >>> will support paging) to move to search-based sorting, rather than
> > > > >>> GUI-based-sorting.
> > > > >>
> > > > >> Sounds good to me. Let me just re-iterate that it is not mandatory
> > > > >> to
> > > > >> set a comparator, so in technical terms it's not even necessary to
> > > > >> introduce it at once to all sub-tabs, if they're already sorting
> > > > >> their
> > > > >> items some other way. It could happen gradually, and only if
> > > > >> developers
> > > > >> find it more convenient. In either case, dropping the GUI sorting
> > > > >> once
> > > > >> search-based sorting is implemented shouldn't be difficult.
> > > > >>
> > > > >>> BTW (maybe the other GUI maintainers can help me with that one) -
> > > > >>> what
> > > > >>> about sub-tabs
> > > > >>> that are not search-based (i.e. display results from a "regular"
> > > > >>> query
> > > > >>> or
> > > > >>> even from a
> > > > >>> field within the selected item in the main grid, e.g. Applications
> > > > >>> in
> > > > >>> VM)
> > > > >>> -
> > > > >>> are these
> > > > >>> managed via SearchableListModel as well? since the comparator
> > > > >>> mechanism
> > > > >>> *is* relevant
> > > > >>> for them.
> > > > >>
> > > > >> As far as I've seen, some are managed via SearchableListModel and
> > > > >> some
> > > > >> aren't. Those that aren't are those that display non-trivial
> > > > >> behaviour
> > > > >> upon receipt of the items to display (setItems() method) - often
> > > > >> this
> > > > >> non-trivial behaviour is exactly sorting :) And if it's doing its
> > > > >> job,
> > > > >> then there's no necessity to change it either. But anyway, I don't
> > > > >> know
> > > > >> all of them, so I'd also love to hear GUI maintainers.
> > > > >>
> > > > >>> Also: Worth mentioning "Bug 893999 - webadmin: please allow column
> > > > >>> sorting", which
> > > > >>> requests to enable sorting when clicking on a grid-column header;
> > > > >>> when
> > > > >>> implementing
> > > > >>> column-sorting, probably worth attaching your mechanism to it
> > > > >>> somehow
> > > > >>> (i.e.
> > > > >>> clicking on
> > > > >>> a column header should set the relevant comparator in the relevant
> > > > >>> SearchableListModel).
> > > > >>
> > > > >> I didn't want to say it, because if we upgrade to a newer version of
> > > > >> GWT
> > > > >> then we could probably use their table column sorting. But this
> > > > >> mechanism could allow us to achieve this without upgrading, and it
> > > > >> was
> > > > >> definitely sitting in the back of my head when I implemented it. All
> > > > >> that's needed is, as you said, to listen to table header clicks in
> > > > >> the
> > > > >> view, and then appropriately set the comparator in the model.
> > > > >>
> > > > > 
> > > > > [Vojtech/GUI-maintainers - your input would be appreciated here]
> > > > > 
> > > > > we are actually planning on upgrading the GWT version *really* soon
> > > > > (to
> > > > > GWT
> > > > > 2.5),
> > > > > so my question is: should we wait until the new GWT is introduced,
> > > > > and
> > > > > implement
> > > > > client-sorting based on the GWT-grid-widget built-in mechanism
> > > > > (assuming
> > > > > there is
> > > > > one)?
> > > > > also, not sure if it is better to utilize the widget default-built-in
> > > > > sorting mechanism
> > > > > (which doesn't manipulate the uicommon model data at all), or if it
> > > > > is
> > > > > better to utilize
> > > > > your comparator mechanism, which manipulates the uicommon model data,
> > > > > and
> > > > > the GUI-widget
> > > > > just reflects this data at any given time.
> > > > > thoughts?
> > > > 
> > > > I'll just give my two cents concerning this and then let others have
> > > > the
> > > > stage: I don't think it really matters.
> > > > 
> > > > Manipulating the models directly is supposedly more portable in case we
> > > > ever move away from GWT, but we'd still have the pain of adding new
> > > > listeners to the new framework's table headers which could be just as
> > > > bad as using its sorting mechanism.
> > > > 
> > > > Graphically, the UI might look tighter if we use GWT's mechanism.
> > > > However, we could probably mimic everything using GWT's graphics (once
> > > > we upgrade) even if we perform the actual sorting using the tab model
> > > > and not their mechanism.
> > > > 
> > > > My gut feeling actually says to use GWT's built-in mechanism, mainly
> > > > because it will force us to put all sorting logic in the same place and
> > > > to always use the same sorting mechanism (whereas currently the sorting
> > > > logic is scattered and works differently in different places, even if
> > > > we
> > > > use this tab mechanism other widgets will differ). But it shouldn't
> > > > stop
> > > > us from setting a comparator for a tab where convenient.
> > > 
> > > [again, GUI maintainers - your input would be appreciated]
> > > 
> > > Thanks, Lior. Need to keep in mind several things:
> > > 
> > > - we are now talking only about client-sorting, which generally we would
> > > like
> > > to apply only to grids that display none-"pagable"-results; for grids
> > > that
> > > would
> > > display pagable-results, we would want to use search-based (i.e. server)
> > > sorting.
> > > [so the sorting logic across the application would be somewhat-scattered
> > > anyway].
> > > 
> > > - I think that if we are choosing to utilize GWT's built-in mechanism,
> > > then
> > > using
> > > a comparator would makes sense in case you want your results to be
> > > initially
> > > displayed
> > > in a specific order that would not be obtainable using the column-header
> > > sorting.
> > > 
> > > e.g. (dumb example, for demonstration only) if you would want your VMs to
> > > be
> > > displayed
> > > by default sorted alphabetically according to the VM's description (and
> > > keep
> > > in mind
> > > that description is NOT one of the columns in the VMs grid), then you
> > > would
> > > probably
> > > want to do that via the comparator.
> > > 
> > > Or (perhaps a better example), if you would want your Templates to be
> > > ordered
> > > by default
> > > in a way that "Blank" would appear first, and the rest would appear
> > > sorted
> > > alphabetically
> > > by name, utilizing the comparator can be a good idea.
> > > [After the initial load of the grid, if the user chooses to sort by name,
> > > he
> > > can click on
> > > the "Name" column header, which will sort the Templates alphabetically
> > > "regularly" (i.e.
> > > not taking "Blank" into special consideration)]
> > > 
> > > But - if, by default, we would want, for example, to sort VMs
> > > alphabetically
> > > by name,
> > > I think that we should imitate a mouse-click on the "Name" column-header,
> > > rather than
> > > utilize a comparator for that (the exact same result shouldn't be
> > > achieved
> > > by
> > > two different
> > > implementations).
> > 
> > Hi Lior,
> > 
> > fist of all, I would be really careful about introducing something to
> > UiCommon which could
> > potentially cause class cast exceptions - not that your approach would be
> > incorrect but the
> > UiCommon has lot's of hidden expectations and it is hard to keep tract who
> > expects the
> > ListModel.getItems will be a list. I would be double careful about this
> > kind
> > of change right
> > before the freeze. Maybe create a SortedSearchableListModel as a child of
> > SearchableListModel
> > would be a much more safe approach.
> > 
> > About the GWT built-in vs Comparator -
> > I would also go with the GWT's built in approach.
> > If we are using a framework we should make use of it and not
> > try to decouple too much to make it simpler to migrate to a different one.
> > 
> > So I agree with Lior that we should go with GWT's built in approach.
> > AFAIK Vojtech is already working on the GWT2.5 integration...
> > 
> > Also I would say that we should have the same user experience regardless
> > the
> > table is paged or not
> > e.g. the user needs to have the same button to click on and the result must
> > make sense after he clicked
> > it everywhere. So I would say we will have to integrate this GUI sorting
> > with
> > server side sorting for paged tables.
> > 
> > But maybe this integration (however a ideal target) is a bit out of the
> > scope
> > of your initial intention ;)
> > 
> > If you have a good reason to have something always sorted and it is not a
> > paged table, I'm not against.
> > Just please be a bit more careful in changing return types in a non-generic
> > code like UiCommon.
> Ooops, now I have looked into your code and realized that you return a
> different type only if the comparator is set
> which makes your patch much less risky - but at the same time much less
> predictable.
> 
> But this kind of implementation details should be discussed on gerrit, so if
> it will be
> decided that the patch should go in in some form we can continue the
> discussion on gerrit.
> 
> > 
> > > 
> > > > 
> > > > >>>>
> > > > >>>>>
> > > > >>>>> I believe that the correct thing to do is to "attach" the GUI
> > > > >>>>> sorting
> > > > >>>>> mechanism
> > > > >>>>> to the one in the search mechanism.
> > > > >>>>>
> > > > >>>>> thoughts?
> > > > >>>>
> > > > >>>> This can be done, however I'm not sure there's much utility in it.
> > > > >>>> Main
> > > > >>>> tabs are always sorted according to some default ordering even if
> > > > >>>> one
> > > > >>>> was not entered in the search panel, and this sorting is also
> > > > >>>> performed
> > > > >>>> consistently with respect to paging. So maybe the right thing to
> > > > >>>> do
> > > > >>>> would be to just "block" the GUI sorting mechanism for main tabs
> > > > >>>> (i.e.
> > > > >>>> override the setter method and make it no-op)?
> > > > >>>
> > > > >>> yes, and related to what I mentioned above - at some point in the
> > > > >>> future,
> > > > >>> we'd might want
> > > > >>> to block it for search-based sub-tabs as well.
> > > > >>>
> > > > >>>>
> > > > >>>>>>
> > > > >>>>>>
> > > > >>>>>>>
> > > > >>>>>>> http://gerrit.ovirt.org/#/c/15846/
> > > > >>>>>>>
> > > > >>>>>>> If a comparator isn't set, then everything should behave as
> > > > >>>>>>> before.
> > > > >>>>>>> If
> > > > >>>>>>> a
> > > > >>>>>>> comparator is set, then from that moment on the tab items will
> > > > >>>>>>> be
> > > > >>>>>>> kept
> > > > >>>>>>> in a SortedSet, so that even if an item is added in a way that
> > > > >>>>>>> doesn't
> > > > >>>>>>> trigger an event (e.g. getItems().add()) the items will be kept
> > > > >>>>>>> sorted
> > > > >>>>>>> according to the given comparator. If the comparator is set to
> > > > >>>>>>> null,
> > > > >>>>>>> from that moment on the tab should revert to its old behaviour.
> > > > >>>>>>>
> > > > >>>>>>> You're most welcome to have a look and let me know if this
> > > > >>>>>>> might
> > > > >>>>>>> break
> > > > >>>>>>> something (remember though that it's not obligatory to set a
> > > > >>>>>>> comparator,
> > > > >>>>>>> so only possible breakage should be in generic flows).
> > > > >>>>>>>
> > > > >>>>>>> Feel free to use it once it's merged; along with
> > > > >>>>>>> SortedListModel,
> > > > >>>>>>> this
> > > > >>>>>>> should make sorting less painful. Just keep in mind that once
> > > > >>>>>>> you
> > > > >>>>>>> set
> > > > >>>>>>> a
> > > > >>>>>>> comparator, you can't cast getItems() to a List. This shouldn't
> > > > >>>>>>> be
> > > > >>>>>>> a
> > > > >>>>>>> problem in general, as mostly it's as useful (and probably more
> > > > >>>>>>> correct)
> > > > >>>>>>> to cast to a Collection.
> > > > >>>>>>>
> > > > >>>>>>> Lior.
> > > > >>>>>>> _______________________________________________
> > > > >>>>>>> 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
> > > > >>>>>>
> > > > >>>>
> > > > >>
> > > > 
> > >
> 