Bug in ItemListbox::findSelectedItem ??? + Suggestions

If you found a bug in our library or on our website, please report it in this section. In this forum you can also make concrete suggestions or feature requests.

Moderators: CEGUI MVP, CEGUI Team

Alain B
Not too shy to talk
Not too shy to talk
Posts: 21
Joined: Tue Aug 14, 2012 16:25

Bug in ItemListbox::findSelectedItem ??? + Suggestions

Postby Alain B » Tue Aug 14, 2012 19:39

Hello,

This is my first post here. I think I have found a bug in CEGUI 0.7.7, that also affects lastest versions. I searched for the guilty method's name on this forum and CEGUI's Mantis, with no success. Please excuse me if this issue has already been adressed.

Also, the wiki said to directly post issues on Mantis, a tool I am familiar with, but I couldn't find a link to a page permitting to report a new issue.

The alleged bug I found is caused by these few lines of code in GEUIItemListbox.cpp (or ItemListbox.cpp in later versions) :

Code: Select all

/************************************************************************
    Get a pointer to the first selected item starting from the given index
************************************************************************/
ItemEntry* ItemListbox::findSelectedItem(size_t start_index) const
{
    size_t max = d_listItems.size();
    if (start_index >= max)
    {
        return 0;
    }

    for (size_t i=start_index; i<max; ++i)
    {
        ItemEntry* li = d_listItems[i];
        if (li->isSelected())
        {
            d_nextSelectionIndex = i; // <----- Issue caused by this line
            return li;
        }
    }

    return 0;
}

/************************************************************************
    Get a pointer to the first selected item
************************************************************************/
ItemEntry* ItemListbox::getFirstSelectedItem(size_t start_index) const
{
    if (!d_multiSelect)
    {
        return d_lastSelected;
    }
    return findSelectedItem(start_index);
}

/************************************************************************
    Get a pointer to the next selected item using internal counter
************************************************************************/
ItemEntry* ItemListbox::getNextSelectedItem() const
{
    if (!d_multiSelect)
    {
        return 0;
    }
    return findSelectedItem(d_nextSelectionIndex);
}


From what I understand, getFirstSelectedItem and getNextSelectedItem call findSelectedItem to do their main job.
The latter method updates the d_nextSelectionIndex member variable in order to keep the ItemListbox's internal state coherent for the next call to getNextSelectedItem.

However, a quick review of the body of findSelectedItem makes me think that successive calls to it will always return the first selected item in the list, without ever proceeding to the next items.
That, in turn, not only causes getNextSelectedItem not to behave as written in the documentation, but moreover causes an infinite loop in my project's code : D

A quick fix would be to replace the commented line above by :

Code: Select all

            d_nextSelectionIndex = i + 1;


Moreover, I'd like to add two suggestions to this post :

1. I think the first if block in findSelectedItem item is useless. Without it, the method's behavior will be exactly the same. I might be wrong, but a modern compiler wouldn't even proceed with allocation of the loop variable before doing the check on the loop condition.

2. Keeping an internal state variable inside the ItemListbox looks bad to me. Shouldn't this state be conserved in an iterator object pointing to the listbox ?

One last question : is there a way to use an iterator to cycle through an ItemListbox's selected values ? I had to write one myself for my project.

Thanks for reading !

User avatar
CrazyEddie
CEGUI Project Lead
Posts: 6760
Joined: Wed Jan 12, 2005 12:06
Location: England
Contact:

Re: Bug in ItemListbox::findSelectedItem ??? + Suggestions

Postby CrazyEddie » Wed Aug 15, 2012 07:06

Hi,

Thanks for the report and suggestions.

I'll cut down on a lot of waffle by saying that you're pretty much correct on all counts; meaning the bug, the useless test in ItemListbox::findSelectedItem and the keeping of state instead of using an iterator of some kind ;)

With regards to mantis, you can log in with the same details you use to log in to the forum, then you will be able to create tickets and such. So if you could log in there and add a couple of tickets (one for the bug, another for the suggestion for the iterator) that would be cool, and if you were to be willing to share the iterator you created for this - for possible inclusion in cegui (this needs updating a bit, but see http://www.cegui.org.uk/api_reference/devel.html) - that would be even more awesome.

Thanks again,

CE.

Alain B
Not too shy to talk
Not too shy to talk
Posts: 21
Joined: Tue Aug 14, 2012 16:25

Re: Bug in ItemListbox::findSelectedItem ??? + Suggestions

Postby Alain B » Wed Aug 15, 2012 17:29

Thank you for your reply.

The iterator I created was a wrapper around the existing CEGUI code, i.e. it calls getFirstSelectedItem and getNextSelectedItem.
I don't think it would be interesting to include in CEGUI as such, as it was more of a workaround for a missing feature than a generic development.

However, I could write (and test locally) a useful iterator, by accessing directly findSelectedItem, and saving state related data in the iterator itself. I'll do that when I have some time on hand.

Right now, the project I am using CEGUI for is entirely developped on my spare time. As such, I can't really plan ahead when I'll have time to work on CEGUI related issues. However, I will use Mantis whenever needed and maybe propose code for inclusion if I can.

I will add the two tickets in Mantis (with a reference to this thread) in the next few days. I didn't think of using my forum credentials to access it, as I created an account here only _after_ failing to use Mantis.

EDIT: tickets added http://www.cegui.org.uk/mantis/view.php?id=899 and http://www.cegui.org.uk/mantis/view.php?id=900

Moreover, it's my first issue with CEGUI: I was quite surprised to find such a bug (and actually lost a lot of time checking my own code before reading the CEGUI source). I would have thought an issue like this one would have been encountered by other people before me. I am not complaining or trolling, only surprised.

EDIT: Mantis hates my guts !
After logging in, when I try to access the "Report Issue" link ( http://www.cegui.org.uk/mantis/bug_report_page.php ), I get redirected to http://www.cegui.org.uk/mantis/my_view_page.php
Tough luck...

EDIT2: Works all right now.
EDIT3: Mantis tickets added http://www.cegui.org.uk/mantis/view.php?id=899 and http://www.cegui.org.uk/mantis/view.php?id=900

User avatar
CrazyEddie
CEGUI Project Lead
Posts: 6760
Joined: Wed Jan 12, 2005 12:06
Location: England
Contact:

Re: Bug in ItemListbox::findSelectedItem ??? + Suggestions

Postby CrazyEddie » Fri Aug 17, 2012 08:44

Hi,

Thanks for the tickets, this will ensure this does not get lost and forgotten about :)

Right now, the project I am using CEGUI for is entirely developped on my spare time. As such, I can't really plan ahead when I'll have time to work on CEGUI related issues.

I understand, CEGUI itself is my "spare time" project, and the same qualification applies ;)

Moreover, it's my first issue with CEGUI: I was quite surprised to find such a bug (and actually lost a lot of time checking my own code before reading the CEGUI source). I would have thought an issue like this one would have been encountered by other people before me. I am not complaining or trolling, only surprised.

Yeah, I'm surprised the issue has not been encountered before, too. Occasionally that does happen, where such an 'obvious' bug has been missed for years, and I find it almost unbelievable that it could be missed for that long (so thanks for finally finding it!)

CE.


Return to “Bug Reports, Suggestions, Feature Requests”

Who is online

Users browsing this forum: No registered users and 1 guest