Bug with forgotten SEvent::GUIEvent::Element = 0

[greenya]

Hello!

/* Please confirm that this is a bug */

When we working with SEvent, we check EventType first, and then we work with proper part of the event. For example:

Code: Select all

if (event.EventType == EET_GUI_EVENT)
{
       // here we can use/check event.GUIEvent freely
}

SEvent::SGUIEvent contains:

Code: Select all

gui::IGUIElement * Caller; // IGUIElement who called the event. 
gui::IGUIElement * Element; // If the event has something to do with another element, it will be held here. 
gui::EGUI_EVENT_TYPE EventType; // Type of GUI Event. 

Problem with Element!
In most cases it filled correctly: with a pointer if used and 0 if not used, BUT there is few places where this behavior omitted and this field left without attention.

Please note, currently we cannot write next:

Code: Select all

if (event.EventType == EET_GUI_EVENT)
{
       s32 callerID = event.GUIEvent.Caller->getID();
       s32 elementID = event.GUIEvent.Element != 0 ? event.GUIEvent.Element->getID() : -1;
}

because assignment of "elementID" gives AccessViolationException only for next gui events:
EGET_ELEMENT_LEFT
EGET_TABLE_HEADER_CHANGED
EGET_TABLE_CHANGED
EGET_TABLE_SELECTED_AGAIN
EGET_TREEVIEW_NODE_EXPAND
EGET_TREEVIEW_NODE_COLLAPS
EGET_TREEVIEW_NODE_DESELECT
EGET_TREEVIEW_NODE_SELECT

Next list of places shows where bug is:

CGUIEnvironment.cpp:

Code: Select all

	...
	if (Hovered != lastHovered)
	{
		SEvent event;
		event.EventType = EET_GUI_EVENT;

		if (lastHovered)
		{
			event.GUIEvent.Caller = lastHovered;
			event.GUIEvent.EventType = EGET_ELEMENT_LEFT;
			lastHovered->OnEvent(event); // <<<<<<<<<<<<<<<<<---- Element IS NOT FILLED HERE!!!
		}

		if ( Hovered )
		{
			event.GUIEvent.Caller  = Hovered;
			event.GUIEvent.Element = Hovered;
			event.GUIEvent.EventType = EGET_ELEMENT_HOVERED;
			Hovered->OnEvent(event);
		}
	}
	...

CGUITable.cpp

Code: Select all

	...
	case EGCO_CUSTOM:
		CurrentOrdering = EGOM_NONE;
		if (Parent)
		{
			SEvent event;
			event.EventType = EET_GUI_EVENT;
			event.GUIEvent.Caller = this;
			event.GUIEvent.EventType = EGET_TABLE_HEADER_CHANGED;
			Parent->OnEvent(event); // <<<<<<<<<<<<<<<<<---- Element IS NOT FILLED HERE!!!
		}

		break;

	...

	if (changed)
	{
		SEvent event;
		event.EventType = EET_GUI_EVENT;
		event.GUIEvent.Caller = this;
		event.GUIEvent.EventType = EGET_TABLE_HEADER_CHANGED;
		Parent->OnEvent(event); // <<<<<<<<<<<<<<<<<---- Element IS NOT FILLED HERE!!!
	}

	...

	// post the news
	if (Parent && !onlyHover)
	{
		SEvent event;
		event.EventType = EET_GUI_EVENT;
		event.GUIEvent.Caller = this;
		event.GUIEvent.EventType = (Selected != oldSelected) ? EGET_TABLE_CHANGED : EGET_TABLE_SELECTED_AGAIN;
		Parent->OnEvent(event); // <<<<<<<<<<<<<<<<<---- Element IS NOT FILLED HERE!!!
	}

	...

CGUITreeView.cpp

Code: Select all

	...
	SEvent					event;

	event.EventType			= EET_GUI_EVENT;
	event.GUIEvent.Caller	= this;
													// <<<<<<<<<<<<<<<<< event.GUIEvent.Element = 0; <<<<<< MUST BE HERE!!!
	xpos -= AbsoluteRect.UpperLeftCorner.X;
	ypos -= AbsoluteRect.UpperLeftCorner.Y;
	...

All this places were found by searching pattern "GUIEvent.Caller" across all *.cpp and *.h files in "source" directory.
At all this places should be done next patch: after each "event.GUIEvent.Caller = " line, should be added line "event.GUIEvent.Element = 0;".


Thank You!

[CuteAlien]

Mhm, I think the original idea was just to set that when it's used. But yeah - it's better to initialize it.

Thanks for finding all the places, I'll try to find time for it in the next days (a little too late already today for me, stupid body need a few hours sleep).

[greenya]

Mhm, I think the original idea was just to set that when it's used.

Here is an example of correct filling of Element:

Code: Select all

		SEvent e;
		e.EventType = EET_GUI_EVENT;
		e.GUIEvent.Caller = Focus;
		e.GUIEvent.Element = 0;
		e.GUIEvent.EventType = EGET_ELEMENT_FOCUS_LOST;

Element not used for EGET_ELEMENT_FOCUS_LOST, but its set 0, so it will not contain garbage.

I think there are 2 correct ways:
- simply set Element to 0;
OR
- write a note in documentation on Element with list of GUI event types where it actually used, since user cannot check is it filled correctly or not.

[CuteAlien]

Yeah, don't worry, I will change it. Although improving event documentation has also been on my todo already for a long time. Some day I'll get to it (basically each gui-element should document which events it does use, nothing tricky - I just didn't get to it yet).

[CuteAlien]

I've fixed it in the 1.7 branch, svn trunk will follow on next merge.

[greenya]

Thank you