Fix: Unknown nested elements in XML packages elements (like Host etc.) will cause erasure of settings

[Delwing]

Brief overview of PR changes/additions

Having unknown elements in XML, might cause clearing of already properly set elements. This shouldn't happen, as we're flattening Host package, but still good to fix that issue. Example:

<Host autoClearCommandLineAfterSend="no" HighlightHistory="yes" printCommand="yes" USE_IRE_DRIVER_BUGFIX="no" mUSE_FORCE_LF_AFTER_PROMPT="no" mUSE_UNIX_EOL="no" mNoAntiAlias="no" mEchoLuaErrors="yes" runAllKeyMatches="no" AmbigousWidthGlyphsToBeWide="auto" mRawStreamDump="yes" mIsLoggingTimestamps="no" ...>
			<name>Test</name>
			<mInstalledPackages />
			...
			<unknowns>
				<unknown>1</unknown>
				<unknown>2</unknown>
			</unknowns>
			<stopwatches />
		</Host>

initially will set all values correctly in Host object, second run will happen, like those values are never set, because it will treat unknowns node just like it was Host node.

Motivation for adding to Mudlet

Fixing

Other info (issues closed, discussion etc)

Same applies to other elements, like Triggers, Aliases etc.

Release post highlight

[add-deployment-links[bot]]

Hey there! Thanks for helping Mudlet improve. :star2:

Test versions

You can directly test the changes here:

• linux: https://make.mudlet.org/snapshots/ab62bd/Mudlet-4.16.0-testing-pr5895-43bb5707-linux-x64.AppImage.tar
• osx: https://make.mudlet.org/snapshots/fa3cf3/Mudlet-4.16.0-testing-pr5895-43bb5707.dmg
• windows: https://make.mudlet.org/snapshots/dbf3d4/Mudlet-4.16.0-testing-pr5895-43bb5707-windows.zip

No need to install anything - just unzip and run. Let us know if it works well, and if it doesn't, please give details.

[github-actions[bot]]

	Messages
:heavy_check_mark:	PR type: Fix

Generated by :no_entry_sign: dangerJS against 895d425e5057ee49bbb644f59d7873daeaac2266

[github-actions[bot]]

clang-tidy review says "All clean, LGTM! :+1:"

[vadi2]

@wiploo @mpconley could you test using this version instead of PTB, and then switching back to release?

[wiploo]

Unfortuntelly I have the same problem with this PR. I lost all profile settings.

[Delwing]

@SlySven unless I misunderstood something here, I'd say this is desired...

While /MudletPackage/HostPackage/Host/wrapAt is correct place to check then some accidental /MudletPackage/HostPackage/Host/DummyWindow/wrapAt because we don't know DummyWindow node, we should not care what's inside, it might contain values that will override our correct values. Not very probable. that they will contain anything valid, but still.

[Delwing]

Unfortuntelly I have the same problem with this PR. I lost all profile settings.

Just to be clear. It can fix things between PTB AND release branch 4.15, not the current version 4.14.1. Did you switch between those two? Switching to 4.14.1 will still cause erasure.

[vadi2]

Switching to 4.14.1 will still cause erasure.

We can't have that, not in Mudlet anyway - losing people's stuff isn't OK

[Delwing]

Switching to 4.14.1 will still cause erasure.

We can't have that, not in Mudlet anyway - losing people's stuff isn't OK

Once again, I can't fix previous versions.

[SlySven]

While /MudletPackage/HostPackage/Host/wrapAt is correct place to check then some accidental /MudletPackage/HostPackage/Host/DummyWindow/wrapAt because we don't know DummyWindow node, we should not care what's inside, it might contain values that will override our correct values. Not very probable. that they will contain anything valid, but still.

Ah, yeah, I get you, in that case there is a (void) QXmlStreamReader::skipCurrentElement() method that should be called when the parser is on the startElement of the unknown item (I believe it is still possible to read the name and attributes of that item at that point) so as to skip the children and move the parser to the matching endElement ready to go around that while {...} loop again and then return...

[Delwing]

Sure, might be wise to just skip it entirely and not produce further unknown lines.

[vadi2]

It doesn't look like we got around to testing this - I don't want to merge this last-minute into the update, let's cover it in the next update.

[ghost]

👇 Click on the image for a new way to code review

• Make big changes easier — review code in small groups of related files

• Know where to start — see the whole change at a glance

• Take a code tour — explore the change with an interactive tour

• Make comments and review — all fully sync’ed with github

  Try it now!

Legend

[SlySven]

I have tested the current code and when I fed it a doctored game save file with the following unexpected ("badElement") data inserted under the <host> element (each line of text ends with a line-feed and then multiple tabs {3 or 4} indent the text):

			<mInstalledPackages>
				<string>deleteOldProfiles</string>
				<string>echo</string>
				<string>run-lua-code-v5</string>
				<string>eventTest_labelDeletion</string>
				<string>NonLatin1Tests</string>
			</mInstalledPackages>
			<badElement type="Not good" ignorable="Oh yeah!">
				<string>this is not supposed to be read</string>
				<string>nor this string</string>
				<integer>42</integer>
			</badElement>
			<url>34.217.254.118</url>
			<serverPackageName>nothing</serverPackageName>
			<serverPackageVersion>-1</serverPackageVersion>

this provoked the following command line debug output from Mudlet:

[LOADING PROFILE]: "/home/stephen/.config/mudlet/profiles/WoTMUD_Jomin/current/bad_elements.xml"
XMLimport::readUnknownHostElement() ERROR: UNKNOWN Host Package Element, name: "" and content: "\n\t\t\t\t"
XMLimport::readUnknownHostElement() ERROR: UNKNOWN Host Package Element, name: "string" and content: ""
XMLimport::readUnknownHostElement() ERROR: UNKNOWN Host Package Element, name: "" and content: "\n\t\t\t\t"
XMLimport::readUnknownHostElement() ERROR: UNKNOWN Host Package Element, name: "string" and content: ""
XMLimport::readUnknownHostElement() ERROR: UNKNOWN Host Package Element, name: "" and content: "\n\t\t\t\t"
XMLimport::readUnknownHostElement() ERROR: UNKNOWN Host Package Element, name: "integer" and content: ""
XMLimport::readUnknownHostElement() ERROR: UNKNOWN Host Package Element, name: "" and content: "\n\t\t\t"
XMLimport::readUnknownHostElement() ERROR: UNKNOWN Host Package Element, name: "badElement" and content: ""

So I don't think https://github.com/Mudlet/Mudlet/pull/5895#discussion_r810469114 is exactly correct...

[SlySven]

:warning: Please do not "update branch" until a decision on https://github.com/Delwing/Mudlet/pull/83 is taken... :crossed_fingers:

[Delwing]

@SlySven merged that one of yours into that one - one Vadi's remark stayed unresolved, but since this is trivial thing to change if needed, I decided not to wait - fixing issue is most crucial here.

[vadi2]

@SlySven mind having another look at this?

[SlySven]

Will do, as soon as I can (but that might be a day or two)...

[SlySven]

@SlySven mind having another look at this?

Done as: https://github.com/Delwing/Mudlet/pull/87 it needs merging into Delwing's PR by him. Note that only https://github.com/Delwing/Mudlet/pull/87/commits/591b0e8d394a4f8cc9115d3d78d0d6f407d87e64 is the actual change - all the preceding ones are the result of merging in the changes to the development branch - and will "evaporate" (:crossed_fingers:) when this PR eventually (very quick one hopes) gets into the development branch...!

[SlySven]

Okay, so @vadi2 has merged in something else into development whist I was putting that second PR together so it has slipped a little bit behind - hence the "This branch is out-of-date..." is still there. :roll_eyes: but it should be good to go now. :pleading_face: