(release_30)bugFix: bad parsing of map room/exit sizes + other XML import/export … by SlySven · Pull Request #373 · Mudlet/Mudlet

SlySven · 2017-01-21T20:14:21Z

…issues

This contains the BugFix and other elements of Pull Request #372 made onto the development branch but without the code cleanup/layout changes.

Fix the parsing of the "mRoomSize" and "mLineSize" ATTRIBUTES to the "Host" element of Mudlet XML files. The reader code was treating the stored strings as integers when they were floating point decimal numbers, whilst this is not an issue for Host::mLineSize which currently DOES operate as a whole number without a decimal part it BREAKS "mRoomSize" which ranges from 0.1 to 1.1 in 0.1 steps. An attempt was made to fix the fault on 2011-06-09 in commit: 7b4da59
but that added two sub-elements to the Host element to store the same data rather than correcting the original fault. This commit cannot remove those two sub-elements because that would currently break saved XML files if they were then used in Mudlet versions without this commit which would only have the "attribute" form of these items and misread them.

The above sub-elements ARE however something that can be removed from the XMLexport code in the future if a separate pending Pull-Request to restore version string checking to the code that parses the "MudletPackage" element as then we can explain and justify the quick and easy one-off fix for an older version of the XML format (to adjust the settings in the 2D Mapper and then save the map in the new format - which may be set to happen by default).
Removed misspelled "commandSeperator" sub-element of the "Host" element and the associated (QString) Host::mCommandSeperator which was unused and duplicated the (QString) Host::mCommandSeperator member...!
Removed a couple of empty string literals from:
- (bool) XMLimportPackage(QIODevice *, QString, int)
  this raised an interesting point in that where one of those was used it was necessary (and valid) to add the const qualifier to:
  TAlias::setScript( QString & )
  so that it became:
  TAlias::setScript( const QString & ) - this is different from the development branch code because that passes an actual QString as the argument (which should be harmless because QStrings are Copy-on-Write and no changes seem to be made to the value being passed). In this branch of code a reference is used instead and the default constructed null QString can only be used to initialise the variable concerned if it is kept constant, or so it seems...!
Removed redundant:
- (QString) XMLexport::mType
- XMLexport::XMLexport(Host *, bool)
Renamed:
(void) XMLimport::readMapList(QMap<QString,QStringList> &)
==> (void) XMLimport::readModulesDetailsMap(QMap<QString,QStringList> &)
to emphasis it is related to populating a QMap data container rather than
doing something with a Mudlet Map!
Added qDebug() code for further methods in XMLimport class when unknown elements are encountered - for some reason they were not in all the similar ones for different types of Mudlet items (Triggers, Aliases, etc.)
Added qDebug() code for further methods in XMLimport class when the script part of different types of Mudlet items (Triggers, Aliases, etc.) are read and inserted into the application data so that if they fail to compile it is reported - for some reason they were not in all the similar place for different types of Mudlet items.
Added an argument to the XMLimport::readIntegerList(...) to pass the name of the parent trigger for use if/when the error condition of having an invalid (not a number) as an element that encodes the type of the trigger condition for a particular condition. This is justified because under those conditions qFatal() is used {probably because it is the only QDebug class method that WILL be detectable on a WINDOWS platform for a RELEASE build} unfortunately it will KILL the Mudlet application - so it behoves Mudlet to report what killed it when it does die through this!
Rename an argument in:
- (void) XMLimport::readHostPackage(Host *)
- (bool) XMLexport::writeGenericPackage(Host *)
- (bool) XMLexport::writeHost(Host *)

from pT to pHost as the latter name is more prevalent elsewhere in the application.

Signed-off-by: Stephen Lyons slysven@virginmedia.com

…issues This contains the BugFix and other elements of a Pull Request made onto the development branch but without the code cleanup/layout changes. * Fix the parsing of the "mRoomSize" and "mLineSize" ATTRIBUTES to the "Host" element of Mudlet XML files. The reader code was treating the stored strings as integers when they were floating point decimal numbers, whilst this is not an issue for Host::mLineSize which currently DOES operate as a whole number without a decimal part it BREAKS "mRoomSize" which ranges from 0.1 to 1.1 in 0.1 steps. An attempt was made to fix the fault on 2011-06-09 in commit: Mudlet@7b4da59 but that added two sub-elements to the Host element to store the same data rather than correcting the original fault. This commit cannot remove those two sub-elements because that would currently break saved XML files if they were then used in Mudlet versions without this commit which would only have the "attribute" form of these items and mis-read them. The above sub-elements ARE however something that can be removed from the XMLexport code in the future if a separate pending Pull-Request to restore version string checking to the code that parses the "MudletPackage" element as then we can explain and justify the quick and easy one-off fix for an older version of the XML format (to adjust the settings in the 2D Mapper and then save the map in the new format - which may be set to happen by default). * Removed mis-spelled "commandSeperator" sub-element of the "Host" element and the associated (QString) Host::mCommandSeperator which was unused and duplicated the (QString) Host::mCommandSeperator member...! * Removed a couple of empty string literals from: (bool) XMLimportPackage(QIODevice *, QString, int) this raised an interesting point in that where one of those was used it was necessary (and valid) to add the const qualifier to: TAlias::setScript( QString & ) so that it became: TAlias::setScript( const QString & ) - this is different from the development branch code because that passes an actual QString as the argument (which should be harmless because QStrings are Copy-on-Write and no changes seem to be made to the value being passed). In this branch of code a reference is used instead and the default constructed null QString can only be used to initialise the variable concerned if it is kept constant, or so it seems...! * Removed redundant: (QString) XMLexport::mType XMLexport::XMLexport(Host *, bool) * Renamed: (void) XMLimport::readMapList(QMap<QString,QStringList> &) ==> (void) XMLimport::readModulesDetailsMap(QMap<QString,QStringList> &) to emphasis it is related to populating a QMap data container rather than doing something with a Mudlet Map! * Added qDebug() code for further methods in XMLimport class when unknown elements are encountered - for some reason they were not in all the similar ones for different types of Mudlet items (Triggers, Aliases, etc.) * Added qDebug() code for further methods in XMLimport class when the script part of different types of Mudlet items (Triggers, Aliases, etc.) are read and inserted into the application data so that if they fail to compile it is reported - for some reason they were not in all the similar place for different types of Mudlet items. * Added an argument to the XMLimport::readIntegerList(...) to pass the name of the parent trigger for use if/when the error condition of having an invalid (not a number) as an element that encodes the type of the trigger condition for a particular condition. This is justified because under those conditions qFatal() is used {probably because it is the only QDebug class method that WILL be detectable on a WINDOWS platform for a RELEASE build} unfortunately it will KILL the Mudlet application - so it behoves Mudlet to report what killed it when it does die through this! * Rename an argument in: * (void) XMLimport::readHostPackage(Host *) * (bool) XMLexport::writeGenericPackage(Host *) * (bool) XMLexport::writeHost(Host *) from pT to pHost as the latter name is more prevalent elsewhere in the application. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

When doing a search and replace for "pT" in the XMLexport.cpp file I hit a few cases where that text was a part of a longer variable name and broke them! Whilst fixing that I spotted that a boolean flag in several methods in that class that was intended to be cleared on error was capable of being returned to true afterwards (although practically it was perhaps unlikely to happen) - for predictable behaviour the code has been amended to never allow the flag to be returned to the set state. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven · 2017-01-28T04:24:21Z

src/XMLexport.cpp

    setAutoFormatting(true);
 }

-XMLexport::XMLexport( Host * pT, bool b )


This method is redundant.

SlySven · 2017-01-28T04:25:09Z

src/Host.cpp

 , mCodeCompletion( true )
 , mCommandLineFont   ( QFont("Bitstream Vera Sans Mono", 10, QFont::Normal ) )//( QFont("Monospace", 10, QFont::Courier) )
 , mCommandSeparator  ( QString(";") )
-, mCommandSeperator  ( QString(";") )


This misspelt member is not used (anymore).

SlySven · 2017-01-28T04:25:55Z

src/TAlias.h

    void             execute();
    QString          getScript()                     { return mScript; }
-    bool             setScript( QString & script );
+    bool             setScript( const QString & script );


Needs to be const so we can pass it by reference!

SlySven · 2017-01-28T04:27:50Z

src/XMLexport.cpp

 }

-bool XMLexport::writeHost( Host * pT )
+bool XMLexport::writeHost( Host * pHost )


pT tends to be used for a TTrigger or other Mudlet item "type", pHost makes things a bit clearer what it points to IMHO.

SlySven · 2017-01-28T04:32:23Z

src/XMLexport.cpp

    writeTextElement( "colorTriggerFgColor", pT->mColorTriggerFgColor.name() );
    writeTextElement( "colorTriggerBgColor", pT->mColorTriggerBgColor.name() );

+// TODO: The next bit could be revised for a new - not BACKWARD COMPATIBLE form


This will have to wait until we advance to Mudlet XML format version 1.001 or greater - whenever we feel we can afford to break previous Mudlet versions but then we will want to (and can) provide a downgrade capability in the same manner as we have coded for in the Map file format now!

SlySven · 2017-01-28T04:34:41Z

src/XMLimport.cpp

-        mpAlias->setScript(_nothing);
-        mpAlias->setRegexCode("");
+        mpAlias->setScript( QString() );
+        mpAlias->setRegexCode( QString() );


Cleaner code that won't trip things when we start looking for string literals as part of I18n work.

SlySven · 2017-01-28T04:36:06Z

src/XMLimport.cpp

-    pT->mAcceptServerGUI = ( attributes().value("mAcceptServerGUI") == "yes" );
-    pT->mMapperUseAntiAlias = ( attributes().value("mMapperUseAntiAlias") == "yes" );
-    pT->mFORCE_MXP_NEGOTIATION_OFF = ( attributes().value("mFORCE_MXP_NEGOTIATION_OFF") == "yes" );
-    pT->mRoomSize = attributes().value("mRoomSize").toString().toInt();


This is the point of failure all those years ago as mRoomsize is between 0.1 and 1.1 !

SlySven · 2017-01-28T04:51:53Z

src/XMLimport.cpp

+    pHost->mFORCE_MXP_NEGOTIATION_OFF = ( attributes().value("mFORCE_MXP_NEGOTIATION_OFF") == "yes" );
+    pHost->mRoomSize = attributes().value("mRoomSize").toString().toFloat();
+    if( qFuzzyCompare( 1.0 + pHost->mRoomSize, 1.0 ) ) {
+        // The value is a float/double and the prior code using "== 0" is a BAD


if( ! pHost->mRoomSize ) being of course equivalent to if( pHost->mRoomSize != 0.0 ) where pHost->mRoomSize has been up converted from its float form to a double - it possibly might come out as a representation of zero but it is not wise to assume it. qFuzzyCompare is a Qt provided floating point comparison (returns true if the numbers have the same internal representation) but since the window used for comparison is based on the magnitude of the two numbers they should not BOTH be zero - also both arguments must both be the same type - either float or double, in this case they are doubles.

SlySven · 2017-01-28T04:54:22Z

src/XMLimport.cpp

+    if( qFuzzyCompare( 1.0 + pHost->mRoomSize, 1.0 ) ) {
+        // The value is a float/double and the prior code using "== 0" is a BAD
+        // THING to do with non-integer number types!
+        pHost->mRoomSize= 0.5f; // Same value as is in Host class initalizer list


The prior code to recover from the absence of a value in the file was 3 - but given that the user can set a value of only 0.1 to 1.1 and that 1.0 means that a room on the 2D map is precisely the same size as the unit distance between rooms mean 3 was a really poor choice!

@vadi2 You have a change pending elsewhere that will impact on this IIRC...

I only changed the default value and this isn't about changing the defaults afaik?

It does, because the failure to read a valid value from the file here now tries to adopt the default values, which is what you have/will be tweaking.

SlySven · 2017-01-28T04:56:17Z

src/XMLimport.cpp

            {
                QMap<QString, QStringList> entry;
-                readMapList(entry);
+                readModulesDetailsMap(entry);


Yeah, we are reading a list of Modules into a QMap - it is nothing to do with a "Map" being a representation of the relative location of things to one another. 🤷‍♂️

SlySven · 2017-01-28T05:01:29Z

src/XMLimport.cpp

-                pT->mScript = readElementText();
-                continue;
+            else if( name() == "script") {
+                tempScript = readElementText();


The previous code set the data read here directly into the Trigger's script area - bypassing the compilation step that would report an error if it didn't compile and then used the proper setter TTrigger::setScript(...) at the end of parsing the trigger to do that bit. Seems a bit pointless when we can save the data locally and then use the same setScript command to avoid touching the TTrigger::script member ourselves.

SlySven · 2017-01-28T05:10:11Z

src/XMLimport.cpp

            {
-                QString script = readElementText();
-                pT->setScript( script );
+                tempScript = readElementText();


Having revised the code for the TTrigger case it seems sensible to repeat the pattern for all the other Mudlet item types (Action/Alias/Ket/Script/Timer) in the same manner.

SlySven · 2017-01-28T05:29:54Z

src/XMLexport.cpp

+//        writeEmptyElement( "RegexCode" );
+//        writeAttribute( "index", QString::number( i ) );
+//        writeAttribute( "type", QString::number( pT->mRegexCodePropertyList.at(i) ) );
+//        writeAttribute( "data", pT->mRegexCodeList.at(i) );


This proposal would include both the content and the type in a new form:

<RegexList size="n"> <RegexCode index="1" type="<regexp type number (0-6)>" data="what ever the string is for the 1st regexp" /> <RegexCode index="2" type="<regexp type number>" data="what ever the string is for the 2nd regexp" /> .... <RegexCode index="n" type="<regexp type number>" data="what ever the string is for the nth regexp" /> </RegexList>

In hindsight the following might be considered more proper:

<RegexList size="n"> <Regex index="1" type="<regexp type number (0-6)>" >what ever the string is for the 1st regexp</RegexCode> <Regex index="2" type="<regexp type number>" >what ever the string is for the 2nd regexp</RegexCode> .... <Regex index="n" type="<regexp type number>" >what ever the string is for the nth regexp</RegexCode> </RegexList>

This would eliminate the need for XMLimport::readIntegerList( QList<int> & list ) all together and remove the need to have a qFatal() to handle the (vanishingly small chance) if something other than a number was read there - making things better all around I think!

vadi2

Looks good, just run this though clang-format so I don't have to review a thousand lines again...

Following inter-developer discussion it was decided to apply the same clang-format code formatting to the XMLimport class in this branch as for the development branch - as this will increase the complexity of the changes I felt there was little need to limit the scope of changes in this branch to the minimum to provide the functional changes so have increased the change-set to include some redundant code elimination steps already done in the other branch commits to reduce the overall differences between the changed files in the two branches. I have also added the .clang-format file to the QMake project file so that it shows up in the Qt Creator IDE - however the file is in the base directory of the project in this branch's code whereas it is in the ./src/ sub-directory in the development branch at some point one branch should be revised to use the same location for the file as the other one. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2 · 2017-02-27T04:00:05Z

Big thanks for getting this improved!

In commit-18750755c8a8acbb35a5dd65478333923da6a8e1 (Pull Request Mudlet#373) a change was made that reordered the compilation of the Lua scripts for Mudlet items. This commit reverts that aspect to restore the compilation of parent (e.g. folder/container) items before their children. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

In commit-18750755c8a8acbb35a5dd65478333923da6a8e1 (Pull Request #373) a change was made that reordered the compilation of the Lua scripts for Mudlet items. This commit reverts that aspect to restore the compilation of parent (e.g. folder/container) items before their children. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven added 2 commits January 21, 2017 18:25

SlySven mentioned this pull request Jan 21, 2017

(release_30)enhance: restore version check to Mudlet XMLimport code #370

Closed

SlySven commented Jan 28, 2017

View reviewed changes

vadi2 requested changes Feb 26, 2017

View reviewed changes

vadi2 approved these changes Feb 27, 2017

View reviewed changes

vadi2 merged commit 1875075 into Mudlet:release_30 Feb 27, 2017

SlySven deleted the (release_30)bugFix_miscellanousXmlParsingDetails branch March 3, 2017 19:57

SlySven mentioned this pull request Mar 10, 2017

(release_30)partialRevert: restore compilation order on profile loading #416

Merged

SlySven restored the (release_30)bugFix_miscellanousXmlParsingDetails branch June 22, 2020 18:06

SlySven deleted the (release_30)bugFix_miscellanousXmlParsingDetails branch June 22, 2020 18:13

Uh oh!

Conversation

SlySven commented Jan 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven Jan 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven Jan 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven Jan 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

vadi2 commented Feb 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SlySven commented Jan 21, 2017 •

edited

Loading

SlySven Jan 28, 2017 •

edited

Loading

SlySven Jan 28, 2017 •

edited

Loading

SlySven Jan 28, 2017 •

edited

Loading