BUG: Listbox::findItemWithText skip start_item

[zanir]

Code: Select all

/*************************************************************************
   Search the list for an item with the specified text
*************************************************************************/
ListboxItem* Listbox::findItemWithText(const String& text, const ListboxItem* start_item)
{
   // if start_item is NULL begin search at begining, else start at item after start_item
   size_t index = (!start_item) ? 0 : (getItemIndex(start_item) + 1);

   while (index < d_listItems.size())
   {
      // return pointer to this item if it's text matches
      if (d_listItems[index]->getText() == text)
      {
         return d_listItems[index];
      }
      // no matching text, advance to next item
      else
      {
         index++;
      }

   }

   // no items matched.
   return 0;
}


I think is no logical to skip start_item in this function. For example:
pCombo->findItemWithText(pCombo->getText(),pCombo->getListboxItemFromIndex(0)); give different result as
pCombo->findItemWithText(pCombo->getText(), NULL); The solution is delete + 1 from first line in function.

[Rackle]

When you pass NULL you include the first element in the search.

Let's say that this first element matches your search; the function then returns that first element. However you now want to find the next entry that matches your text. You call this function again, passing the previously found item rather than null. Without the +1 the function would return that same item rather than searching the following elements.

Here's a concrete example, with the numbers representing the index position:
0. skip
1. text
2. text
3. skip
4. text

The function is called with:
ListboxItem* item = findItemWithText("text", 0)
this returns the item at index 1. Then you call the function with:
item = findItemWithText("text", item)
which now returns the item at index 2. Calling the function again with:
item = findItemWithText("text", item)
returns the last match, at index 4.

The d_listItems[index]->getText() == text test is a bit simplistic. I'd rather have a regular expression test such that a search for the text "*text*" would match "text 1", "text 2", "abracadabra text", etc.

[zanir]

I understand but maybe const ListboxItem* start_item should be named differently. For example start_skipped_item, start_and_skipped_item. Somebody can create code like:

Code: Select all

ListboxItem* item = findItemWithText("text", 0);
while(getItemIndex(item)+1<getItemCount() && item)
    item = findItemWithText("text", getListboxItemFromIndex(getItemIndex(item)+1));


He/she will wonder why items are skipped and will find a bug.