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

Allow assigning more than one Hotcue to a Color in a Colorpalette #2902

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/preferences/colorpaletteeditor.cpp
Expand Up @@ -186,7 +186,7 @@ void ColorPaletteEditor::slotTableViewDoubleClicked(const QModelIndex& index) {
}

void ColorPaletteEditor::slotAddColor() {
m_pModel->appendRow(kDefaultPaletteColor);
m_pModel->appendRow(kDefaultPaletteColor, {});
m_pTableView->scrollToBottom();
m_pTableView->setCurrentIndex(
m_pModel->index(m_pModel->rowCount() - 1, 0));
Expand Down
151 changes: 120 additions & 31 deletions src/preferences/colorpaletteeditormodel.cpp
@@ -1,5 +1,13 @@
#include "preferences/colorpaletteeditormodel.h"

#include <util/assert.h>

#include <QList>
#include <QMap>
#include <QMultiMap>
#include <QRegularExpression>
#include <algorithm>

#include "moc_colorpaletteeditormodel.cpp"

namespace {
Expand All @@ -10,6 +18,16 @@ QIcon toQIcon(const QColor& color) {
return QIcon(pixmap);
}

const QRegularExpression hotcueListMatchingRegex(QStringLiteral("(\\d+)[ ,]*"));

HotcueIndexListItem* toHotcueIndexListItem(QStandardItem* from) {
VERIFY_OR_DEBUG_ASSERT(from->type() == QStandardItem::UserType) {
// does std::optional make sense for pointers?
Copy link
Member

Choose a reason for hiding this comment

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

No, a std::optional is for value types that don't have a null value.

return nullptr;
}
return static_cast<HotcueIndexListItem*>(from);
Holzhaus marked this conversation as resolved.
Show resolved Hide resolved
}

} // namespace

ColorPaletteEditorModel::ColorPaletteEditorModel(QObject* parent)
Expand Down Expand Up @@ -51,27 +69,46 @@ bool ColorPaletteEditorModel::dropMimeData(const QMimeData* data, Qt::DropAction
}

bool ColorPaletteEditorModel::setData(const QModelIndex& modelIndex, const QVariant& value, int role) {
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'm still confused when setData on the model and when setData on the Item is called...

setDirty(true);
if (modelIndex.isValid() && modelIndex.column() == 1) {
bool ok;
int hotcueIndex = value.toInt(&ok);
const auto rollbackData = itemFromIndex(modelIndex)->data(role);

// Make sure that the value is valid
if (!ok || hotcueIndex <= 0 || hotcueIndex > rowCount()) {
return QStandardItemModel::setData(modelIndex, QVariant(), role);
}
// parse and validate in color-context
const bool initialAttemptSuccessful = QStandardItemModel::setData(modelIndex, value, role);

QList<int> allHotcueIndicies;
allHotcueIndicies.reserve(rowCount());

for (int i = 0; i < rowCount(); ++i) {
auto* hotcueIndexListItem = toHotcueIndexListItem(item(i, 1));
Swiftb0y marked this conversation as resolved.
Show resolved Hide resolved

// Make sure there is no other row with the same hotcue index
for (int i = 0; i < rowCount(); i++) {
QModelIndex otherModelIndex = index(i, 1);
QVariant otherValue = data(otherModelIndex);
int otherHotcueIndex = otherValue.toInt(&ok);
if (ok && otherHotcueIndex == hotcueIndex) {
QStandardItemModel::setData(otherModelIndex, QVariant(), role);
if (hotcueIndexListItem) {
Swiftb0y marked this conversation as resolved.
Show resolved Hide resolved
allHotcueIndicies.append(hotcueIndexListItem->getHotcueIndexList());
}
}

// validate hotcueindicies in palette context
// checks for duplicates and validates largest index

const int preDedupLen = allHotcueIndicies.length();

std::sort(allHotcueIndicies.begin(), allHotcueIndicies.end());
const auto end = std::unique(allHotcueIndicies.begin(), allHotcueIndicies.end());
allHotcueIndicies.erase(end, allHotcueIndicies.end());

const bool isOutsidePalette = !allHotcueIndicies.empty() &&
allHotcueIndicies.last() > rowCount();

if (preDedupLen != allHotcueIndicies.length() || isOutsidePalette) {
// checks failed!
// rollback cell content to previous hotcue index list
return QStandardItemModel::setData(modelIndex, rollbackData, role);
} else {
setDirty(true);
return initialAttemptSuccessful;
}
}

setDirty(true);
return QStandardItemModel::setData(modelIndex, value, role);
}

Expand All @@ -84,29 +121,26 @@ void ColorPaletteEditorModel::setColor(int row, const QColor& color) {
setDirty(true);
}

void ColorPaletteEditorModel::appendRow(const QColor& color, int hotcueIndex) {
void ColorPaletteEditorModel::appendRow(
const QColor& color, const QList<int>& hotcueIndicies) {
QStandardItem* pColorItem = new QStandardItem(toQIcon(color), color.name());
pColorItem->setEditable(false);
pColorItem->setDropEnabled(false);

QString hotcueIndexStr;
if (hotcueIndex >= 0) {
hotcueIndexStr = QString::number(hotcueIndex + 1);
}

QStandardItem* pHotcueIndexItem = new QStandardItem(hotcueIndexStr);
QStandardItem* pHotcueIndexItem = new HotcueIndexListItem(hotcueIndicies);
pHotcueIndexItem->setEditable(true);
pHotcueIndexItem->setDropEnabled(false);

QStandardItemModel::appendRow(QList<QStandardItem*>{pColorItem, pHotcueIndexItem});
QStandardItemModel::appendRow(
QList<QStandardItem*>{pColorItem, pHotcueIndexItem});
}

void ColorPaletteEditorModel::setColorPalette(const ColorPalette& palette) {
// Remove all rows
removeRows(0, rowCount());

// Make a map of hotcue indices
QMap<int, int> hotcueColorIndicesMap;
QMultiMap<int, int> hotcueColorIndicesMap;
QList<int> colorIndicesByHotcue = palette.getIndicesByHotcue();
for (int i = 0; i < colorIndicesByHotcue.size(); i++) {
int colorIndex = colorIndicesByHotcue.at(i);
Expand All @@ -115,31 +149,86 @@ void ColorPaletteEditorModel::setColorPalette(const ColorPalette& palette) {

for (int i = 0; i < palette.size(); i++) {
QColor color = mixxx::RgbColor::toQColor(palette.at(i));
int colorIndex = hotcueColorIndicesMap.value(i, kNoHotcueIndex);
QList<int> colorIndex = hotcueColorIndicesMap.values(i);
Swiftb0y marked this conversation as resolved.
Show resolved Hide resolved
appendRow(color, colorIndex);
}

setDirty(false);
}

ColorPalette ColorPaletteEditorModel::getColorPalette(const QString& name) const {
ColorPalette ColorPaletteEditorModel::getColorPalette(
const QString& name) const {
QList<mixxx::RgbColor> colors;
QMap<int, int> hotcueColorIndices;
for (int i = 0; i < rowCount(); i++) {
QStandardItem* pColorItem = item(i, 0);
QStandardItem* pHotcueIndexItem = item(i, 1);
mixxx::RgbColor::optional_t color = mixxx::RgbColor::fromQString(pColorItem->text());

auto* pHotcueIndexItem = toHotcueIndexListItem(item(i, 1));
if (!pHotcueIndexItem)
continue;
Swiftb0y marked this conversation as resolved.
Show resolved Hide resolved

mixxx::RgbColor::optional_t color =
mixxx::RgbColor::fromQString(pColorItem->text());

if (color) {
QList<int> hotcueIndexes = pHotcueIndexItem->getHotcueIndexList();
Swiftb0y marked this conversation as resolved.
Show resolved Hide resolved
colors << *color;

bool ok;
int hotcueIndex = pHotcueIndexItem->text().toInt(&ok);
if (ok) {
hotcueColorIndices.insert(hotcueIndex - 1, colors.size() - 1);
for (int index : qAsConst(hotcueIndexes)) {
hotcueColorIndices.insert(index - 1, colors.size() - 1);
}
}
}
// If we have a non consequitive list of hotcue indexes, indexes are shifted down
// due to the sorting nature of QMap. This is intended, this way we have a color for every hotcue.
return ColorPalette(name, colors, hotcueColorIndices.values());
Comment on lines 176 to 178
Copy link
Member Author

@Swiftb0y Swiftb0y Apr 3, 2021

Choose a reason for hiding this comment

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

I fear handling this silently results in confusion for the user. While we unfortunately don't have a "default color" we could insert into these empty positions, we should still test whether this shifting will occur and warn the user in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opinions? @Holzhaus

Copy link
Member

Choose a reason for hiding this comment

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

If you want to implement a warning icon/message that is displayed in the edit dialog, go ahead.

}

HotcueIndexListItem::HotcueIndexListItem(const QList<int>& hotcueList)
: QStandardItem(), m_hotcueIndexList(hotcueList) {
std::sort(m_hotcueIndexList.begin(), m_hotcueIndexList.end());
}
QVariant HotcueIndexListItem::data(int role) const {
switch (role) {
case Qt::DisplayRole:
case Qt::EditRole: {
QString serializedIndexStrings;
for (int i = 0; i < m_hotcueIndexList.size(); ++i) {
serializedIndexStrings += QString::number(m_hotcueIndexList.at(i));
if (i < m_hotcueIndexList.size() - 1) {
serializedIndexStrings += QStringLiteral(", ");
}
}
return QVariant(serializedIndexStrings);
break;
}
default:
return QStandardItem::data(role);
break;
}
}

void HotcueIndexListItem::setData(const QVariant& value, int role) {
switch (role) {
case Qt::EditRole: {
m_hotcueIndexList.clear();
QRegularExpressionMatchIterator regexResults =
hotcueListMatchingRegex.globalMatch(value.toString());
while (regexResults.hasNext()) {
QRegularExpressionMatch match = regexResults.next();
bool ok;
const int hotcueIndex = match.captured(1).toInt(&ok);
if (ok && !m_hotcueIndexList.contains(hotcueIndex)) {
m_hotcueIndexList.append(hotcueIndex);
}
}
std::sort(m_hotcueIndexList.begin(), m_hotcueIndexList.end());

emitDataChanged();
break;
}
default:
QStandardItem::setData(value, role);
break;
}
}
30 changes: 26 additions & 4 deletions src/preferences/colorpaletteeditormodel.h
@@ -1,23 +1,23 @@
#pragma once
#include <QStandardItem>
#include <QStandardItemModel>
#include <QVariant>

#include "util/color/colorpalette.h"

// Model that is used by the QTableView of the ColorPaletteEditor.
// Takes of displaying palette colors and provides a getter/setter for
// Takes care of displaying palette colors and provides a getter/setter for
// ColorPalette instances.
class ColorPaletteEditorModel : public QStandardItemModel {
Q_OBJECT
public:
static constexpr int kNoHotcueIndex = -1;

ColorPaletteEditorModel(QObject* parent = nullptr);

bool dropMimeData(const QMimeData* data, Qt::DropAction action, int row, int column, const QModelIndex& parent) override;
bool setData(const QModelIndex& index, const QVariant& value, int role = Qt::EditRole) override;

void setColor(int row, const QColor& color);
void appendRow(const QColor& color, int hotcueIndex = kNoHotcueIndex);
void appendRow(const QColor& color, const QList<int>& hotcueIndicies);
daschuer marked this conversation as resolved.
Show resolved Hide resolved

void setDirty(bool bDirty) {
if (m_bDirty == bDirty) {
Expand Down Expand Up @@ -46,3 +46,25 @@ class ColorPaletteEditorModel : public QStandardItemModel {
bool m_bEmpty;
bool m_bDirty;
};

class HotcueIndexListItem : public QStandardItem {
Copy link
Member Author

Choose a reason for hiding this comment

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

The entire purpose of creating this class is to shift the "(de-)serialization" logic out of the model into the item so the item is responsible for translating between the String input and the data actually supposed to be represented as the string.

public:
HotcueIndexListItem(const QList<int>& hotcueList = {});

void setData(const QVariant& value, int role = Qt::UserRole + 1) override;
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 still don't quite understand the purpose of roles. I get that setData and data are "generic" accessors to get data for different purposes (such as displaying the data itself, providing a tooltip description etc). Since I only intent to intercept the data for editing and displaying, I should be able to reuse those roles instead of making my own role, right? Or what exactly am I supposed to do with the custom role.

Copy link
Member

Choose a reason for hiding this comment

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

Right.

QVariant data(int role = Qt::UserRole + 1) const override;

int type() const override {
return QStandardItem::UserType;
};

const QList<int>& getHotcueIndexList() const {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const QList<int>& getHotcueIndexList() const {
const QList<int>& getHotcueIndexList() const {

QList is implicitly shared, should we really return a const reference here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. My guess was that explicitly constructing a QList with that reference at the call site when needed should still be pretty fast because of implicit sharing.

Copy link
Member

Choose a reason for hiding this comment

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

@daschuer @uklotzde opinions? I don't think really care and it can be changed in a subsequent PR you think it's needed. So no need to delay this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, references should be always const, except in a locals scope like fool[i].bar() and similar.

return m_hotcueIndexList;
}
void setHotcueIndexList(const QList<int>& list) {
m_hotcueIndexList = std::move(list);
}

private:
QList<int> m_hotcueIndexList;
};