You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pivot.apache.org by Michael Allman <ms...@allman.ms> on 2010/08/25 02:01:20 UTC
FilteredList bugs
There appears to be some inconsistency in FilteredList with how it behaves
when the filter is null. I think if the filter is null, the list is
supposed to show all values. In that case, the filter logic in the add()
and insert() methods is incorrect. Rather than checking
filter != null && filter.include(item)
it should be checking
filter == null || filter.include(item)
Also, the itemUpdated() method in the listListener field sometimes adds
items to the list which are already there. The problem seems to be in the
following if block
if (comparator == null) {
// Add the item to the view
viewIndex = view.add(item);
listListeners.itemInserted(FilteredList.this, viewIndex);
}
I modified the if statement to make a correction, but I'm not sure my
patch is correct. Here's what I replaced the if statement with
// Update the item in the view
int previousViewIndex = view.indexOf(item);
if (previousViewIndex == viewIndex) {
// Update the item in the view
view.update(viewIndex, item);
} else {
// Remove the item from the view
Sequence<T> removed = view.remove(previousViewIndex, 1);
listListeners.itemsRemoved(FilteredList.this, previousViewIndex, removed);
// Re-add the item to the view
viewIndex = view.add(item);
listListeners.itemInserted(FilteredList.this, viewIndex);
}
Again, I'm not sure this is correct but it seems to have resolved the
issue I was having with duplicate items being added to the list.
Cheers,
Michael
Re: FilteredList bugs
Posted by Todd Volkert <tv...@gmail.com>.
I was using FilteredList in one of my Pivot apps by setting a list of
expense categories as the list data of a ListButton and filtering out
the ones that the user wasn't authorized to bill to. But if it
doesn't belong in the platform, I can do the filtering in application
code...
-T
On Sun, Aug 29, 2010 at 3:46 AM, Michael Allman <ms...@allman.ms> wrote:
> Np. I forgot to file the bug. I've been busy lately, but it turns out
> there was a better way to do what I wanted to do than use a FilteredList
> anyway.
>
> I have found this sort of thing valuable in my Flex work, but it wouldn't
> bother me too much to implement my own version.
>
> Michael
>
>
> On Fri, 27 Aug 2010, Greg Brown wrote:
>
>> I went ahead and made this change - if we think there is still some value
>> in maintaining FilteredList, maybe we can resurrect it in the "extras"
>> project or something.
>>
>> On Aug 27, 2010, at 2:40 PM, Greg Brown wrote:
>>
>>> Now that I look at FilteredList again, I have to wonder if it is worth
>>> keeping. There's a lot of code there and, realistically, not that much
>>> value. Also, I believe it is the only remaining class that still has the
>>> potential for memory leaks if you forget to clear the source property (we
>>> have gotten rid of all the others or re-implemented them such that this
>>> isn't a problem).
>>>
>>> I think I might advocate eliminating this class for Pivot 2.0.
>>>
>>> G
>>>
>>> On Aug 25, 2010, at 8:22 AM, Greg Brown wrote:
>>>
>>>> Hi Michael,
>>>> Can you submit your patch via JIRA? This sounds like a bug, so you can
>>>> simply file it as such.
>>>> Thanks!
>>>> Greg
>>>>
>>>> On Aug 24, 2010, at 8:01 PM, Michael Allman wrote:
>>>>
>>>>> There appears to be some inconsistency in FilteredList with how it
>>>>> behaves when the filter is null. I think if the filter is null, the list is
>>>>> supposed to show all values. In that case, the filter logic in the add()
>>>>> and insert() methods is incorrect. Rather than checking
>>>>>
>>>>> filter != null && filter.include(item)
>>>>>
>>>>> it should be checking
>>>>>
>>>>> filter == null || filter.include(item)
>>>>>
>>>>>
>>>>> Also, the itemUpdated() method in the listListener field sometimes adds
>>>>> items to the list which are already there. The problem seems to be in the
>>>>> following if block
>>>>>
>>>>>
>>>>> if (comparator == null) {
>>>>> // Add the item to the view
>>>>> viewIndex = view.add(item);
>>>>> listListeners.itemInserted(FilteredList.this, viewIndex);
>>>>> }
>>>>>
>>>>>
>>>>> I modified the if statement to make a correction, but I'm not sure my
>>>>> patch is correct. Here's what I replaced the if statement with
>>>>>
>>>>> // Update the item in the view
>>>>> int previousViewIndex = view.indexOf(item);
>>>>>
>>>>> if (previousViewIndex == viewIndex) {
>>>>> // Update the item in the view
>>>>> view.update(viewIndex, item);
>>>>> } else {
>>>>> // Remove the item from the view
>>>>> Sequence<T> removed = view.remove(previousViewIndex, 1);
>>>>> listListeners.itemsRemoved(FilteredList.this, previousViewIndex,
>>>>> removed);
>>>>>
>>>>> // Re-add the item to the view
>>>>> viewIndex = view.add(item);
>>>>> listListeners.itemInserted(FilteredList.this, viewIndex);
>>>>> }
>>>>>
>>>>>
>>>>> Again, I'm not sure this is correct but it seems to have resolved the
>>>>> issue I was having with duplicate items being added to the list.
>>>>>
>>>>> Cheers,
>>>>>
>>>>> Michael
>>>>
>>>
>>
>
Re: FilteredList bugs
Posted by Michael Allman <ms...@allman.ms>.
Np. I forgot to file the bug. I've been busy lately, but it turns out
there was a better way to do what I wanted to do than use a FilteredList
anyway.
I have found this sort of thing valuable in my Flex work, but it wouldn't
bother me too much to implement my own version.
Michael
On Fri, 27 Aug 2010, Greg Brown wrote:
> I went ahead and made this change - if we think there is still some value in maintaining FilteredList, maybe we can resurrect it in the "extras" project or something.
>
> On Aug 27, 2010, at 2:40 PM, Greg Brown wrote:
>
>> Now that I look at FilteredList again, I have to wonder if it is worth keeping. There's a lot of code there and, realistically, not that much value. Also, I believe it is the only remaining class that still has the potential for memory leaks if you forget to clear the source property (we have gotten rid of all the others or re-implemented them such that this isn't a problem).
>>
>> I think I might advocate eliminating this class for Pivot 2.0.
>>
>> G
>>
>> On Aug 25, 2010, at 8:22 AM, Greg Brown wrote:
>>
>>> Hi Michael,
>>> Can you submit your patch via JIRA? This sounds like a bug, so you can simply file it as such.
>>> Thanks!
>>> Greg
>>>
>>> On Aug 24, 2010, at 8:01 PM, Michael Allman wrote:
>>>
>>>> There appears to be some inconsistency in FilteredList with how it behaves when the filter is null. I think if the filter is null, the list is supposed to show all values. In that case, the filter logic in the add() and insert() methods is incorrect. Rather than checking
>>>>
>>>> filter != null && filter.include(item)
>>>>
>>>> it should be checking
>>>>
>>>> filter == null || filter.include(item)
>>>>
>>>>
>>>> Also, the itemUpdated() method in the listListener field sometimes adds items to the list which are already there. The problem seems to be in the following if block
>>>>
>>>>
>>>> if (comparator == null) {
>>>> // Add the item to the view
>>>> viewIndex = view.add(item);
>>>> listListeners.itemInserted(FilteredList.this, viewIndex);
>>>> }
>>>>
>>>>
>>>> I modified the if statement to make a correction, but I'm not sure my patch is correct. Here's what I replaced the if statement with
>>>>
>>>> // Update the item in the view
>>>> int previousViewIndex = view.indexOf(item);
>>>>
>>>> if (previousViewIndex == viewIndex) {
>>>> // Update the item in the view
>>>> view.update(viewIndex, item);
>>>> } else {
>>>> // Remove the item from the view
>>>> Sequence<T> removed = view.remove(previousViewIndex, 1);
>>>> listListeners.itemsRemoved(FilteredList.this, previousViewIndex, removed);
>>>>
>>>> // Re-add the item to the view
>>>> viewIndex = view.add(item);
>>>> listListeners.itemInserted(FilteredList.this, viewIndex);
>>>> }
>>>>
>>>>
>>>> Again, I'm not sure this is correct but it seems to have resolved the issue I was having with duplicate items being added to the list.
>>>>
>>>> Cheers,
>>>>
>>>> Michael
>>>
>>
>
Re: FilteredList bugs
Posted by Greg Brown <gk...@mac.com>.
I went ahead and made this change - if we think there is still some value in maintaining FilteredList, maybe we can resurrect it in the "extras" project or something.
On Aug 27, 2010, at 2:40 PM, Greg Brown wrote:
> Now that I look at FilteredList again, I have to wonder if it is worth keeping. There's a lot of code there and, realistically, not that much value. Also, I believe it is the only remaining class that still has the potential for memory leaks if you forget to clear the source property (we have gotten rid of all the others or re-implemented them such that this isn't a problem).
>
> I think I might advocate eliminating this class for Pivot 2.0.
>
> G
>
> On Aug 25, 2010, at 8:22 AM, Greg Brown wrote:
>
>> Hi Michael,
>> Can you submit your patch via JIRA? This sounds like a bug, so you can simply file it as such.
>> Thanks!
>> Greg
>>
>> On Aug 24, 2010, at 8:01 PM, Michael Allman wrote:
>>
>>> There appears to be some inconsistency in FilteredList with how it behaves when the filter is null. I think if the filter is null, the list is supposed to show all values. In that case, the filter logic in the add() and insert() methods is incorrect. Rather than checking
>>>
>>> filter != null && filter.include(item)
>>>
>>> it should be checking
>>>
>>> filter == null || filter.include(item)
>>>
>>>
>>> Also, the itemUpdated() method in the listListener field sometimes adds items to the list which are already there. The problem seems to be in the following if block
>>>
>>>
>>> if (comparator == null) {
>>> // Add the item to the view
>>> viewIndex = view.add(item);
>>> listListeners.itemInserted(FilteredList.this, viewIndex);
>>> }
>>>
>>>
>>> I modified the if statement to make a correction, but I'm not sure my patch is correct. Here's what I replaced the if statement with
>>>
>>> // Update the item in the view
>>> int previousViewIndex = view.indexOf(item);
>>>
>>> if (previousViewIndex == viewIndex) {
>>> // Update the item in the view
>>> view.update(viewIndex, item);
>>> } else {
>>> // Remove the item from the view
>>> Sequence<T> removed = view.remove(previousViewIndex, 1);
>>> listListeners.itemsRemoved(FilteredList.this, previousViewIndex, removed);
>>>
>>> // Re-add the item to the view
>>> viewIndex = view.add(item);
>>> listListeners.itemInserted(FilteredList.this, viewIndex);
>>> }
>>>
>>>
>>> Again, I'm not sure this is correct but it seems to have resolved the issue I was having with duplicate items being added to the list.
>>>
>>> Cheers,
>>>
>>> Michael
>>
>
Re: FilteredList bugs
Posted by Greg Brown <gk...@mac.com>.
Now that I look at FilteredList again, I have to wonder if it is worth keeping. There's a lot of code there and, realistically, not that much value. Also, I believe it is the only remaining class that still has the potential for memory leaks if you forget to clear the source property (we have gotten rid of all the others or re-implemented them such that this isn't a problem).
I think I might advocate eliminating this class for Pivot 2.0.
G
On Aug 25, 2010, at 8:22 AM, Greg Brown wrote:
> Hi Michael,
> Can you submit your patch via JIRA? This sounds like a bug, so you can simply file it as such.
> Thanks!
> Greg
>
> On Aug 24, 2010, at 8:01 PM, Michael Allman wrote:
>
>> There appears to be some inconsistency in FilteredList with how it behaves when the filter is null. I think if the filter is null, the list is supposed to show all values. In that case, the filter logic in the add() and insert() methods is incorrect. Rather than checking
>>
>> filter != null && filter.include(item)
>>
>> it should be checking
>>
>> filter == null || filter.include(item)
>>
>>
>> Also, the itemUpdated() method in the listListener field sometimes adds items to the list which are already there. The problem seems to be in the following if block
>>
>>
>> if (comparator == null) {
>> // Add the item to the view
>> viewIndex = view.add(item);
>> listListeners.itemInserted(FilteredList.this, viewIndex);
>> }
>>
>>
>> I modified the if statement to make a correction, but I'm not sure my patch is correct. Here's what I replaced the if statement with
>>
>> // Update the item in the view
>> int previousViewIndex = view.indexOf(item);
>>
>> if (previousViewIndex == viewIndex) {
>> // Update the item in the view
>> view.update(viewIndex, item);
>> } else {
>> // Remove the item from the view
>> Sequence<T> removed = view.remove(previousViewIndex, 1);
>> listListeners.itemsRemoved(FilteredList.this, previousViewIndex, removed);
>>
>> // Re-add the item to the view
>> viewIndex = view.add(item);
>> listListeners.itemInserted(FilteredList.this, viewIndex);
>> }
>>
>>
>> Again, I'm not sure this is correct but it seems to have resolved the issue I was having with duplicate items being added to the list.
>>
>> Cheers,
>>
>> Michael
>
Re: FilteredList bugs
Posted by Greg Brown <gk...@mac.com>.
Hi Michael,
Can you submit your patch via JIRA? This sounds like a bug, so you can simply file it as such.
Thanks!
Greg
On Aug 24, 2010, at 8:01 PM, Michael Allman wrote:
> There appears to be some inconsistency in FilteredList with how it behaves when the filter is null. I think if the filter is null, the list is supposed to show all values. In that case, the filter logic in the add() and insert() methods is incorrect. Rather than checking
>
> filter != null && filter.include(item)
>
> it should be checking
>
> filter == null || filter.include(item)
>
>
> Also, the itemUpdated() method in the listListener field sometimes adds items to the list which are already there. The problem seems to be in the following if block
>
>
> if (comparator == null) {
> // Add the item to the view
> viewIndex = view.add(item);
> listListeners.itemInserted(FilteredList.this, viewIndex);
> }
>
>
> I modified the if statement to make a correction, but I'm not sure my patch is correct. Here's what I replaced the if statement with
>
> // Update the item in the view
> int previousViewIndex = view.indexOf(item);
>
> if (previousViewIndex == viewIndex) {
> // Update the item in the view
> view.update(viewIndex, item);
> } else {
> // Remove the item from the view
> Sequence<T> removed = view.remove(previousViewIndex, 1);
> listListeners.itemsRemoved(FilteredList.this, previousViewIndex, removed);
>
> // Re-add the item to the view
> viewIndex = view.add(item);
> listListeners.itemInserted(FilteredList.this, viewIndex);
> }
>
>
> Again, I'm not sure this is correct but it seems to have resolved the issue I was having with duplicate items being added to the list.
>
> Cheers,
>
> Michael