[fixed] OnEvent handling in gui elements

[CuteAlien]

Todays slow svn makes it somewhat difficult for me to check for recent changes to the gui, but I checked my local version here and stumpled upon something which would be easy to change, but all gui-element files would have to be checked.

Nearly all guielements have the following common functionality in OnEvent.
1. In the first lines they check if they are enabled. Usually like that:

Code: Select all

if (!IsEnabled)
	return Parent ? Parent->OnEvent(event) : false;

2. As last line they pass the event to the parent like:

Code: Select all

return Parent ? Parent->OnEvent(event) : false;

Well, actually a few elements still miss the first step. Those are: CGUIColorSelectDialog, CGUIComboBox, CGUIFileOpenDialog, CGUIListBox, CGUIMessageBox, CGUIModalScreen (which should also ignore events when !IsVisible), CGUIScrollBar, CGUISpinBox (my fault...), CGUIToolBar (but not sure if it would be needed there), CGUIWindow (a little tricky because of focus things).It's btw. ok in CGUIEditBox and CGUITable as it's checked there later on and the check is just done the other way round in CGUICheckBox.

But the exact same check is also done in IGUIElement. And I think it would generally be the cleaner way to replace those lines in all(!) gui-elements (including CGUIEnvironment) by the following (which is already done in some elements):

Code: Select all

// do first check only in those elements which care about IsEnabled
if (!IsEnabled)
	return IGUIElement::OnEvent(event);

// ... event stuff

return IGUIElement::OnEvent(event);

That way we can even add further code to IGUIElement::OnEvent and can be sure it's called (unless explicitly forbidden). Which is actually the functionality which I had needed and why I stumpled upon this ;-)

edit: forgot one thing (which I also didn't fix in my local version yet). Probably the same call should be done in all places where return false is currently use - not just at the start and end of OnEvent.

[bitplane]

I'm a bit behind on all your bug reports, but this one is now fixed

I put an "if (IsEnabled)" condition around the logic in most elements and called IGUIElement::OnEvent to pass the events. I think it's slightly tidier than having two IGUIElement::OnEvent calls in each element.

I didn't do the modal screen, but there's other bugs open for that one and I'll look at that shortly

[CuteAlien]

Thanks! I'm also rather busy right now, so it's probably also going to take me a while before I find time to check the fixes in the new version.

I remember seeing some other reports about the Modal Dialog. Was this about having two modals above each other? I did add extra checks for that situation when I had that problem here some time ago. So it looked like:

Code: Select all

case EGET_ELEMENT_FOCUSED:
	if ( event.GUIEvent.Caller != this 
		&& !isMyChild(event.GUIEvent.Caller) 
		&& (!event.GUIEvent.Element 
			|| (	event.GUIEvent.Element->getType() != EGUIET_MODAL_SCREEN
				&&	event.GUIEvent.Element->getType() != EGUIET_MESSAGE_BOX )
			)  
		)
		Environment->setFocus(this);
	return false;
case EGET_ELEMENT_FOCUS_LOST:
	if ( (!isMyChild(event.GUIEvent.Element) 
			&& (!event.GUIEvent.Element 
				|| (	event.GUIEvent.Element->getType() != EGUIET_MODAL_SCREEN
					&&	event.GUIEvent.Element->getType() != EGUIET_MESSAGE_BOX )
				)
		 )
		|| event.GUIEvent.Element == this)
	{
		MouseDownTime = os::Timer::getTime();
		return true;
	}
	else
		return IGUIElement::OnEvent(event);
	
	break;

But I'm still using an old irrlicht version in that project, so don't just copy-paste this. It's just to give you an idea how it could be solved in case you work on that.