(release3_0_1)Enhance: restore version check to Mudlet XMLimport code by SlySven · Pull Request #870 · Mudlet/Mudlet

SlySven · 2017-04-11T18:43:06Z

I foresee the need to tweak the Xml format in the future, however though there once was some checks for this in the code in the past it was commented out. This commit is a revised version of a pull request (#370) originally aimed at the release_30 branch but which has changed so much it was easier to restart from a more up to date point.

This version checks the "version" attribute of the MudletPackage element of most types of XML files read (not Maps) and will allow through without comment a string value that works out to a float less than 2.000f (three decimal place resolution) but will cause XMLimport::importPackage(...) to return a FALSE value - {the return is largely ignored currently} - to not read the file further and to cause an "[ ALERT ]" message on the main console should the version value be 2.000 or greater.

This is to cover the CURRENT code against problems that might arise in the future if the XML format/structure gets changed too much which will otherwise go undetected by the Mudlet application as it is currently.

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

I foresee the need to tweak the Xml format in the future, however though there once was some checks for this in the code in the past it was commented out. This commit is a revised version of a pull request (Mudlet#370) originally aimed at the release_30 branch but which has changed so much it was easier to restart from a more up to date point. This version checked the "version" attribute of the MudletPackage element of most types of XML files read (not Maps) and will allow through without comment a string value that works out to a float less than 2.000f (three decimal place resolution) but will cause XMLimport::importPackage(...) to return a FALSE value - {the return is largely ignored currently) - to not read the file further and to cause an "[ ALERT ]" message on the main console should the version value be 2.000 or greater. This is to cover the CURRENT code against problems that might arise in the future if the XML format/structure gets changed too much which will otherwise go *undetected* by the Mudlet application as it is currently. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

vadi2

As agreed elsewhere, looks good, just minor wording improvements

vadi2 · 2017-04-11T18:51:52Z

src/XMLimport.cpp

+                    /*||(mVersionMajor==1&&mVersionMinor)*/) {
+                    // Minor check is not currently relevant, just abort on 2.000f or more
+
+                    QString moanMsg = tr("[ ALERT ] - Sorry, the file being read is marked as being version %1 which is\n"


So I think we agreed that we would only up the version number when we are sure we want to block older Mudlet from reading it because it will not be able to cope with handling it at all, not even able to ignore the features enabled in the XML file that it doesn't know about, right?

How about this message then:

"Sorry, this settings file:\n%s\ncomes from a newer version of Mudlet and can't be understood - please upgrade your Mudlet"

Yeah, but I retain the ability to make decimal changes to the number so that code can consider extra sub-elements/attributes that older versions will qDebug() message about/ignore respectively - if needed. Off the top of my head I do not have any thing in mind that I cannot do without just using a pair of elements with an attribute that older code will ignore the first and use the second but newer code can spot. E.g, storing both a relative file name and an absolute file name for the same file e.g. the sound file for a trigger:

<mSoundFile isRelative="true">../../sounds/tada.wav</mSoundFile> <mSoundFile isRelative="false">/home/stephen/.config/mudlet/sounds/tada.wav</mSoundFile>

Old code will read both entries but the second will override the first, new code will spot the relative attribute and know to use that one - which is portable between different OS platforms because it is relative to the profile's directory. This I can use without changing a version value.

OTOH I have plans to add (upto) 4 icons to buttons that do not disappear when disabled (due to a new attribute "isNotHiddenWhenDisabled") - I hope and think I can squeeze it into the existing layout in a way that degrades in a similar manner, but in the worse case scenario I might (I am only saying might here) need to flag the code to treat the contents in that element in a different way - thinking harder I can't come up with a concrete use case, but it will remain a tool available - should the need arise - to nudge that value up by 0.001...

"Sorry, this settings file:\n%s\ncomes from a newer version of Mudlet and can't be understood - please upgrade your Mudlet"

Humm, is "settings" file the right term - it can be any of:

Mudlet profile save file

a "package" containing one or more of "items" of one type

the Mudlet core/guts at the root of a "module".

I will need to check to see how to get the PathFileName to report as this method is working with a QIODevice* and I will have to check there is always a file on the other end of that - IIRC sometimes we might be use a buffer in memory.

I think we need to report the version found simply so when someone asks we can say "yes, your version of Mudlet is too old to read that type of file; you need version 5.1 or later and ideally you will find 6.0 which we released last month and that will really frob that file."

It turns out that XMLimport::importPackage(...) although having a QIODevice * as it's first argument is always used with a QFile * as that argument. Given that QFile inherits from QIODevice it seems reasonable to modify the first argument to be a QFile * so that it is possible to use a QFile::fileName() to get the name of the file without having to mess with qobject_cast<QFile *> or dynamic_cast<QFile *>.

vadi2 · 2017-04-11T18:53:53Z

src/XMLimport.h

    XMLimport(Host*);

-    bool importPackage(QIODevice* device, QString packageName = QString(), int moduleFlag = 0);
+    bool importPackage(QIODevice*, QString packageName = QString(), int moduleFlag = 0, QString* pVersionString = Q_NULLPTR);


Why not nullptr? We enforce C++11 and a lot more tools will understand nullptr than Qt's macro that's only useful if you are allowing < C++11.

Q_NULLPTR resolves to nullptr as far I could see and when I started using it I wasn't coding exclusively for C++11 (e.g. recent back-port to Qt4) - I was also using it as that seemed to be the Qt way. What tools do not understand it BTW?

I don't see it being recommended as the Qt way anywhere - it's more like a Qt workaround of anyone < C++11. Nevermind me though, I was wrong on the tools account and since Q_NULLPTR has made its way into the codebase might as well stick with it.

Well I was going to stay quiet but can I whisper the following as well: Q_DECL_OVERRIDE, possibly Q_DECL_FINAL and, for debug output I vaguely recall a discussion about identifying the function in which an error occurs, so Q_FUNC_INFO() might be useful. {I tend to manually enter the function and possibly argument types in my qDebug() lines but this may help to do it auto-magically}!

SlySven · 2017-04-11T18:56:55Z

If this gets the green light I'll get the same code into the development branch ASAP - it shouldn't be mangled by the auto iterator changes as it hits a different code base area...

The QtMath include is not available in Qt 5.0.2 so breaks compilation on that, pretty much the earliest version of the qt Libs we now support. Unfortunately this is the version that we are using for Linux platform Travis C.I. builds! Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven · 2017-04-11T20:55:05Z

Ah, forgot that this is release_30 - hit an error ENOCOFFEE - forgot to put in the conditional tweak to get the right Qt maths header in to use qFloor(...) - like I did on the previous incarnation so no wonder the Linux CI builds errorred out.

In XMLimport::importPackage(...) we would like to get access to the file name details of the file being read but this is not available from the base QIODevice type as it is a property of the QFile instances that are ALWAYS used as the first argument to the method. That being the case, to avoid having to use a qobject_cast<T*>(...) or dynamic_cast<T*>(...) to convert the original pointer to the desired type - with the need to handle unanticipated situations where those might have failed, it makes sense to explicitly use the QFile type so that we can use QFile::fileName() directly as needed. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven · 2017-04-11T22:05:48Z

Well that works, I clobbered the "version" to "2.000" in a local copy of a module I was using:

ahmedcharles · 2017-04-12T06:18:55Z

It would probably be simpler to not support floating point versions inside the file format and just assume that 1.0 is the same as 1 to deal with backwards compatibility.

SlySven · 2017-04-12T14:49:35Z

Well, the decimal part is the "minor" number, as we have adopted what I am informed is a symantec version numbering scheme for various aspects, and, although I have not been able to come up with a concrete case where it is definitely needed for our XML file format; I want to ensure we have some wriggle room as this commit aims to ensure that anything with a 2 major number part will ~~cleaning~~cleanly fail in current code going forward until a change which is most likely/intended to happen with a Mudlet application release version with a major version number increment. I do not believe that we need to go to the extent of supporting a third, patch number here so a decimal is the sensible (IMHO) option. To avoid miss-interpretation is why I am clearly specifying the form and how it is to be interpreted (3 decimal digits as the minor number, integer part as the major) so that errors are not made if/when things change...

SlySven · 2017-04-12T14:51:39Z

Also we have been using "1.0" as far back as I can trace so treating the string as a float number does not seem unreasonable. 😁

vadi2 · 2017-04-12T15:15:15Z

src/XMLimport.cpp

+                                         "\"%1\"\n"
+                                         "reports it has a version (%2) it must have come from a newer Mudlet version,\n"
+                                         "which this one cannot read it, you need to update! The creators of Mudlet\n"
+                                         "suggest that you seek help and an update on-line using the links in the panel\n"


Can we just say "Please upgrade your Mudlet."? Why do they need to seek help to upgrade an Mudlet? All we'll tell them is to upgrade Mudlet, not much else. If they are confused, they'll seek help anyway and don't need telling as such.

My thoughts were that if they are getting this message it means that there has been a significant change - because I would hope that we would have included migration steps in Mudlet versions with minor number increments and if the user was a keen/frequent user they would already have adopted a later version - therefore someone getting this is not likely to be as familiar with the product and advising them of how/where to go to fix the issue is more friendly than just saying "Sorry Dude, you shouldn't be doing that with this!"

OTOH perhaps I am over-thinking this, so: "delete that last sentence" + previous sentence replace "to update" with "a newer Mudlet!" yes / no?

On it... while you look at the other issue... 😀

Using:

..."reports it has a version (%2) it must have come from a later Mudlet version,\n" "and this one cannot read it, you need a newer Mudlet!")

to avoid the use of the word "newer" twice in one sentence - as that does not feel right in this case.

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

vadi2

💯

vadi2 · 2017-04-12T17:06:25Z

OK, awesome. Making 3.0.1.

SlySven mentioned this pull request Apr 11, 2017

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

Closed

SlySven requested a review from vadi2 April 11, 2017 18:53

SlySven self-assigned this Apr 11, 2017

SlySven added the 1 - Ready label Apr 11, 2017

vadi2 requested changes Apr 11, 2017

View reviewed changes

SlySven mentioned this pull request Apr 12, 2017

Enhance: restore version check to Mudlet XMLimport code #369

Closed

vadi2 requested changes Apr 12, 2017

View reviewed changes

vadi2 mentioned this pull request Apr 12, 2017

(release_30) Fixed GUI saved variables crashing with autologin #875

Merged

Revise: shorten and revise ALERT text

be22c38

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

vadi2 approved these changes Apr 12, 2017

View reviewed changes

SlySven merged commit 0f1b16d into Mudlet:release_30 Apr 12, 2017

SlySven deleted the (release_3_0_1)enhance_restoreVersionCheckingForXmlFiles branch April 12, 2017 17:06

SlySven restored the (release_3_0_1)enhance_restoreVersionCheckingForXmlFiles branch June 22, 2020 17:58

SlySven deleted the (release_3_0_1)enhance_restoreVersionCheckingForXmlFiles branch June 22, 2020 18:49

Uh oh!

Conversation

SlySven commented Apr 11, 2017

Uh oh!

vadi2 left a comment

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 Apr 11, 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!

SlySven commented Apr 11, 2017

Uh oh!

SlySven commented Apr 11, 2017

Uh oh!

SlySven commented Apr 11, 2017

Uh oh!

ahmedcharles commented Apr 12, 2017

Uh oh!

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

Uh oh!

Uh oh!

SlySven commented Apr 12, 2017

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!

vadi2 left a comment

Choose a reason for hiding this comment

Uh oh!

vadi2 commented Apr 12, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SlySven Apr 11, 2017 •

edited

Loading

SlySven commented Apr 12, 2017 •

edited

Loading