On 02/06/14 18:22, Alexander Wels wrote: > On Monday, June 02, 2014 10:46:08 AM Vojtech Szocs wrote: >> ----- Original Message ----- >> >>> From: "Vojtech Szocs" <vsz...@redhat.com> >>> To: aw...@redhat.com >>> Cc: devel@ovirt.org >>> Sent: Monday, June 2, 2014 4:37:27 PM >>> Subject: Re: [ovirt-devel] Sortable Ui Columns >>> >>> >>> ----- Original Message ----- >>> >>>> From: "Alexander Wels" <aw...@redhat.com> >>>> To: "Lior Vernia" <lver...@redhat.com> >>>> Cc: devel@ovirt.org >>>> Sent: Monday, June 2, 2014 3:35:51 PM >>>> Subject: Re: [ovirt-devel] Sortable Ui Columns >>>> >>>> On Monday, June 02, 2014 11:27:26 AM Lior Vernia wrote: >>>>> After discussing this with Alex on another thread, I just pushed this: >>>>> http://gerrit.ovirt.org/#/c/28268/ >>>>> >>>>> If it's approved, then it'll take care of the secondary criterion for >>>>> you, so you only need to pass the criterion that actually interests >>>>> you. >>>>> Should simplify our work in the next couple of weeks. >>>> >>>> Okay, I think we need to get everyone on the same page with this, right >>>> now >>>> we >>>> have several solutions to the problem, and I think we should pick one >>>> and >>>> go >>>> with that. Right now we have the following solutions available: >>>> >>>> 1. Attempt to keep the current order in case of 'duplication' in the >>>> comparator. So the fallback is to keep the current order. This is >>>> implemented >>>> in Liors patch [1]. >>>> 2. Use the hash code as a fallback in case of 'duplication' in the >>>> comparator. >>>> Due to hashcode rules this should result in the 'natural' order in case >>>> of >>>> duplicates. This is implemented in my patch [2]. >>>> 3. Do ad-hoc fallback mechanism which can specialize to do the proper >>>> thing >>>> based on domain knowledge. It looks like this is what Anmol is doing in >>>> his >>>> gluster sorting patch [3] >>>> 4. Replace the SortedSet with a List but lose the ability to >>>> automatically >>>> sort when calling getItems().add(X). >>>> >>>> To me it seems that 3 and 4 are off the table as we want to keep the >>>> ability >>>> to >>>> automatically sort. And doing ad hoc solutions for every single sorting >>>> column >>>> is going to take a lot of time and is going to lead to maintainability >>>> issues >>>> down the road. That leaves 1 and 2 on which I would like to have a >>>> discussion, >>>> so we can pick the appropriate method and go forward with that. >>> >>> I just reviewed Lior's patch and posted some comments: >>> http://gerrit.ovirt.org/#/c/28268/2/frontend/webadmin/modules/uicommonwe >>> b/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SortedListModel.j >>> ava> >>> Initially, I thought that computing item positions based on the order >>> in which they are returned by Collection's iterator might be unreliable, >>> now that I think more about it, I think this approach is something we >>> should consider. >>> >>> In other words, in edge case when comparing two items with same logical >>> property (i.e. two Data Centers with same name), we should honor the >>> ordering of items within the original Collection provided by the server. >>> Otherwise, comparing hashCode might change their order in such edge >>> case. So actually it seems better to preserve Collection's iteration >>> order rather than preserving natural order in such edge case. >> >> Just wanted to mention one more idea I just realized while talking with >> Alex: after we move to REST API, there won't be any hashCode anymore. >> We'll receive a collection (array) of items (objects) with IDs, which >> means hashCode will become backend business entity impl. detail. This >> is also the reason why I'm more inclined towards preserving order of >> items within original collection; there won't be any "natural ordering" >> of items after we move to REST API. >> > > Thinking about Liors patch a little bit doesn't it have the same problem as > just replacing the sorted set with a list that we sort? doing > getItems().add(item) will cause the sorting to no longer work, as that new > item won't be in the positions map. So essentially this doesn't work any > better than replacing the SortedSet with some sort of List. >
You are correct that it needs to be tweaked a little to accommodate that, but it can. I'll push a new patchset and let's discuss this on the patch itself. >>> What do you think guys? >>> >>>> [1] http://gerrit.ovirt.org/#/c/28268/ >>>> >>>> [2] http://gerrit.ovirt.org/#/c/28225/ >>>> >>>> [3] http://gerrit.ovirt.org/#/c/28233/ >>>> >>>>> On 31/05/14 15:35, Anmol Babu wrote: >>>>>> Thanks a lot Alexander.Your idea of having a second criteria based >>>>>> sorting >>>>>> just in case of comparing same values(compare returning 0) looks >>>>>> good >>>>>> to >>>>>> me and now, I have also done the same in my patch set >>>>>> http://gerrit.ovirt.org/#/c/28233/.I have added you as a reviewer as >>>>>> well. Thanks, >>>>>> Anmol >>>>>> >>>>>> ----- Original Message ----- >>>>>> From: "Alexander Wels" <aw...@redhat.com> >>>>>> To: devel@ovirt.org >>>>>> Cc: "Anmol Babu" <anb...@redhat.com> >>>>>> Sent: Friday, May 30, 2014 5:55:20 PM >>>>>> Subject: Re: [ovirt-devel] Sortable Ui Columns >>>>>> >>>>>> Anmol, >>>>>> >>>>>> This is due to the fact that the sorting is done by a SortedSet >>>>>> instead >>>>>> of >>>>>> a list in the SortedListModel. To fix this we have to do one of two >>>>>> things: >>>>>> >>>>>> 1. Change the sorting to use a list of some sort, but apparently >>>>>> that >>>>>> is >>>>>> not much of an option as people want automatic sorting when they add >>>>>> new >>>>>> items to the collection. >>>>>> 2. Make sure that the comparator never returns 0 for two entities >>>>>> that >>>>>> are >>>>>> really not the same. The reason you are seeing the issue is because >>>>>> you >>>>>> are >>>>>> using a different comparator that does return 0 on whatever field >>>>>> you >>>>>> are >>>>>> comparing against. I had the exact same issue and I solved it by >>>>>> using >>>>>> a >>>>>> compound comparator. Basically what I did was create a comparator >>>>>> that >>>>>> contains the field I am trying to compare on and if that returns 0 >>>>>> then >>>>>> I >>>>>> use a different comparator that is guaranteed to not return 0 for >>>>>> the >>>>>> entity. >>>>>> >>>>>> I have a patch that implements sorting for all the data center main >>>>>> tab >>>>>> and >>>>>> sub tabs here [1] that demonstrates how I solved the issue. This >>>>>> patch >>>>>> hasn't been reviewed yet, and my solution might get rejected, but it >>>>>> does >>>>>> work and doesn't make your entries disappear when you sort. >>>>>> >>>>>> >>>>>> [1] http://gerrit.ovirt.org/#/c/28225/ >>>>>> >>>>>> On Friday, May 30, 2014 02:32:17 AM Anmol Babu wrote: >>>>>>> Hi, >>>>>>> >>>>>>> While applying client-side sort using the sorting infra, to the >>>>>>> "Server" >>>>>>> >>>>>>> column of the "Volumes" sub tab "Bricks", I had 2 Bricks with same >>>>>>> server >>>>>>> name.So,when I sorted it, it removed one of the bricks that had the >>>>>>> same >>>>>>> server name. I found that this issue occurs when the sort values >>>>>>> compared >>>>>>> are same(i.e, comparator's compare returns 0). Regards, >>>>>>> Anmol.B >>>>>>> _______________________________________________ >>>>>>> Devel mailing list >>>>>>> Devel@ovirt.org >>>>>>> http://lists.ovirt.org/mailman/listinfo/devel >>>>>> >>>>>> _______________________________________________ >>>>>> Devel mailing list >>>>>> Devel@ovirt.org >>>>>> http://lists.ovirt.org/mailman/listinfo/devel >>>> >>>> _______________________________________________ >>>> Devel mailing list >>>> Devel@ovirt.org >>>> http://lists.ovirt.org/mailman/listinfo/devel >>> >>> _______________________________________________ >>> Devel mailing list >>> Devel@ovirt.org >>> http://lists.ovirt.org/mailman/listinfo/devel > > _______________________________________________ > Devel mailing list > Devel@ovirt.org > http://lists.ovirt.org/mailman/listinfo/devel > _______________________________________________ Devel mailing list Devel@ovirt.org http://lists.ovirt.org/mailman/listinfo/devel