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" <tjeli...@redhat.com> > To: "Einav Cohen" <eco...@redhat.com> > Cc: "Lior Vernia" <lver...@redhat.com>, "Vojtech Szocs" <vsz...@redhat.com>, > "Eli Mesika" <emes...@redhat.com>, > engine-devel@ovirt.org, "Alexander Wels" <aw...@redhat.com>, "Daniel Erez" > <de...@redhat.com>, "Gilad Chaplik" > <gchap...@redhat.com>, "Alona Kaplan" <alkap...@redhat.com> > Sent: Friday, June 28, 2013 9:00:47 AM > Subject: Re: [Engine-devel] Sorting in tabs > > > > ----- Original Message ----- > > From: "Tomas Jelinek" <tjeli...@redhat.com> > > To: "Einav Cohen" <eco...@redhat.com> > > Cc: "Lior Vernia" <lver...@redhat.com>, "Vojtech Szocs" > > <vsz...@redhat.com>, "Eli Mesika" <emes...@redhat.com>, > > engine-devel@ovirt.org, "Alexander Wels" <aw...@redhat.com>, "Daniel Erez" > > <de...@redhat.com>, "Gilad Chaplik" > > <gchap...@redhat.com>, "Alona Kaplan" <alkap...@redhat.com> > > Sent: Friday, June 28, 2013 8:45:17 AM > > Subject: Re: [Engine-devel] Sorting in tabs > > > > > > > > ----- Original Message ----- > > > From: "Einav Cohen" <eco...@redhat.com> > > > To: "Lior Vernia" <lver...@redhat.com> > > > Cc: "Vojtech Szocs" <vsz...@redhat.com>, "Eli Mesika" > > > <emes...@redhat.com>, > > > engine-devel@ovirt.org, "Alexander Wels" > > > <aw...@redhat.com>, "Daniel Erez" <de...@redhat.com>, "Gilad Chaplik" > > > <gchap...@redhat.com>, "Alona Kaplan" > > > <alkap...@redhat.com>, "Tomas Jelinek" <tjeli...@redhat.com> > > > Sent: Thursday, June 27, 2013 6:40:56 PM > > > Subject: Re: [Engine-devel] Sorting in tabs > > > > > > > ----- Original Message ----- > > > > From: "Lior Vernia" <lver...@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" <lver...@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" <lver...@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" <emes...@redhat.com> > > > > >>>>>> Sent: Thursday, June 27, 2013 6:46:58 AM > > > > >>>>>> > > > > >>>>>> > > > > >>>>>> ----- Original Message ----- > > > > >>>>>>> From: "Lior Vernia" <lver...@redhat.com> > > > > >>>>>>> To: engine-devel@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@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 > > > > >>>>>> > > > > >>>> > > > > >> > > > > > > > > _______________________________________________ Engine-devel mailing list Engine-devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-devel