change SKIN_WARNING to show the skin file:line first, then c++ context

[ronso0]

The failing skin xml line can now be spotted much easier, especially if your source directory path is rather long.

before:

warning [Main] /long/path/to/mixxx-source-directory/src/widget/whotcuebutton.cpp:36 SKIN ERROR at skin:../LateNight/controls/button_hotcue.xml:10 <HotcueButton>: Hotcue value invalid

now:

warning [Main] Skin parsing failed at skin:../LateNight/controls/button_hotcue.xml:10 <HotcueButton>: Hotcue value invalid at /long/path/to/mixxx-source-directory/src/widget/whotcuebutton.cpp:36

The xml path and the error message are meaningful already, so maybe we can omit /path/to/mixxx-source-file/:123 altogether. Just my impression, I didn't check all usages.

[JoergAtGithub]

I tested this an works as expected, apart that I get absolute path for the XML file (independent of the change in this PR).

warning [Main] LegacySkinParser::loadTemplate - setContent failed see "C:/Users/Joerg.WORLDWARTWEB/source/repos/JoergAtGithub/mixxx/res/skins/LateNight/mixer.xml" line: 27 column: 24
warning [Main] LegacySkinParser::loadTemplate - message: "error occurred while parsing element"
warning [Main] SKIN ERROR at C:/Users/Joerg.WORLDWARTWEB/source/repos/JoergAtGithub/mixxx/res/skins/LateNight/skin.xml:453 <Template>: Template instantiation for template failed: skin:../LateNight/mixer.xml | C:\Users\Joerg.WORLDWARTWEB\source\repos\JoergAtGithub\mixxx\src\skin\legacy\legacyskinparser.cpp:1760

Why do we print this as warning and use the words SKIN ERROR? This does not match to me.

[ronso0]

Well, it's not fatal for the app, it's just SKIN PARSING ERROR to be exact.
Any idea how to improve it? Shall we rename it?

[JoergAtGithub]

Maybe warning [Main] Skin parsing failed at:

[ronso0]

Makes sense. I'll check if SKIN_WARNING is indeed only used for parsing errors.

[ronso0]

It is. I changed it to Skin parsing failed at
And amended some minor fixes to the previous commit.

[daschuer]

Maybe warning [Main] Skin parsing failed at:

Or even shorter:

warning [Main] parsing:

[daschuer]

Here is the %1 missing.
I am afraid the arg() version is more expensive than the original.
I have an idea to keep the stream type working. See my other comment.

[ronso0]

While I understand the argument to aim for best possible performance and appreciate the effort you put into the skinWarning class example below, I do wonder if this fully applies here. It's a warning after all indicating the GUI won't be functional as expected, and if a significant amount of warnings is printed this small performance regression is the least we should be worried about IMHO.

[daschuer]

This does not apply here, sure. The mandatory issue is only the missing %1

[daschuer]

To allow keep using the stream operators you may inherit from QDebug()
(Not sure if it is worth the effort)

Not yet working idea:

class SkinWarning : public QDebug {
  public:
    SkinWarning(const char* file,
        const int line,
        const QDomNode& node,
        const QString& message) 
            : QDebug(QtWarningMsg) {
        *this << "Skin parsing failed at" << QString(m_xmlPath,) + QChar(':') + QString::number(node.lineNumber());
    }

    ~SkinWarning() {
        *this << QString(m_file) + QChar(':') + QString::number(m_line);
    }
  private:
      const char* m_file,
      const int m_line,
};
```

[ronso0]

Interesting! Didn't think about creating a dedicated class.

[daschuer]

What are your plans with this PR?

[ronso0]

I'll try your suggestions, and if it gives my too much headaches I'd rather merge this as is.

[ronso0]

aaah, oooh, FILE and LINE are pre-processor macros. A little less magic now.

Anyway, I gave it a shot but it doesn't work as expected, or I just didn't fully understand how you plan to implement. I assumed you aimed to leave all SKIN_WARNING() calls unchanged.
https://github.com/ronso0/mixxx/pull/new/skin-warning-class incl. one commit with hotcue 0 to trigger a warning.

[ronso0]

If you are motivated and manage to fix it, fine. I can re-apply the individual tweaks on top of your branch.
Otherwise let's merge this as is, maybe drop a comment so someone can try again later.

[daschuer]

Suggested change

	QStringLiteral("Error reading double value from skin attribute: %1")
	QStringLiteral("Failed reading double value from skin attribute: %1")

[ronso0]

done.

[JoergAtGithub]

LGTM! Thank you!