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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b57201e
change SKIN_WARNING to show the skin file:line first, then c++ context
ronso0 Nov 4, 2023
72b6f1c
debug logging: rename SKIN ERROR to 'Skin parsing failed at'
ronso0 Nov 5, 2023
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
159 changes: 101 additions & 58 deletions src/skin/legacy/legacyskinparser.cpp
View file
Open in desktop

Large diffs are not rendered by default.

21 changes: 13 additions & 8 deletions src/skin/legacy/skincontext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 parsing failed 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 {
Expand Down
8 changes: 6 additions & 2 deletions src/skin/legacy/skincontext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down
6 changes: 4 additions & 2 deletions src/widget/wcombobox.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
6 changes: 4 additions & 2 deletions src/widget/weffectbuttonparametername.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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."));
}
}

Expand Down
6 changes: 4 additions & 2 deletions src/widget/weffectchainpresetbutton.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
6 changes: 4 additions & 2 deletions src/widget/weffectchainpresetselector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
6 changes: 4 additions & 2 deletions src/widget/weffectknobparametername.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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."));
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/widget/weffectparameterknob.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
2 changes: 1 addition & 1 deletion src/widget/weffectparameterknobcomposed.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
2 changes: 1 addition & 1 deletion src/widget/weffectpushbutton.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
6 changes: 4 additions & 2 deletions src/widget/weffectselector.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
7 changes: 5 additions & 2 deletions src/widget/whotcuebutton.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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"));
}
}

Expand Down
10 changes: 7 additions & 3 deletions src/widget/wpushbutton.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>");
}
}
}
Expand Down
12 changes: 6 additions & 6 deletions src/widget/wsingletoncontainer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}

Expand Down
32 changes: 14 additions & 18 deletions src/widget/wsplitter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down
6 changes: 4 additions & 2 deletions src/widget/wvumeterbase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Loading