ListboxTextItem not invalidated upon TextColour change

[BigG]

Hello,

I'm not sure if this is a bug or intended behaviour. However, as pretty much all widgets are invalidated and redrawn upon property changes, it only seems natural to me that calling setTextColour() on a ListboxTextItem would invalidate it. In my case, the ListboxTextItem is attached to a MultiColumnList. After changing the text colour, I have to manually call invalidate() on that MCL in order to see the new colour.

I currently use the outdated version 0.8.2, but I have read all patch notes up to the most recent release, and could not find any fixes related to this case. This is my essential log section:

Code: Select all

---- Version: 0.8.2 (Build: Nov 26 2013 Debug Microsoft Windows MSVC++ 11.0 64 bit) ----
---- Renderer module is: CEGUI::OgreRenderer - Official OGRE based 2nd generation renderer module. ----
---- XML Parser module is: CEGUI::ExpatParser - Official expat based parser module for CEGUI ----
---- Image Codec module is: OgreImageCodec - Integrated ImageCodec using the Ogre engine. ----
---- Scripting module is: None ----

Thank you for your time and looking forward to a response,
BigG

[Ident]

Definitely sounds like a bug. I will look into it soon and report back.

[BigG]

Looks like it's a simple bug, which is only present for setTextColours(const ColourRect&).

Confer ListboxTextItem.h:

Code: Select all

void   setTextColours(const ColourRect& cols)         {d_textCols = cols;}

in comparison to ListboxTextItem.cpp

Code: Select all

void ListboxTextItem::setTextColours(Colour top_left_colour,
                                     Colour top_right_colour,
                                     Colour bottom_left_colour,
                                     Colour bottom_right_colour)
{
   d_textCols.d_top_left      = top_left_colour;
   d_textCols.d_top_right      = top_right_colour;
   d_textCols.d_bottom_left   = bottom_left_colour;
   d_textCols.d_bottom_right   = bottom_right_colour;

    d_renderedStringValid = false;
}


I'm not really familiar with the internals of CEGUI, but it seems that the overload taking a ColourRect simply does not invalidate the rendered string, and thus does not trigger a redraw.

//EDIT:
While the above appears to be a bug, fixing it still didn't solve the issue

//EDIT2:
I tried out applying different property changes to the ListboxTextItems in the grid, and actually none of them appears to invalidate the parent MCL - not even the very basic things like SetText(). It could be that this behaviour is in fact intentional - maybe to avoid multiple calls to invalidate() when modifying several items at once?

[Ident]

Hm, that is interesting. I know that timotei worked on the MLC and other widgets as part of his GSOC project last year, I asked him to look at this: Maybe it has been changed in default branch since then or maybe he knows a good workaround. I am not familiar with MLC much so I dont know what the regular way to update its elements is.

Is there anything in the docs regarding this? I see there is a function handleUpdatedItemData(), does this do what you need? Clearly it is not optimal to do this on every tiny change (all items affected). Maybe manual invalidation is indeed the way to go. This should be documented better then.

[BigG]

Regarding the handleUpdatedItemData() - yes, it does do what I want. However, it's name sounds like an internal interface to me. In fact, this function is present in all Widgets that can contain ListboxItems, but not called by the ListboxItem upon a property change. Strange enough, the handleUpdatedItemData() is also present for MenuBase - but in this case, it is called by the contained MenuItem when its properties are changed.

As far as I could find, there's zero information about this behaviour in the documentation. I think that if it is indeed intended, then it would be a good idea to explicitly state that - unlike the rest of CEGUI - changing properties of ListboxItems does not invalidate the parent container. Furthermore, I think that those inconsistencies between MenuItems and ListboxItems should be removed, because they are very similar elements.

I understand that the performance impact of immediate and automated updating of the parent widget upon each property change of a ListboxItem can cause large overhead - which is why I propose to, in some way, allow both: immediate and deferred updating. Maybe something similar to old-school OpenGL?

Deferred updating:

Code: Select all

Listbox->beginUpdate(); //This disables automated internal updates
ListboxItem1->setText("Lorem Ipsum...");
ListboxItem2->setTextColours(....);
...
Listbox->endUpdate(); //This re-enables automated internal updates and triggers an update to take the changes since beginUpdate() into account

While, when simply changing individual ListboxItems, or when accepting the overhead of multiple updates, you could just write

Code: Select all

ListboxItem1->setText("Lorem Ipsum...");
ListboxItem2->setTextColours(....);

I think a solution similar to this would provide the benefits of both worlds: the convenience of automated updates and the consistency to the rest of the API's behaviour, as well as the performance benefit (or rather overhead reduction) of deferred updating.

Greetings,
BigG

[Ident]

Please let's not add anything that reminds me of the deprecated fixed-function pipeline of OpenGL, it makes me suffer from horrible night- and daymares .

The idea itself is good though. We already use such "muting" for the event-firing of CEGUI windows. So i suggest the function should have the same functionality but instead be declared as "void muteAutomaticUpdates(bool mute_auto_updates)" or similar.

Are you able to provide a fix for this in the form of a pull request?

In any case we should make a ticket for this, once we are done with the migration of our issue tracker to bitbucket (I will notify you in this thread)

[BigG]

Heh, writing that draft made me a bit nostalgic of the good (?) ol' times What you proposed sounds reasonable. I'll add the (default: disabled I guess) muting to all widgets that can contain a ListboxItem or a MenuItem and make both consistent to each other. Will take some time though, as there are several exams to be written

[Ident]

Good luck with your exams! There is no rush for patches, of course sooner is better but in general we are happy about PRs no matter if they are done quickly or slowly . My todo list is pretty long at the moment and I got very, very large and also important changes in it (for the 1.0 release) and I will not get to work on them until I finished my thesis and last exams as well. Any bugfixes that are user-provided allow us to spend more time on general improvements in the limited free time we manage to spend on CEGUI, so we are very thankful for such contributions.

Btw., if you add members to existing and exposed classes this will break ABI compatibility. You will have to use v0 branch if adding members is needed.

[Ident]

Any news on this?

[BigG]

Hi, I had my last exam a few days ago, so unfortunately I haven't gotten to any programming yet. I think I'll use the easter days to finally get back to working on my project again and start by looking into this issue

[Ident]

Sounds good, just wanted to know if you are still around for this change.

[BigG]

Upon further inspection, the matter seems to be more complex than we've initially thought. The only information a ListboxItem knows about its owner is a const Window* to him. However, the different ListboxItem container classes (Listbox, MultiColumnList, ...) do not share a common ancestor class (something like "ListboxBase" in analogy to "MenuBase" for menu-related stuff). For this reason, in order to call the appropriate handleUpdatedItemData() function from a ListboxItem, one would have to find out the correct type of its owner by brute-force comparison to all "known" Listbox containers via RTTI or something else - which would be incredibly ugly and obviously infeasible.

I assume that this very problem is why it was initially implemented so that MenuItems correctly notify their container (as there is a MenuBase class), but ListboxItems do not (as there is no ListboxBase class). As we've discussed before, this inconsistency between these two very similar structures should be removed - however, this would require some significant (probably even API-breaking) changes. Possibly they could even be unified to a certain extent? I mean both share the functionality of a parent containing multiple Item elements... In that case, I'd imagine a class inheritance structure like this:

Code: Select all

            Item
         /         \
  ListboxItem     MenuItem
       |
ListboxTextItem


Code: Select all

                             ItemContainer
                /                                  \
ListboxItemContainer (aka ListboxBase)       MenuItemContainer (aka MenuBase)
        /              \                         /            \
    Listbox      MultiColumnList             Menubar       PopupMenu


I haven't taken a look at timotei's changes yet. Possibly this issue has already been resolved in the default branch due to his work. If not, it might be a good idea if someone (me?) made the necessary changes.

Looking forward to opinions on this

//EDIT: I just found that there already is an inheritance structure similar to what I've proposed. Apparently ItemEntry / ItemListBase represent Item / ItemContainer of my proposal. Then MenuBase and MenuItem inherit ItemListBase / ItemEntry respectively. However, in the "Listbox" branch of the inheritance diagram, there's only ItemListbox - which I've never used before It seems to be a generic list box for items other than just text.
At this point, I'm even more confused about why there's a Listbox (for text items only) and an ItemListbox (for any window object), but they are implemented entirely independently from each other, resulting in a lot of duplicate code

[Ident]

ItemEntry is the future and afaik this (and derivative) is what should be used in 1.0 instead of this weird mix of classes. I dont know the current state after timotei's work in 1.0 however.

[Ident]

Timotei notified me that ItemEntry is NOT the future. Maybe he will find time to reply here by himself, because I do not know more about this.