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

Jump to bottom

Safe Global Predefined Color Palettes #13598

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+451 −453
Open

Safe Global Predefined Color Palettes #13598

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
1c7afe6
refactor: initialize global static const color palettes in first use
Swiftb0y Aug 25, 2024
e710203
feat: reduce ColorPalette duplications.
Swiftb0y Oct 11, 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
1 change: 1 addition & 0 deletions .codespellignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ ReflectIn
bufferIn
indexIn
allLocations
statics
6 changes: 3 additions & 3 deletions src/dialog/dlgreplacecuecolor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ DlgReplaceCueColor::DlgReplaceCueColor(
// Set up new color button
ColorPaletteSettings colorPaletteSettings(pConfig);
ColorPalette hotcuePalette = colorPaletteSettings.getHotcueColorPalette();
mixxx::RgbColor firstColor = mixxx::PredefinedColorPalettes::kDefaultCueColor;
mixxx::RgbColor firstColor = mixxx::predefinedcolorpalettes::kDefaultCueColor;
DEBUG_ASSERT(hotcuePalette.size() > 0);
if (hotcuePalette.size() > 0) { // Should always be true
firstColor = hotcuePalette.at(0);
Expand Down Expand Up @@ -116,7 +116,7 @@ DlgReplaceCueColor::DlgReplaceCueColor(
// Set up 'Current color' button
setButtonColor(pushButtonCurrentColor,
mixxx::RgbColor::toQColor(
mixxx::PredefinedColorPalettes::kDefaultCueColor));
mixxx::predefinedcolorpalettes::kDefaultCueColor));

// Update apply button when the current color comparison combobox is
// modified
Expand All @@ -134,7 +134,7 @@ DlgReplaceCueColor::DlgReplaceCueColor(
this);
m_pCurrentColorPickerAction->setObjectName("HotcueColorPickerAction");
m_pCurrentColorPickerAction->setSelectedColor(
mixxx::PredefinedColorPalettes::kDefaultCueColor);
mixxx::predefinedcolorpalettes::kDefaultCueColor);
connect(m_pCurrentColorPickerAction,
&WColorPickerAction::colorPicked,
this,
Expand Down
2 changes: 1 addition & 1 deletion src/engine/controls/cuecontrol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -932,7 +932,7 @@ void CueControl::hotcueSet(HotcueControl* pControl, double value, HotcueSetMode

int hotcueIndex = pControl->getHotcueIndex();

mixxx::RgbColor color = mixxx::PredefinedColorPalettes::kDefaultCueColor;
mixxx::RgbColor color = mixxx::predefinedcolorpalettes::kDefaultCueColor;
if (cueType == mixxx::CueType::Loop) {
ConfigKey autoLoopColorsKey("[Controls]", "auto_loop_colors");
if (getConfig()->getValue(autoLoopColorsKey, false)) {
Expand Down
22 changes: 13 additions & 9 deletions src/preferences/colorpaletteeditor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

#include "moc_colorpaletteeditor.cpp"
#include "preferences/colorpalettesettings.h"
#include "util/color/colorpalette.h"
#include "util/color/predefinedcolorpalettes.h"

namespace {
Expand Down Expand Up @@ -146,8 +147,10 @@ void ColorPaletteEditor::initialize(
m_resetPalette = paletteName;
QString saveName = paletteName;

for (const ColorPalette& palette : mixxx::PredefinedColorPalettes::kPalettes) {
if (paletteName == palette.getName()) {
const auto& kPalettes = mixxx::predefinedcolorpalettes::get();
Copy link
Member

Choose a reason for hiding this comment

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

A free get() function in a namespace looks weird. This is an indicator that the Namespace is an object.

Copy link
Member Author

Choose a reason for hiding this comment

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

well, its effectively a singleton. So its up to interpretation whether it should be modeled as a class with a static member function or a namespace with a free function. they're basically equivalent, though I find the namespace more elegant.


for (mixxx::predefinedcolorpalettes::ColorPalettePointer palette : kPalettes.palettes) {
if (paletteName == palette->getName()) {
saveName = paletteName + QStringLiteral(" (") + tr("Edited") + QChar(')');
ColorPaletteSettings colorPaletteSettings(m_pConfig);
if (colorPaletteSettings.getColorPaletteNames().contains(saveName)) {
Expand Down Expand Up @@ -206,8 +209,9 @@ void ColorPaletteEditor::slotRemoveColor() {
void ColorPaletteEditor::slotPaletteNameChanged(const QString& text) {
bool bPaletteIsReadOnly = false;
bool bPaletteExists = false;
for (const ColorPalette& palette : mixxx::PredefinedColorPalettes::kPalettes) {
if (text == palette.getName()) {
const auto& kPalettes = mixxx::predefinedcolorpalettes::get();
for (mixxx::predefinedcolorpalettes::ColorPalettePointer palette : kPalettes.palettes) {
if (text == palette->getName()) {
bPaletteExists = true;
bPaletteIsReadOnly = true;
break;
Expand All @@ -222,16 +226,16 @@ void ColorPaletteEditor::slotPaletteNameChanged(const QString& text) {
if (bPaletteExists) {
if (!m_pModel->isDirty()) {
bool bPaletteFound = false;
for (const ColorPalette& palette : mixxx::PredefinedColorPalettes::kPalettes) {
if (text == palette.getName()) {
for (mixxx::predefinedcolorpalettes::ColorPalettePointer palette : kPalettes.palettes) {
if (text == palette->getName()) {
bPaletteFound = true;
m_pModel->setColorPalette(palette);
m_pModel->setColorPalette(*palette);
break;
}
}
if (!bPaletteFound) {
m_pModel->setColorPalette(colorPaletteSettings.getColorPalette(
text, mixxx::PredefinedColorPalettes::kDefaultHotcueColorPalette));
text, kPalettes.defaultHotcueColorPalette));
}
}
}
Expand Down Expand Up @@ -277,7 +281,7 @@ void ColorPaletteEditor::slotResetButtonClicked() {
ColorPaletteSettings colorPaletteSettings(m_pConfig);
ColorPalette palette = colorPaletteSettings.getColorPalette(
m_resetPalette,
mixxx::PredefinedColorPalettes::kDefaultHotcueColorPalette);
mixxx::predefinedcolorpalettes::get().defaultHotcueColorPalette);
m_pModel->setColorPalette(palette);
slotUpdateButtons();
}
Expand Down
19 changes: 11 additions & 8 deletions src/preferences/colorpalettesettings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include <QRegularExpression>

#include "util/color/colorpalette.h"
#include "util/color/predefinedcolorpalettes.h"

namespace {
Expand Down Expand Up @@ -39,9 +40,10 @@ ColorPalette ColorPaletteSettings::getColorPalette(
}

// If we find a predefined palette with this name, return it
for (const ColorPalette& palette : mixxx::PredefinedColorPalettes::kPalettes) {
if (name == palette.getName()) {
return palette;
for (mixxx::predefinedcolorpalettes::ColorPalettePointer palette :
mixxx::predefinedcolorpalettes::get().palettes) {
if (name == palette->getName()) {
return *palette;
}
}

Expand Down Expand Up @@ -87,8 +89,9 @@ void ColorPaletteSettings::setColorPalette(const QString& name, const ColorPalet
return;
}

for (const ColorPalette& palette : mixxx::PredefinedColorPalettes::kPalettes) {
if (name == palette.getName()) {
for (mixxx::predefinedcolorpalettes::ColorPalettePointer palette :
mixxx::predefinedcolorpalettes::get().palettes) {
if (name == palette->getName()) {
qDebug() << "Color Palette" << name << "is a built-in palette, not writing to config!";
return;
}
Expand Down Expand Up @@ -131,7 +134,7 @@ ColorPalette ColorPaletteSettings::getHotcueColorPalette(
const QString& name) const {
return getColorPalette(
name,
mixxx::PredefinedColorPalettes::kDefaultHotcueColorPalette);
mixxx::predefinedcolorpalettes::get().defaultHotcueColorPalette);
}

void ColorPaletteSettings::setHotcueColorPalette(const ColorPalette& colorPalette) {
Expand All @@ -148,7 +151,7 @@ ColorPalette ColorPaletteSettings::getTrackColorPalette(
const QString& name) const {
return getColorPalette(
name,
mixxx::PredefinedColorPalettes::kDefaultTrackColorPalette);
mixxx::predefinedcolorpalettes::get().defaultTrackColorPalette);
}

ColorPalette ColorPaletteSettings::getTrackColorPalette() const {
Expand All @@ -170,7 +173,7 @@ ColorPalette ColorPaletteSettings::getKeyColorPalette(
const QString& name) const {
return getColorPalette(
name,
mixxx::PredefinedColorPalettes::kDefaultKeyColorPalette);
mixxx::predefinedcolorpalettes::get().defaultKeyColorPalette);
}

ColorPalette ColorPaletteSettings::getConfigKeyColorPalette() const {
Expand Down
40 changes: 19 additions & 21 deletions src/preferences/dialog/dlgprefcolors.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,8 +94,9 @@ void DlgPrefColors::slotUpdate() {
checkboxKeyColorsEnabled->setChecked(
m_pConfig->getValue(kKeyColorsEnabledConfigKey,
BaseTrackTableModel::kKeyColorsEnabledDefault));
for (const auto& palette : std::as_const(mixxx::PredefinedColorPalettes::kPalettes)) {
QString paletteName = palette.getName();
for (mixxx::predefinedcolorpalettes::ColorPalettePointer palette :
mixxx::predefinedcolorpalettes::get().palettes) {
QString paletteName = palette->getName();
QString translatedName = QCoreApplication::translate(
"PredefinedColorPalettes", qPrintable(paletteName));
QIcon paletteIcon = drawPalettePreview(paletteName);
Expand All @@ -109,7 +110,7 @@ void DlgPrefColors::slotUpdate() {
comboBoxTrackColors->count() - 1,
paletteIcon);

if (palette.size() == 12) {
if (palette->size() == 12) {
comboBoxKeyColors->addItem(translatedName, paletteName);
comboBoxKeyColors->setItemIcon(
comboBoxKeyColors->count() - 1,
Expand Down Expand Up @@ -184,23 +185,19 @@ void DlgPrefColors::slotUpdate() {

// Set the default values for all the widgets
void DlgPrefColors::slotResetToDefaults() {
const auto& kPalettes = mixxx::predefinedcolorpalettes::get();
comboBoxHotcueColors->setCurrentText(QCoreApplication::translate(
"PredefinedColorPalettes",
qPrintable(
mixxx::PredefinedColorPalettes::kDefaultHotcueColorPalette
.getName())));
kPalettes.defaultHotcueColorPalette.getName())));
comboBoxTrackColors->setCurrentText(QCoreApplication::translate(
"PredefinedColorPalettes",
qPrintable(mixxx::PredefinedColorPalettes::kDefaultTrackColorPalette
.getName())));
qPrintable(kPalettes.defaultTrackColorPalette.getName())));
comboBoxKeyColors->setCurrentText(QCoreApplication::translate(
"PredefinedColorPalettes",
qPrintable(mixxx::PredefinedColorPalettes::kDefaultKeyColorPalette
.getName())));
comboBoxHotcueDefaultColor->setCurrentIndex(
mixxx::PredefinedColorPalettes::kDefaultTrackColorPalette.size());
comboBoxLoopDefaultColor->setCurrentIndex(
mixxx::PredefinedColorPalettes::kDefaultTrackColorPalette.size() - 1);
qPrintable(kPalettes.defaultKeyColorPalette.getName())));
comboBoxHotcueDefaultColor->setCurrentIndex(kPalettes.defaultTrackColorPalette.size());
comboBoxLoopDefaultColor->setCurrentIndex(kPalettes.defaultTrackColorPalette.size() - 1);
checkboxKeyColorsEnabled->setChecked(BaseTrackTableModel::kKeyColorsEnabledDefault);
}

Expand All @@ -213,21 +210,22 @@ void DlgPrefColors::slotApply() {
bool bTrackColorPaletteFound = false;
bool bKeyColorPaletteFound = false;

for (const auto& palette :
std::as_const(mixxx::PredefinedColorPalettes::kPalettes)) {
for (mixxx::predefinedcolorpalettes::ColorPalettePointer palette :
mixxx::predefinedcolorpalettes::get().palettes) {
QString name = palette->getName();
if (!bHotcueColorPaletteFound &&
hotcueColorPaletteName == palette.getName()) {
m_colorPaletteSettings.setHotcueColorPalette(palette);
hotcueColorPaletteName == name) {
m_colorPaletteSettings.setHotcueColorPalette(*palette);
bHotcueColorPaletteFound = true;
}
if (!bTrackColorPaletteFound &&
trackColorPaletteName == palette.getName()) {
m_colorPaletteSettings.setTrackColorPalette(palette);
trackColorPaletteName == name) {
m_colorPaletteSettings.setTrackColorPalette(*palette);
bTrackColorPaletteFound = true;
}
if (!bKeyColorPaletteFound &&
keyColorPaletteName == palette.getName()) {
m_colorPaletteSettings.setKeyColorPalette(palette);
keyColorPaletteName == name) {
m_colorPaletteSettings.setKeyColorPalette(*palette);
bKeyColorPaletteFound = true;
}
}
Expand Down
5 changes: 3 additions & 2 deletions src/test/colorconfig_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,9 @@ TEST_F(ColorConfigTest, LoadSavePalettes) {

TEST_F(ColorConfigTest, DefaultColorPalette) {
ColorPaletteSettings colorPaletteSettings(config());
ASSERT_EQ(mixxx::PredefinedColorPalettes::kDefaultHotcueColorPalette,
const auto& kPalettes = mixxx::predefinedcolorpalettes::get();
ASSERT_EQ(kPalettes.defaultHotcueColorPalette,
colorPaletteSettings.getHotcueColorPalette());
ASSERT_EQ(mixxx::PredefinedColorPalettes::kDefaultTrackColorPalette,
ASSERT_EQ(kPalettes.defaultTrackColorPalette,
colorPaletteSettings.getTrackColorPalette());
}
14 changes: 7 additions & 7 deletions src/test/colorpalette_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,25 +5,25 @@
#include "test/mixxxtest.h"
#include "util/color/predefinedcolorpalettes.h"

class ColorPaletteTest : public MixxxTest {};
class ColorPaletteTest : public MixxxTest {
protected:
ColorPalette palette{mixxx::predefinedcolorpalettes::get().defaultHotcueColorPalette};
void SetUp() override {
ASSERT_TRUE(palette.size() >= 1);
}
};

TEST_F(ColorPaletteTest, NextColor) {
const ColorPalette palette = mixxx::PredefinedColorPalettes::kDefaultHotcueColorPalette;
ASSERT_TRUE(palette.size() >= 1);
ASSERT_EQ(palette.nextColor(palette.at(0)), palette.at(1));
ASSERT_EQ(palette.nextColor(palette.at(palette.size() - 1)), palette.at(0));
}

TEST_F(ColorPaletteTest, PreviousColor) {
const ColorPalette palette = mixxx::PredefinedColorPalettes::kDefaultHotcueColorPalette;
ASSERT_TRUE(palette.size() >= 1);
ASSERT_EQ(palette.previousColor(palette.at(1)), palette.at(0));
ASSERT_EQ(palette.previousColor(palette.at(0)), palette.at(palette.size() - 1));
}

TEST_F(ColorPaletteTest, NextAndPreviousColorRoundtrip) {
const ColorPalette palette = mixxx::PredefinedColorPalettes::kDefaultHotcueColorPalette;
ASSERT_TRUE(palette.size() >= 1);
ASSERT_EQ(palette.nextColor(palette.previousColor(palette.at(0))), palette.at(0));
ASSERT_EQ(palette.nextColor(palette.previousColor(palette.at(palette.size() - 1))), palette.at(palette.size() - 1));
ASSERT_EQ(palette.previousColor(palette.nextColor(palette.at(0))), palette.at(0));
Expand Down
2 changes: 1 addition & 1 deletion src/test/cue_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ TEST(CueTest, NewCueIsDirty) {
1,
mixxx::audio::kStartFramePos,
mixxx::audio::kInvalidFramePos,
mixxx::PredefinedColorPalettes::kDefaultCueColor);
mixxx::predefinedcolorpalettes::kDefaultCueColor);
EXPECT_TRUE(cue.isDirty());
}

Expand Down
6 changes: 3 additions & 3 deletions src/test/seratotagstest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,14 +256,14 @@ TEST_F(SeratoTagsTest, SetIncompatibleCueInfos) {
}

TEST_F(SeratoTagsTest, CueColorConversionRoundtrip) {
for (const auto color : mixxx::PredefinedColorPalettes::
kSeratoTrackMetadataHotcueColorPalette) {
const auto& kPalettes = mixxx::predefinedcolorpalettes::get();
for (const auto color : kPalettes.seratoTrackMetadataHotcueColorPalette) {
const auto displayedColor = mixxx::SeratoStoredHotcueColor(color).toDisplayedColor();
const auto storedColor = mixxx::SeratoStoredHotcueColor::fromDisplayedColor(displayedColor);
EXPECT_EQ(color, storedColor.toQRgb());
}

for (const auto color : mixxx::PredefinedColorPalettes::kSeratoDJProHotcueColorPalette) {
for (const auto color : kPalettes.seratoDJProHotcueColorPalette) {
const auto storedColor = mixxx::SeratoStoredHotcueColor::fromDisplayedColor(color);
const auto displayedColor = storedColor.toDisplayedColor();
EXPECT_EQ(color, displayedColor);
Expand Down
2 changes: 1 addition & 1 deletion src/track/cue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ Cue::Cue(
sampleRate)),
m_iHotCue(cueInfo.getHotCueIndex().value_or(kNoHotCue)),
m_label(cueInfo.getLabel()),
m_color(cueInfo.getColor().value_or(mixxx::PredefinedColorPalettes::kDefaultCueColor)) {
m_color(cueInfo.getColor().value_or(mixxx::predefinedcolorpalettes::kDefaultCueColor)) {
DEBUG_ASSERT(!m_dbId.isValid());
}

Expand Down
10 changes: 6 additions & 4 deletions src/track/serato/color.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,11 @@ RgbColor::optional_t SeratoStoredHotcueColor::toDisplayedColor() const {
if (m_color == SeratoStoredColor::kNoColor) {
return RgbColor::nullopt();
}
const auto& kPalettes = mixxx::predefinedcolorpalettes::get();

return RgbColor::optional(getColorFromOtherPalette(
PredefinedColorPalettes::kSeratoTrackMetadataHotcueColorPalette,
PredefinedColorPalettes::kSeratoDJProHotcueColorPalette,
kPalettes.seratoTrackMetadataHotcueColorPalette,
kPalettes.seratoDJProHotcueColorPalette,
m_color));
}

Expand All @@ -92,9 +93,10 @@ SeratoStoredHotcueColor SeratoStoredHotcueColor::fromDisplayedColor(RgbColor::op
if (!color) {
return SeratoStoredHotcueColor(SeratoStoredColor::kNoColor);
}
const auto& kPalettes = mixxx::predefinedcolorpalettes::get();
return SeratoStoredHotcueColor(getColorFromOtherPalette(
PredefinedColorPalettes::kSeratoDJProHotcueColorPalette,
PredefinedColorPalettes::kSeratoTrackMetadataHotcueColorPalette,
kPalettes.seratoDJProHotcueColorPalette,
kPalettes.seratoTrackMetadataHotcueColorPalette,
*color));
}

Expand Down
2 changes: 1 addition & 1 deletion src/track/track.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -955,7 +955,7 @@ void Track::setMainCuePosition(mixxx::audio::FramePos position) {
Cue::kNoHotCue,
position,
mixxx::audio::kInvalidFramePos,
mixxx::PredefinedColorPalettes::kDefaultCueColor));
mixxx::predefinedcolorpalettes::kDefaultCueColor));
// While this method could be called from any thread,
// associated Cue objects should always live on the
// same thread as their host, namely this->thread().
Expand Down
4 changes: 2 additions & 2 deletions src/track/track.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,13 +305,13 @@ class Track : public QObject {
int hotCueIndex,
mixxx::audio::FramePos startPosition,
mixxx::audio::FramePos endPosition,
mixxx::RgbColor color = mixxx::PredefinedColorPalettes::kDefaultCueColor);
mixxx::RgbColor color = mixxx::predefinedcolorpalettes::kDefaultCueColor);
CuePointer createAndAddCue(
mixxx::CueType type,
int hotCueIndex,
double startPositionSamples,
double endPositionSamples,
mixxx::RgbColor color = mixxx::PredefinedColorPalettes::kDefaultCueColor) {
mixxx::RgbColor color = mixxx::predefinedcolorpalettes::kDefaultCueColor) {
return createAndAddCue(type,
hotCueIndex,
mixxx::audio::FramePos::fromEngineSamplePosMaybeInvalid(
Expand Down
Loading
Loading