Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Dec 9, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
change SKIN_WARNING to show the skin file:line first, then c++ context
  • Loading branch information
ronso0 committed Nov 6, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit b57201ef1215ffd43bb5b93ac2c2ece2cc01c02b
159 changes: 101 additions & 58 deletions src/skin/legacy/legacyskinparser.cpp

Large diffs are not rendered by default.

21 changes: 13 additions & 8 deletions src/skin/legacy/skincontext.cpp
Original file line number Diff line number Diff line change
@@ -135,14 +135,19 @@ PixmapSource SkinContext::getPixmapSourceInner(const QString& filename) const {
return PixmapSource();
}

QDebug SkinContext::logWarning(const char* file, const int line,
const QDomNode& node) const {
return qWarning() << QString("%1:%2 SKIN ERROR at %3:%4 <%5>:")
.arg(file, QString::number(line), m_xmlPath,
QString::number(node.lineNumber()),
node.nodeName())
.toUtf8()
.constData();
QDebug SkinContext::logWarning(const char* file,
const int line,
const QDomNode& node,
const QString& message) const {
return qWarning() << QString("SKIN ERROR at %1:%2 <%3>: %4 | %5:%6")
.arg(m_xmlPath,
QString::number(node.lineNumber()),
node.nodeName(),
message,
file,
QString::number(line))
.toUtf8()
.constData();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
};
```

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your plans with this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

int SkinContext::scaleToWidgetSize(QString& size) const {
8 changes: 6 additions & 2 deletions src/skin/legacy/skincontext.h
Original file line number Diff line number Diff line change
@@ -15,7 +15,8 @@
#include "widget/wpixmapstore.h"
#include "widget/wsingletoncontainer.h"

#define SKIN_WARNING(node, context) (context).logWarning(__FILE__, __LINE__, (node))
#define SKIN_WARNING(node, context, message) \
(context).logWarning(__FILE__, __LINE__, (node), (message))

// A class for managing the current context/environment when processing a
// skin. Used hierarchically by LegacySkinParser to create new contexts and
@@ -225,7 +226,10 @@ class SkinContext {
return defaultDrawMode;
}

QDebug logWarning(const char* file, const int line, const QDomNode& node) const;
QDebug logWarning(const char* file,
const int line,
const QDomNode& node,
const QString& message) const;

void defineSingleton(const QString& objectName, QWidget* widget) {
m_pSharedState->singletons.insertSingleton(objectName, widget);
6 changes: 4 additions & 2 deletions src/widget/wcombobox.cpp
Original file line number Diff line number Diff line change
@@ -25,8 +25,10 @@ void WComboBox::setup(const QDomNode& node, const SkinContext& context) {
QString icon = context.selectString(state, "Icon");
addItem(QIcon(icon), text, QVariant(iState));
} else {
SKIN_WARNING(state, context)
<< "WComboBox ignoring <State> without <Number> node.";
SKIN_WARNING(state,
context,
QStringLiteral("WComboBox ignoring <State> without "
"<Number> node."));
}
}
state = state.nextSibling();
6 changes: 4 additions & 2 deletions src/widget/weffectbuttonparametername.cpp
Original file line number Diff line number Diff line change
@@ -22,8 +22,10 @@ void WEffectButtonParameterName::setup(const QDomNode& node, const SkinContext&
EffectWidgetUtils::getButtonParameterSlotFromNode(
node, context, m_pEffectSlot);
VERIFY_OR_DEBUG_ASSERT(m_pParameterSlot) {
SKIN_WARNING(node, context)
<< "EffectButtonParameter node could not attach to effect parameter";
SKIN_WARNING(node,
context,
QStringLiteral("EffectButtonParameter node could not attach to "
"effect parameter"));
}
setEffectParameterSlot(m_pParameterSlot);
}
6 changes: 4 additions & 2 deletions src/widget/weffectchain.cpp
Original file line number Diff line number Diff line change
@@ -19,8 +19,10 @@ void WEffectChain::setup(const QDomNode& node, const SkinContext& context) {
if (pChainSlot) {
setEffectChain(pChainSlot);
} else {
SKIN_WARNING(node, context)
<< "EffectChain node could not attach to effect chain slot.";
SKIN_WARNING(node,
context,
QStringLiteral("EffectChain node could not attach to effect "
"chain slot."));
}
}

6 changes: 4 additions & 2 deletions src/widget/weffectchainpresetbutton.cpp
Original file line number Diff line number Diff line change
@@ -26,8 +26,10 @@ void WEffectChainPresetButton::setup(const QDomNode& node, const SkinContext& co
m_pChain = EffectWidgetUtils::getEffectChainFromNode(
node, context, m_pEffectsManager);
VERIFY_OR_DEBUG_ASSERT(m_pChain) {
SKIN_WARNING(node, context)
<< "EffectChainPresetButton node could not attach to effect chain";
SKIN_WARNING(node,
context,
QStringLiteral("EffectChainPresetButton node could not attach "
"to effect chain"));
return;
}
connect(m_pChain.get(),
6 changes: 4 additions & 2 deletions src/widget/weffectchainpresetselector.cpp
Original file line number Diff line number Diff line change
@@ -31,8 +31,10 @@ void WEffectChainPresetSelector::setup(const QDomNode& node, const SkinContext&
node, context, m_pEffectsManager);

VERIFY_OR_DEBUG_ASSERT(m_pChain != nullptr) {
SKIN_WARNING(node, context)
<< "EffectChainPresetSelector node could not attach to EffectChain";
SKIN_WARNING(node,
context,
QStringLiteral("EffectChainPresetSelector node could not "
"attach to EffectChain"));
return;
}

6 changes: 4 additions & 2 deletions src/widget/weffectknobparametername.cpp
Original file line number Diff line number Diff line change
@@ -22,8 +22,10 @@ void WEffectKnobParameterName::setup(const QDomNode& node, const SkinContext& co
EffectWidgetUtils::getParameterSlotFromNode(
node, context, m_pEffectSlot);
VERIFY_OR_DEBUG_ASSERT(m_pParameterSlot) {
SKIN_WARNING(node, context)
<< "EffectParameter node could not attach to effect parameter";
SKIN_WARNING(node,
context,
QStringLiteral("EffectParameter node could not attach to "
"effect parameter"));
}
setEffectParameterSlot(m_pParameterSlot);
}
6 changes: 4 additions & 2 deletions src/widget/weffectname.cpp
Original file line number Diff line number Diff line change
@@ -22,8 +22,10 @@ void WEffectName::setup(const QDomNode& node, const SkinContext& context) {
if (pEffectSlot) {
setEffectSlot(pEffectSlot);
} else {
SKIN_WARNING(node, context)
<< "EffectName node could not attach to effect slot.";
SKIN_WARNING(node,
context,
QStringLiteral(
"EffectName node could not attach to effect slot."));
}
}

2 changes: 1 addition & 1 deletion src/widget/weffectparameterknob.cpp
Original file line number Diff line number Diff line change
@@ -12,7 +12,7 @@ void WEffectParameterKnob::setup(const QDomNode& node, const SkinContext& contex
m_pEffectParameterSlot = EffectWidgetUtils::getParameterSlotFromNode(
node, context, pEffectSlot);
VERIFY_OR_DEBUG_ASSERT(m_pEffectParameterSlot) {
SKIN_WARNING(node, context) << "Could not find effect parameter slot";
SKIN_WARNING(node, context, QStringLiteral("Could not find effect parameter slot"));
return;
}
connect(m_pEffectParameterSlot.data(),
2 changes: 1 addition & 1 deletion src/widget/weffectparameterknobcomposed.cpp
Original file line number Diff line number Diff line change
@@ -12,7 +12,7 @@ void WEffectParameterKnobComposed::setup(const QDomNode& node, const SkinContext
m_pEffectParameterSlot = EffectWidgetUtils::getParameterSlotFromNode(
node, context, pEffectSlot);
VERIFY_OR_DEBUG_ASSERT(m_pEffectParameterSlot) {
SKIN_WARNING(node, context) << "Could not find effect parameter slot";
SKIN_WARNING(node, context, QStringLiteral("Could not find effect parameter slot"));
return;
}
connect(m_pEffectParameterSlot.data(),
2 changes: 1 addition & 1 deletion src/widget/weffectpushbutton.cpp
Original file line number Diff line number Diff line change
@@ -23,7 +23,7 @@ void WEffectPushButton::setup(const QDomNode& node, const SkinContext& context)
m_pEffectParameterSlot = EffectWidgetUtils::getButtonParameterSlotFromNode(
node, context, pEffectSlot);
if (!m_pEffectParameterSlot) {
SKIN_WARNING(node, context) << "Could not find effect parameter slot";
SKIN_WARNING(node, context, QStringLiteral("Could not find effect parameter slot"));
DEBUG_ASSERT(false);
}
connect(m_pEffectParameterSlot.data(),
6 changes: 4 additions & 2 deletions src/widget/weffectselector.cpp
Original file line number Diff line number Diff line change
@@ -42,8 +42,10 @@ void WEffectSelector::setup(const QDomNode& node, const SkinContext& context) {
this,
&WEffectSelector::slotEffectSelected);
} else {
SKIN_WARNING(node, context)
<< "EffectSelector node could not attach to effect slot.";
SKIN_WARNING(node,
context,
QStringLiteral("EffectSelector node could not attach to effect "
"slot."));
}

populate();
7 changes: 5 additions & 2 deletions src/widget/whotcuebutton.cpp
Original file line number Diff line number Diff line change
@@ -33,7 +33,10 @@ void WHotcueButton::setup(const QDomNode& node, const SkinContext& context) {
if (ok && hotcue > 0) {
m_hotcue = hotcue - 1;
} else {
SKIN_WARNING(node, context) << "Hotcue value invalid";
SKIN_WARNING(node,
context,
QStringLiteral("Hotcue index '%1' invalid")
.arg(context.selectString(node, QStringLiteral("Hotcue"))));
}

bool okay;
@@ -84,7 +87,7 @@ void WHotcueButton::setup(const QDomNode& node, const SkinContext& context) {

QDomNode con = context.selectNode(node, QStringLiteral("Connection"));
if (!con.isNull()) {
SKIN_WARNING(node, context) << "Additional Connections are not allowed";
SKIN_WARNING(node, context, QStringLiteral("Additional Connections are not allowed"));
}
}

10 changes: 7 additions & 3 deletions src/widget/wpushbutton.cpp
Original file line number Diff line number Diff line change
@@ -195,9 +195,13 @@ void WPushButton::setup(const QDomNode& node, const SkinContext& context) {
if (m_rightButtonMode != ControlPushButton::PUSH &&
m_rightButtonMode != ControlPushButton::TOGGLE &&
m_rightButtonMode != ControlPushButton::TRIGGER) {
SKIN_WARNING(node, context)
<< "WPushButton::setup: Connecting a Pushbutton not in PUSH, TRIGGER or TOGGLE mode is not implemented\n"
<< "Please consider to set <RightClickIsPushButton>true</RightClickIsPushButton>";
SKIN_WARNING(node,
context,
"WPushButton::setup: Connecting a Pushbutton not "
"in PUSH, TRIGGER or TOGGLE mode is not "
"implemented\n Please consider to set "
"<RightClickIsPushButton>true</"
"RightClickIsPushButton>");
}
}
}
12 changes: 6 additions & 6 deletions src/widget/wsingletoncontainer.cpp
Original file line number Diff line number Diff line change
@@ -20,19 +20,19 @@ void WSingletonContainer::setup(const QDomNode& node, const SkinContext& context
QDomElement containerNode = node.toElement();
QString objectName;
if (!context.hasNodeSelectString(node, "ObjectName", &objectName)) {
SKIN_WARNING(node, context)
<< "Need objectName attribute for Singleton tag";
SKIN_WARNING(node, context, QStringLiteral("Need ObjectName attribute for Singleton tag"));
return;
}
if (objectName.isEmpty()) {
SKIN_WARNING(node, context)
<< "Singleton tag's ObjectName is empty";
SKIN_WARNING(node, context, QStringLiteral("Singleton tag's ObjectName is empty"));
return;
}
m_pWidget = context.getSingletonWidget(objectName);
if (m_pWidget == nullptr) {
SKIN_WARNING(node, context)
<< "Asked for an unknown singleton widget:" << objectName;
SKIN_WARNING(node,
context,
QStringLiteral("Asked for an unknown singleton widget: %1")
.arg(objectName));
}
}

32 changes: 14 additions & 18 deletions src/widget/wsplitter.cpp
Original file line number Diff line number Diff line change
@@ -35,22 +35,19 @@ void WSplitter::setup(const QDomNode& node, const SkinContext& context) {

if (m_pConfig->exists(m_configKey)) {
sizesJoined = m_pConfig->getValueString(m_configKey);
msg = "Reading .cfg file: '"
+ m_configKey.group + " "
+ m_configKey.item + " "
+ sizesJoined
+ "' does not match the number of children nodes:"
+ QString::number(this->count());
msg = "Reading .cfg file: '" + m_configKey.group + " " +
m_configKey.item + " " + sizesJoined +
"' does not match the number of children nodes:" +
QString::number(count());
ok = true;
}
}

// nothing in mixxx.cfg? Load default values
if (!ok && context.hasNodeSelectString(node, "SplitSizes", &sizesJoined)) {
msg = "<SplitSizes> for <Splitter> ("
+ sizesJoined
+ ") does not match the number of children nodes:"
+ QString::number(this->count());
msg = "<SplitSizes> for <Splitter> (" + sizesJoined +
") does not match the number of children nodes:" +
QString::number(count());
}

// found some value for splitsizes?
@@ -64,8 +61,8 @@ void WSplitter::setup(const QDomNode& node, const SkinContext& context) {
break;
}
}
if (sizesList.length() != this->count()) {
SKIN_WARNING(node, context) << msg;
if (sizesList.length() != count()) {
SKIN_WARNING(node, context, msg);
ok = false;
}
if (ok) {
@@ -85,12 +82,11 @@ void WSplitter::setup(const QDomNode& node, const SkinContext& context) {
break;
}
}
if (collapsibleList.length() != this->count()) {
msg = "<Collapsible> for <Splitter> ("
+ collapsibleJoined
+ ") does not match the number of children nodes:"
+ QString::number(this->count());
SKIN_WARNING(node, context) << msg;
if (collapsibleList.length() != count()) {
msg = "<Collapsible> for <Splitter> (" + collapsibleJoined +
") does not match the number of children nodes:" +
QString::number(count());
SKIN_WARNING(node, context, msg);
ok = false;
}
if (ok) {
6 changes: 4 additions & 2 deletions src/widget/wvumeterbase.cpp
Original file line number Diff line number Diff line change
@@ -80,8 +80,10 @@ void WVuMeterBase::setup(const QDomNode& node, const SkinContext& context) {
if (height() < 2 || width() < 2) {
// This triggers a QT bug and displays a white widget instead.
// We warn here, because the skin designer may not use the affected mode.
SKIN_WARNING(node, context)
<< "VuMeterBase needs to have 2 pixel in all extents to be visible on all targets.";
SKIN_WARNING(node,
context,
QStringLiteral("VuMeterBase needs to have 2 pixel in all "
"extents to be visible on all targets."));
}

setFocusPolicy(Qt::NoFocus);