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

Made Paintable::DrawMode an enum class #13424

Merged
merged 5 commits into from
Jul 2, 2024
Merged

Made Paintable::DrawMode an enum class #13424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c101433
Made Paintable::DrawMode an enum class
JoergAtGithub Jun 30, 2024
8d2da16
Changed enum values to TitleCase
JoergAtGithub Jun 30, 2024
ab99424
Implemented fast map lookup for backward compatible DrawMode strings
JoergAtGithub Jul 1, 2024
9b6997f
Merge remote-tracking branch 'upstream/main' into PaintableDrawmodeEn…
JoergAtGithub Jul 1, 2024
74fac0a
Use early return style
JoergAtGithub Jul 1, 2024
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
80 changes: 41 additions & 39 deletions src/widget/paintable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,38 +15,39 @@

// static
Paintable::DrawMode Paintable::DrawModeFromString(const QString& str) {
if (str.compare("FIXED", Qt::CaseInsensitive) == 0) {
return FIXED;
} else if (str.compare("STRETCH", Qt::CaseInsensitive) == 0) {
return STRETCH;
} else if (str.compare("STRETCH_ASPECT", Qt::CaseInsensitive) == 0) {
return STRETCH_ASPECT;
} else if (str.compare("TILE", Qt::CaseInsensitive) == 0) {
return TILE;
static const QMap<QString, DrawMode> stringMap = {
{"FIXED", DrawMode::Fixed},
{"STRETCH", DrawMode::Stretch},
{"STRETCH_ASPECT", DrawMode::StretchAspect},
{"TILE", DrawMode::Tile}};

auto it = stringMap.find(str.toUpper());
if (it == stringMap.end()) {
qWarning() << "Unknown DrawMode string passed to DrawModeFromString:"
<< str << "using DrawMode::Fixed as fallback";
return DrawMode::Fixed;
}

// Fall back on the implicit default from before Mixxx supported draw modes.
qWarning() << "Unknown DrawMode string in DrawModeFromString:"
<< str << "using FIXED";
return FIXED;
return it.value();
}

// static
QString Paintable::DrawModeToString(DrawMode mode) {
switch (mode) {
case FIXED:
return "FIXED";
case STRETCH:
return "STRETCH";
case STRETCH_ASPECT:
return "STRETCH_ASPECT";
case TILE:
return "TILE";
static const QMap<DrawMode, QString> modeMap = {
{DrawMode::Fixed, "FIXED"},
{DrawMode::Stretch, "STRETCH"},
{DrawMode::StretchAspect, "STRETCH_ASPECT"},
{DrawMode::Tile, "TILE"}};

auto it = modeMap.find(mode);
if (it == modeMap.end()) {
qWarning() << "Unknown DrawMode passed to DrawModeToString "
<< static_cast<int>(mode) << "using FIXED as fallback";
DEBUG_ASSERT(false);
return "FIXED";
}
// Fall back on the implicit default from before Mixxx supported draw modes.
qWarning() << "Unknown DrawMode in DrawModeToString " << mode
<< "using FIXED";
return "FIXED";

return it.value();
}

Paintable::Paintable(const PixmapSource& source, DrawMode mode, double scaleFactor)
Expand All @@ -72,13 +73,14 @@ Paintable::Paintable(const PixmapSource& source, DrawMode mode, double scaleFact
m_pSvg.reset(pSvg.release());
#ifdef __APPLE__
// Apple does Retina scaling behind the scenes, so we also pass a
// Paintable::FIXED image. On the other targets, it is better to
// cache the pixmap. We do not do this for TILE and color schemas.
// DrawMode::Fixed image. On the other targets, it is better to
// cache the pixmap. We do not do this for Tile and color schemas.
// which can result in a correct but possibly blurry picture at a
// Retina display. This can be fixed when switching to QT5
if (mode == TILE || WPixmapStore::willCorrectColors()) {
if (mode == DrawMode::Tile || WPixmapStore::willCorrectColors()) {
#else
if (mode == TILE || mode == Paintable::FIXED || WPixmapStore::willCorrectColors()) {
if (mode == DrawMode::Tile || mode == DrawMode::Fixed ||
WPixmapStore::willCorrectColors()) {
#endif
// The SVG renderer doesn't directly support tiling, so we render
// it to a pixmap which will then get tiled.
Expand Down Expand Up @@ -162,7 +164,7 @@ void Paintable::draw(const QRectF& targetRect, QPainter* pPainter,
}

switch (m_drawMode) {
case FIXED: {
case DrawMode::Fixed: {
// Only render the minimum overlapping rectangle between the source
// and target.
QSizeF fixedSize(math_min(sourceRect.width(), targetRect.width()),
Expand All @@ -172,7 +174,7 @@ void Paintable::draw(const QRectF& targetRect, QPainter* pPainter,
drawInternal(adjustedTarget, pPainter, adjustedSource);
break;
}
case STRETCH_ASPECT: {
case DrawMode::StretchAspect: {
qreal sx = targetRect.width() / sourceRect.width();
qreal sy = targetRect.height() / sourceRect.height();

Expand All @@ -189,10 +191,10 @@ void Paintable::draw(const QRectF& targetRect, QPainter* pPainter,
}
break;
}
case STRETCH:
case DrawMode::Stretch:
drawInternal(targetRect, pPainter, sourceRect);
break;
case TILE:
case DrawMode::Tile:
drawInternal(targetRect, pPainter, sourceRect);
break;
}
Expand All @@ -201,7 +203,7 @@ void Paintable::draw(const QRectF& targetRect, QPainter* pPainter,
void Paintable::drawCentered(const QRectF& targetRect, QPainter* pPainter,
const QRectF& sourceRect) {
switch (m_drawMode) {
case FIXED: {
case DrawMode::Fixed: {
// Only render the minimum overlapping rectangle between the source
// and target.
QSizeF fixedSize(math_min(sourceRect.width(), targetRect.width()),
Expand All @@ -214,7 +216,7 @@ void Paintable::drawCentered(const QRectF& targetRect, QPainter* pPainter,
drawInternal(adjustedTarget, pPainter, adjustedSource);
break;
}
case STRETCH_ASPECT: {
case DrawMode::StretchAspect: {
qreal sx = targetRect.width() / sourceRect.width();
qreal sy = targetRect.height() / sourceRect.height();

Expand All @@ -231,10 +233,10 @@ void Paintable::drawCentered(const QRectF& targetRect, QPainter* pPainter,
}
break;
}
case STRETCH:
case DrawMode::Stretch:
drawInternal(targetRect, pPainter, sourceRect);
break;
case TILE:
case DrawMode::Tile:
// TODO(XXX): What's the right behavior here? Draw the first tile at the
// center point and then tile all around it based on that?
drawInternal(targetRect, pPainter, sourceRect);
Expand All @@ -247,7 +249,7 @@ void Paintable::drawInternal(const QRectF& targetRect, QPainter* pPainter,
// qDebug() << "Paintable::drawInternal" << DrawModeToString(m_draw_mode)
// << targetRect << sourceRect;
if (m_pPixmap) {
if (m_drawMode == TILE) {
if (m_drawMode == DrawMode::Tile) {
// TODO(rryan): Using a source rectangle doesn't make much sense
// with tiling. Ignore the source rect and tile our natural size
// across the target rect. What's the right general behavior here?
Expand All @@ -261,7 +263,7 @@ void Paintable::drawInternal(const QRectF& targetRect, QPainter* pPainter,
sourceRect.toRect());
}
} else if (m_pSvg) {
if (m_drawMode == TILE) {
if (m_drawMode == DrawMode::Tile) {
qWarning() << "Tiled SVG should have been rendered to pixmap!";
} else {
// NOTE(rryan): QSvgRenderer render does not clip for us -- it
Expand Down
10 changes: 5 additions & 5 deletions src/widget/paintable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,15 @@ class QSvgRenderer;
// high fidelity.
class Paintable {
public:
enum DrawMode {
enum class DrawMode {
Copy link
Member

Choose a reason for hiding this comment

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

if this change results in a touch to every point of use anyways, can you also change the members to TitleCase?

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 noticed, that the upper case strings are used accross the skin XML files. Therefore the string must remain upper case for backward compatibility.
PS: I just added a debug assert, which worked;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Now we have to decide, should we switch to Q_ENUM or should we rename to TitelCase?
Since Q_ENUM adds a overhead, I would prefer to rename and implement these two functions otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

Since Q_ENUM adds a overhead

Out of curiosity, in what sense? From what I can tell, it only adds a little bit of compilation time due to the MOC.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the class must be a QObject and the MOC compilation comes on top.

Copy link
Member

Choose a reason for hiding this comment

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

The QObject could be avoided if the enum was moved to a namespace technically. Though I'm fine with leaving it as a plain enum class for now. Its unlikely that this enum would need to get exposed to the QML environment anyways (which is where the biggest value of a QEnum lies IMO).

Copy link
Member Author

Choose a reason for hiding this comment

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

This enum is only used in our legacy widgets. A use with QML is highly unlikely.

// Draw the image in its native dimensions with no stretching or tiling.
FIXED,
Fixed,
// Stretch the image.
STRETCH,
Stretch,
// Stretch the image maintaining its aspect ratio.
STRETCH_ASPECT,
StretchAspect,
// Tile the image.
TILE
Tile
};

Paintable(const PixmapSource& source, DrawMode mode, double scaleFactor);
Expand Down
18 changes: 9 additions & 9 deletions src/widget/wbattery.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,17 @@ void WBattery::setup(const QDomNode& node, const SkinContext& context) {
QDomElement backPath = context.selectElement(node, "BackPath");
if (!backPath.isNull()) {
setPixmap(&m_pPixmapBack,
context.getPixmapSource(backPath),
context.selectScaleMode(backPath, Paintable::TILE),
context.getScaleFactor());
context.getPixmapSource(backPath),
context.selectScaleMode(backPath, Paintable::DrawMode::Tile),
context.getScaleFactor());
}

QDomElement chargedPath = context.selectElement(node, "PixmapCharged");
if (!chargedPath.isNull()) {
setPixmap(&m_pPixmapCharged,
context.getPixmapSource(chargedPath),
context.selectScaleMode(chargedPath, Paintable::TILE),
context.getScaleFactor());
context.getPixmapSource(chargedPath),
context.selectScaleMode(chargedPath, Paintable::DrawMode::Tile),
context.getScaleFactor());
}

int numberStates = context.selectInt(node, "NumberStates");
Expand All @@ -50,7 +50,7 @@ void WBattery::setup(const QDomNode& node, const SkinContext& context) {
// TODO(XXX) inline SVG support via context.getPixmapSource.
QString chargingPath = context.nodeToString(pixmapsCharging);
Paintable::DrawMode mode = context.selectScaleMode(pixmapsCharging,
Paintable::TILE);
Paintable::DrawMode::Tile);
for (int i = 0; i < m_chargingPixmaps.size(); ++i) {
PixmapSource source = context.getPixmapSource(chargingPath.arg(i));
setPixmap(&m_chargingPixmaps[i], source, mode, context.getScaleFactor());
Expand All @@ -62,7 +62,7 @@ void WBattery::setup(const QDomNode& node, const SkinContext& context) {
// TODO(XXX) inline SVG support via context.getPixmapSource.
QString dischargingPath = context.nodeToString(pixmapsDischarging);
Paintable::DrawMode mode = context.selectScaleMode(pixmapsDischarging,
Paintable::TILE);
Paintable::DrawMode::Tile);
for (int i = 0; i < m_dischargingPixmaps.size(); ++i) {
PixmapSource source = context.getPixmapSource(dischargingPath.arg(i));
setPixmap(&m_dischargingPixmaps[i], source, mode, context.getScaleFactor());
Expand Down Expand Up @@ -148,7 +148,7 @@ void WBattery::setPixmap(PaintablePointer* ppPixmap, const PixmapSource& source,
qDebug() << this << "Error loading pixmap:" << source.getPath();
} else {
*ppPixmap = pPixmap;
if (mode == Paintable::FIXED) {
if (mode == Paintable::DrawMode::Fixed) {
setFixedSize(pPixmap->size());
}
}
Expand Down
10 changes: 5 additions & 5 deletions src/widget/wdisplay.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ void WDisplay::setup(const QDomNode& node, const SkinContext& context) {
QDomElement backPathNode = context.selectElement(node, "BackPath");
if (!backPathNode.isNull()) {
setPixmapBackground(context.getPixmapSource(backPathNode),
context.selectScaleMode(backPathNode, Paintable::TILE),
context.getScaleFactor());
context.selectScaleMode(backPathNode, Paintable::DrawMode::Tile),
context.getScaleFactor());
}

// Number of states
Expand All @@ -39,7 +39,7 @@ void WDisplay::setup(const QDomNode& node, const SkinContext& context) {
// The implicit default in <1.12.0 was FIXED so we keep it for
// backwards compatibility.
Paintable::DrawMode pathMode =
context.selectScaleMode(pathNode, Paintable::FIXED);
context.selectScaleMode(pathNode, Paintable::DrawMode::Fixed);
for (int i = 0; i < m_pixmaps.size(); ++i) {
setPixmap(&m_pixmaps, i, context.makeSkinPath(path.arg(i)),
pathMode, context.getScaleFactor());
Expand All @@ -52,7 +52,7 @@ void WDisplay::setup(const QDomNode& node, const SkinContext& context) {
// The implicit default in <1.12.0 was FIXED so we keep it for
// backwards compatibility.
Paintable::DrawMode disabledMode =
context.selectScaleMode(disabledNode, Paintable::FIXED);
context.selectScaleMode(disabledNode, Paintable::DrawMode::Fixed);
for (int i = 0; i < m_disabledPixmaps.size(); ++i) {
setPixmap(&m_disabledPixmaps, i,
context.makeSkinPath(disabledPath.arg(i)),
Expand Down Expand Up @@ -108,7 +108,7 @@ void WDisplay::setPixmap(
<< "Error loading pixmap:" << filename;
} else {
(*pPixmaps)[iPos] = pPixmap;
if (mode == Paintable::FIXED) {
if (mode == Paintable::DrawMode::Fixed) {
setFixedSize(pPixmap->size());
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/widget/wknobcomposed.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ void WKnobComposed::setup(const QDomNode& node, const SkinContext& context) {
if (!backPathElement.isNull()) {
setPixmapBackground(
context.getPixmapSource(backPathElement),
context.selectScaleMode(backPathElement, Paintable::STRETCH),
context.selectScaleMode(backPathElement, Paintable::DrawMode::Stretch),
scaleFactor);
}

Expand All @@ -42,7 +42,7 @@ void WKnobComposed::setup(const QDomNode& node, const SkinContext& context) {
if (!knobNode.isNull()) {
setPixmapKnob(
context.getPixmapSource(knobNode),
context.selectScaleMode(knobNode, Paintable::STRETCH),
context.selectScaleMode(knobNode, Paintable::DrawMode::Stretch),
scaleFactor);
}

Expand Down
8 changes: 4 additions & 4 deletions src/widget/wpushbutton.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ void WPushButton::setup(const QDomNode& node, const SkinContext& context) {
// backwards compatibility.
setPixmapBackground(
backgroundSource,
context.selectScaleMode(backPathNode, Paintable::FIXED),
context.selectScaleMode(backPathNode, Paintable::DrawMode::Fixed),
context.getScaleFactor());
}
}
Expand Down Expand Up @@ -97,7 +97,7 @@ void WPushButton::setup(const QDomNode& node, const SkinContext& context) {
// The implicit default in <1.12.0 was FIXED so we keep it for
// backwards compatibility.
Paintable::DrawMode unpressedMode =
stateContext->selectScaleMode(unpressedNode, Paintable::FIXED);
stateContext->selectScaleMode(unpressedNode, Paintable::DrawMode::Fixed);
if (!pixmapSource.isEmpty()) {
setPixmap(iState, false, pixmapSource,
unpressedMode, context.getScaleFactor());
Expand All @@ -108,7 +108,7 @@ void WPushButton::setup(const QDomNode& node, const SkinContext& context) {
// The implicit default in <1.12.0 was FIXED so we keep it for
// backwards compatibility.
Paintable::DrawMode pressedMode =
stateContext->selectScaleMode(pressedNode, Paintable::FIXED);
stateContext->selectScaleMode(pressedNode, Paintable::DrawMode::Fixed);
if (!pixmapSource.isEmpty()) {
setPixmap(iState, true, pixmapSource,
pressedMode, context.getScaleFactor());
Expand Down Expand Up @@ -265,7 +265,7 @@ void WPushButton::setPixmap(int iState,
if (!source.isEmpty()) {
qDebug() << "WPushButton: Error loading pixmap:" << source.getPath();
}
} else if (mode == Paintable::FIXED) {
} else if (mode == Paintable::DrawMode::Fixed) {
// Set size of widget equal to pixmap size
setFixedSize(pPixmap->size());
}
Expand Down
18 changes: 9 additions & 9 deletions src/widget/wslidercomposed.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ void WSliderComposed::setup(const QDomNode& node, const SkinContext& context) {
PixmapSource sourceSlider = context.getPixmapSource(slider);
setSliderPixmap(
sourceSlider,
context.selectScaleMode(slider, Paintable::FIXED),
context.selectScaleMode(slider, Paintable::DrawMode::Fixed),
scaleFactor);
}

Expand All @@ -76,7 +76,7 @@ void WSliderComposed::setup(const QDomNode& node, const SkinContext& context) {
// compatibility.
setHandlePixmap(
sourceHandle,
context.selectScaleMode(handle, Paintable::FIXED),
context.selectScaleMode(handle, Paintable::DrawMode::Fixed),
scaleFactor);

// Set up the level bar.
Expand Down Expand Up @@ -183,7 +183,7 @@ void WSliderComposed::setSliderPixmap(const PixmapSource& sourceSlider,
m_pSlider = WPixmapStore::getPaintable(sourceSlider, drawMode, scaleFactor);
if (!m_pSlider) {
qDebug() << "WSliderComposed: Error loading slider pixmap:" << sourceSlider.getPath();
} else if (drawMode == Paintable::FIXED) {
} else if (drawMode == Paintable::DrawMode::Fixed) {
// Set size of widget, using size of slider pixmap
setFixedSize(m_pSlider->size());
}
Expand Down Expand Up @@ -353,10 +353,10 @@ double WSliderComposed::calculateHandleLength() {
Paintable::DrawMode mode = m_pHandle->drawMode();
if (m_bHorizontal) {
// Stretch the pixmap to be the height of the widget.
if (mode == Paintable::FIXED || mode == Paintable::STRETCH ||
mode == Paintable::TILE || m_pHandle->height() == 0.0) {
if (mode == Paintable::DrawMode::Fixed || mode == Paintable::DrawMode::Stretch ||
mode == Paintable::DrawMode::Tile || m_pHandle->height() == 0.0) {
return m_pHandle->width();
} else if (mode == Paintable::STRETCH_ASPECT) {
} else if (mode == Paintable::DrawMode::StretchAspect) {
const int iHeight = m_pHandle->height();
if (iHeight == 0) {
qDebug() << "WSliderComposed: Invalid height.";
Expand All @@ -368,10 +368,10 @@ double WSliderComposed::calculateHandleLength() {
}
} else {
// Stretch the pixmap to be the width of the widget.
if (mode == Paintable::FIXED || mode == Paintable::STRETCH ||
mode == Paintable::TILE || m_pHandle->width() == 0.0) {
if (mode == Paintable::DrawMode::Fixed || mode == Paintable::DrawMode::Stretch ||
mode == Paintable::DrawMode::Tile || m_pHandle->width() == 0.0) {
return m_pHandle->height();
} else if (mode == Paintable::STRETCH_ASPECT) {
} else if (mode == Paintable::DrawMode::StretchAspect) {
const int iWidth = m_pHandle->width();
if (iWidth == 0) {
qDebug() << "WSliderComposed: Invalid width.";
Expand Down
4 changes: 2 additions & 2 deletions src/widget/wspinnybase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,8 @@ void WSpinnyBase::setup(const QDomNode& node,
m_pBgImage = WImageStore::getImage(context.getPixmapSource(backPathElement),
context.getScaleFactor());
Paintable::DrawMode bgmode = context.selectScaleMode(backPathElement,
Paintable::FIXED);
if (m_pBgImage && !m_pBgImage->isNull() && bgmode == Paintable::FIXED) {
Paintable::DrawMode::Fixed);
if (m_pBgImage && !m_pBgImage->isNull() && bgmode == Paintable::DrawMode::Fixed) {
setFixedSize(m_pBgImage->size());
} else {
setSizePolicy(QSizePolicy::MinimumExpanding, QSizePolicy::MinimumExpanding);
Expand Down
Loading
Loading