#334 fix CGUIScrollBar TrayClick

[Mercury233]

OnPostRender is called during the rendering process. This stage generally should not dispatch events, and it definitely should not process events. If GUI controls are added or removed while handling events, it may invalidate the iterator used by IGUIElement::OnPostRender. In addition, developers would not expect GUI event handling and rendering to occur simultaneously, and may therefore use mutexes for event handling and GUI rendering. In such cases, dispatching events from OnPostRender could result in double locking.

At present, the event dispatching in CGUIScrollBar::OnPostRender is the only occurrence of this pattern in the codebase, and it poses a real risk. CGUIEnvironment::OnPostRender also adds and removes controls, but the current implementation does not appear to introduce any actual risk.

The event dispatching in CGUIScrollBar::OnPostRender currently exists to implement the behavior where clicking on the empty area of the scrollbar gradually moves the slider toward the clicked position. After this change, clicking on the empty area of the scrollbar will immediately move the slider directly to the clicked position.

[Michael Zeilfelder]

To remove elements safely in OnPostRender you can use addToDeletionQueue (instead of usual remove()). If you really need to add an element in that event then delay it yourself somehow.

Not sure why you would put mutexes in the event-handler, that sounds pretty specific.

I agree it can be a bit risk, but not exactly one that you can't avoid. And I also don't think sending events from animations is really bad. Events can be send from anywhere.

If you have some case where you really can't work around this I'd consider changing it somehow. But I'd like to keep that animation intact as it's pretty usual behavior in UI. If you really find no other way - maybe try to ignore that event and poll the position later. Or just remember the event and handle it later.

[Mercury233]

Thanks for taking the time to review the patch and explain your thoughts.

Since I'm already using my own modified fork (to support IME, etc.), I don't mind if this patch isn't merged. I submitted it mainly in the hope of making the upstream architecture a bit safer and more consistent.

Just to clarify my motivation: CGUIScrollBar is the only control in the entire GUI system that uses OnPostRender and dispatch events. From an API design perspective, I felt it's generally better to eliminate an architectural anti-pattern rather than asking users to navigate around the risks.

I personally feel that for game UIs, it might be better if clicking on an empty area of the scrollbar immediately jumps to the target position. However, I completely understand your desire to keep the current gradual scrolling behavior as it's a standard UI feature.

If you're open to it, I can look into other ways to implement the gradual scroll without using OnPostRender.

[Michael Zeilfelder]

I don't consider it an anti-pattern, just a new one not used yet in other places. Irrlicht doesn't have other animations in UI yet, but events at end of animations are not really such an uncommon thing in libraries. And one place always has to start.

[Michael Zeilfelder]

I get the reasoning - it's pretty closely related to https://sourceforge.net/p/irrlicht/bugs/328/. Just removing animation feature is not the solution. So unless we have really a better solution using addToDeletionQueue etc for such events is an OK enough workaround.