[ovirt-devel] Sortable Ui Columns

[Lior Vernia]

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" <vszocs at redhat.com>
>>> To: awels at redhat.com
>>> Cc: devel at ovirt.org
>>> Sent: Monday, June 2, 2014 4:37:27 PM
>>> Subject: Re: [ovirt-devel] Sortable Ui Columns
>>>
>>>
>>> ----- Original Message -----
>>>
>>>> From: "Alexander Wels" <awels at redhat.com>
>>>> To: "Lior Vernia" <lvernia at redhat.com>
>>>> Cc: devel at 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" <awels at redhat.com>
>>>>>> To: devel at ovirt.org
>>>>>> Cc: "Anmol Babu" <anbabu at 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 at ovirt.org
>>>>>>> http://lists.ovirt.org/mailman/listinfo/devel
>>>>>>
>>>>>> _______________________________________________
>>>>>> Devel mailing list
>>>>>> Devel at ovirt.org
>>>>>> http://lists.ovirt.org/mailman/listinfo/devel
>>>>
>>>> _______________________________________________
>>>> Devel mailing list
>>>> Devel at ovirt.org
>>>> http://lists.ovirt.org/mailman/listinfo/devel
>>>
>>> _______________________________________________
>>> Devel mailing list
>>> Devel at ovirt.org
>>> http://lists.ovirt.org/mailman/listinfo/devel
> 
> _______________________________________________
> Devel mailing list
> Devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/devel
>