darksylinc / colibrigui

MIT License
23 stars 8 forks source link

Potential infinite loop in Colibri::Widget #56

Open edherbert opened 1 year ago

edherbert commented 1 year ago

Hi. As part of my work I've run into what seems to be a potential infinite loop in the Widget::getNextKeyboardNavigableWidget function.

https://github.com/darksylinc/colibrigui/blob/022089cb371cc84ecbbf8676d5f779bef241f1be/src/ColibriGui/ColibriWidget.cpp#L108

The problem seems to be that the two widgets register each other as their next widget so the code just loops back and forth between the two. In this scenario, I have a list of buttons representing combat moves. I also create a panel (Colibri::Renderable) to sit ontop of the button to represent cooldown. Seems the panel and the button register themselves as their next widgets. When the button is pressed it disables itself as part of the callback and the panel is set visible to perform the animation.

This loop is fairly rare as I haven't taken the time to dig further into it, but I imagine it relates to how I animate my cover panels. I'm wondering if something can be done to this while loop regardless to prevent it from potentially hanging like this? Perhaps add some extra exit cases?

darksylinc commented 1 year ago

Hi!

From what I can see, this would loop infinitely only if widgets create a circular connection, which needs to be done explicitly but should be a valid case.

e.g. if you have:

If you press "Down" on Button 4 to wrap around to Button 1 but all 4 buttons are disabled, it would loop infinitely. Note that by default Colibri won't register wrap-around behavior, you need to explicitly tell Colibri that you want Btn 4 to go back to Btn 1 when Down is pressed.

The problem seems to be that the two widgets register each other as their next widget so the code

That won't be a problem because when Btn 1 and Btn 2 register reciprocally, Btn 1 registers Btn 2 for "Down" button, and Btn 2 registers Btn 1 for "Up". The loop checks the next one in the same direction.

darksylinc commented 1 year ago

To be clear: I'm not saying this isn't a problem. I'm saying by default I don't see this accidentally triggering.

However there are valid usage cases (like wrap around behavior) that Colibri should support but could trigger the infinite loop you mention if the other conditions are also met (e.g. all buttons are disabled).