Report and tolerate errors when parsing strings as values for settings that don't exist
ClosedPublic
Actions

Authored by elexis on Sep 16 2019, 5:37 PM.

Details

Reviewers: None
Commits: rP22911: Report and tolerate XML GUI object setting errors when parsing XML attribute…

Summary

When there is a syntax error in an XML file, such as the example from D1574 which was the typo reported in https://code.wildfiregames.com/rP21847#30949, then there will be a crash as of rP22796 (trying to access a map item that doesn't exist and cant be healthily default-constructed).
In the string parsing function previous to rP22796 (IGUIObject::SetSetting(const CStr& Setting, const CStrW& Value, const bool& SkipMessage)), it did simply not report it for most calls, which is wrong as well.
For example if a translatable attribute was not found, then it should inform the XML author.

For example the silent ignoring of the error in this case:

	CStrW caption(Element.GetText().FromUTF8());
	if (!caption.empty())
		object->SetSettingFromString("caption", caption, false);

was a intentional choice in rP290:

// There is no harm if the object didn't have a "caption"

However there is harm to having a caption there, since the XML author specified something that he assumes will have an effect, will maintain and optimize it when in truth it is not used.
Compare also to rP22792 showing a LOGWARNING when the style specifies a value for as setting that does not exist.

rP14953 introduced the elmt_translatableAttribute code without error reporting.
rP74 originally threw an error, rP1494 changed it to a return value, but most/many callers did not test for that value.

Test Plan

Notice that we can't throw a JS exception unless throwing a C++ exception, catching that in a different class. Notice that a JS error might not necessarily express the right connotation, because the error is not on the JS side (not the PushGUIPage call).
If one wants greater error support than that, it should probably become a validation error, because the error is in the XML file.
For completeness, notice that most of the SetSetting calls come from IGUIObject code and not from the XML loading code, i.e. it is a C++ code error if calling SetSetting for a setting that does not exist, but it is an XML author (user) error for SetSettingFromString. (That is excluding the few FromString occurrences that seem like calls that should use SetSetting instead, but thats offtopic.)

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

elexis created this revision.Sep 16 2019, 5:37 PM

This gets a 'Yup' for me, probably won't spend the time to formally review it though.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/720/display/redirect

elexis edited the summary of this revision. (Show Details)Sep 16 2019, 5:50 PM

elexis edited the test plan for this revision. (Show Details)

elexis edited the summary of this revision. (Show Details)Sep 16 2019, 5:59 PM

I guess we don't really have a form other than the accept button and arbitrary text.
But the completeness test of a review might consider that one could possibly remove more warnings / errors, for example in GUITooltip.cpp:

	if (usedobj->SettingExists("caption"))
	{
		const CStrW& text = obj->GetSetting<CStrW>(m_IsIconTooltip ? "_icon_tooltip" : "tooltip");
		usedobj->SetSettingFromString("caption", text, true);
	}
	else
	{
		LOGERROR("Object '%s' used by tooltip '%s' must have a caption setting!", usedobj->GetPresentableName().c_str(), style.c_str());
		return;
	}

But this error message is a bit more specific and leads the XML author of broken tooltips more quickly to the solution for his XML error.

The other case is the LOGWARNING from about invalid styles, that should definitely remain a specific message.

One could argue that CText::MouseOverIcon() and the other callers should also have a specialized error.

But when I initially wrote that, it seemed like making the code much longer than needed.
Especially catching only in the caller means that new callers also must catch it if they dont want to crash, whereas this diff will report and return false (still catchable) but not crash.
So under the feature complete solutions this seems the solution with least implementation cost (GUITooltip can be considered special enough and the others not special enough, whatever)

This revision was not accepted when it landed; it landed in state Needs Review.Sep 16 2019, 6:14 PM

This revision was landed with ongoing or failed builds.

Closed by commit rP22911: Report and tolerate XML GUI object setting errors when parsing XML attribute… (authored by elexis). · Explain Why

This revision was automatically updated to reflect the committed changes.

elexis added a commit: rP22911: Report and tolerate XML GUI object setting errors when parsing XML attribute….