From 8ffb21ef7d266b27c8094a044ce701bbc5ad53a9 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Thu, 25 Jun 2020 15:49:02 +0200 Subject: [PATCH 01/18] ColorPaletteEditor: enable multi cue auto-assign Make it possible to assign the same color to different hotcues by writing them as a comma-separated list into the table cell --- src/preferences/colorpaletteeditor.cpp | 2 +- src/preferences/colorpaletteeditormodel.cpp | 150 ++++++++++++++++---- src/preferences/colorpaletteeditormodel.h | 30 +++- 3 files changed, 146 insertions(+), 36 deletions(-) diff --git a/src/preferences/colorpaletteeditor.cpp b/src/preferences/colorpaletteeditor.cpp index 1d3408787f1..00fb53c63a4 100644 --- a/src/preferences/colorpaletteeditor.cpp +++ b/src/preferences/colorpaletteeditor.cpp @@ -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)); diff --git a/src/preferences/colorpaletteeditormodel.cpp b/src/preferences/colorpaletteeditormodel.cpp index d0f2d1a4255..3a5d54dcb4b 100644 --- a/src/preferences/colorpaletteeditormodel.cpp +++ b/src/preferences/colorpaletteeditormodel.cpp @@ -1,5 +1,13 @@ #include "preferences/colorpaletteeditormodel.h" +#include + +#include +#include +#include +#include +#include + #include "moc_colorpaletteeditormodel.cpp" namespace { @@ -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? + return nullptr; + } + return static_cast(from); +} + } // namespace ColorPaletteEditorModel::ColorPaletteEditorModel(QObject* parent) @@ -51,27 +69,45 @@ bool ColorPaletteEditorModel::dropMimeData(const QMimeData* data, Qt::DropAction } bool ColorPaletteEditorModel::setData(const QModelIndex& modelIndex, const QVariant& value, int role) { - 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 allHotcueIndicies; + allHotcueIndicies.reserve(rowCount()); + + for (int i = 0; i < rowCount(); ++i) { + auto* hotcueIndexListItem = toHotcueIndexListItem(item(i, 1)); - // 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) { + 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.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); } @@ -84,21 +120,18 @@ 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& 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{pColorItem, pHotcueIndexItem}); + QStandardItemModel::appendRow( + QList{pColorItem, pHotcueIndexItem}); } void ColorPaletteEditorModel::setColorPalette(const ColorPalette& palette) { @@ -106,7 +139,7 @@ void ColorPaletteEditorModel::setColorPalette(const ColorPalette& palette) { removeRows(0, rowCount()); // Make a map of hotcue indices - QMap hotcueColorIndicesMap; + QMultiMap hotcueColorIndicesMap; QList colorIndicesByHotcue = palette.getIndicesByHotcue(); for (int i = 0; i < colorIndicesByHotcue.size(); i++) { int colorIndex = colorIndicesByHotcue.at(i); @@ -115,27 +148,33 @@ 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 colorIndex = hotcueColorIndicesMap.values(i); appendRow(color, colorIndex); } setDirty(false); } -ColorPalette ColorPaletteEditorModel::getColorPalette(const QString& name) const { +ColorPalette ColorPaletteEditorModel::getColorPalette( + const QString& name) const { QList colors; QMap 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; + + mixxx::RgbColor::optional_t color = + mixxx::RgbColor::fromQString(pColorItem->text()); + if (color) { + QList hotcueIndexes = pHotcueIndexItem->getHotcueIndexList(); 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); } } } @@ -143,3 +182,52 @@ ColorPalette ColorPaletteEditorModel::getColorPalette(const QString& name) const // 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()); } + +HotcueIndexListItem::HotcueIndexListItem(const QList& 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; + } +} diff --git a/src/preferences/colorpaletteeditormodel.h b/src/preferences/colorpaletteeditormodel.h index b76c7a0053f..ef4050b3b79 100644 --- a/src/preferences/colorpaletteeditormodel.h +++ b/src/preferences/colorpaletteeditormodel.h @@ -1,23 +1,23 @@ #pragma once +#include #include +#include #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& hotcueIndicies); void setDirty(bool bDirty) { if (m_bDirty == bDirty) { @@ -46,3 +46,25 @@ class ColorPaletteEditorModel : public QStandardItemModel { bool m_bEmpty; bool m_bDirty; }; + +class HotcueIndexListItem : public QStandardItem { + public: + HotcueIndexListItem(const QList& hotcueList = {}); + + void setData(const QVariant& value, int role = Qt::UserRole + 1) override; + QVariant data(int role = Qt::UserRole + 1) const; + + int type() const { + return QStandardItem::UserType; + }; + + const QList& getHotcueIndexList() const { + return m_hotcueIndexList; + } + void setHotcueIndexList(const QList& list) { + m_hotcueIndexList = std::move(list); + } + + private: + QList m_hotcueIndexList; +}; From 8d53dcde409839ee55a90728c644e968b635e2f0 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Sat, 3 Apr 2021 16:52:30 +0200 Subject: [PATCH 02/18] ColorPaletteEditor: fix crash when autoassign column is empty --- src/preferences/colorpaletteeditormodel.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/preferences/colorpaletteeditormodel.cpp b/src/preferences/colorpaletteeditormodel.cpp index 3a5d54dcb4b..4a0487f5b85 100644 --- a/src/preferences/colorpaletteeditormodel.cpp +++ b/src/preferences/colorpaletteeditormodel.cpp @@ -95,7 +95,8 @@ bool ColorPaletteEditorModel::setData(const QModelIndex& modelIndex, const QVari const auto end = std::unique(allHotcueIndicies.begin(), allHotcueIndicies.end()); allHotcueIndicies.erase(end, allHotcueIndicies.end()); - const bool isOutsidePalette = allHotcueIndicies.last() > rowCount(); + const bool isOutsidePalette = !allHotcueIndicies.empty() && + allHotcueIndicies.last() > rowCount(); if (preDedupLen != allHotcueIndicies.length() || isOutsidePalette) { // checks failed! From 68b211566d31b213dfc01445d56514021ecb09bd Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Sat, 3 Apr 2021 17:09:35 +0200 Subject: [PATCH 03/18] HotcueIndexListItem: fix missing override warning --- src/preferences/colorpaletteeditormodel.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/preferences/colorpaletteeditormodel.h b/src/preferences/colorpaletteeditormodel.h index ef4050b3b79..131688cbdcb 100644 --- a/src/preferences/colorpaletteeditormodel.h +++ b/src/preferences/colorpaletteeditormodel.h @@ -52,9 +52,9 @@ class HotcueIndexListItem : public QStandardItem { HotcueIndexListItem(const QList& hotcueList = {}); void setData(const QVariant& value, int role = Qt::UserRole + 1) override; - QVariant data(int role = Qt::UserRole + 1) const; + QVariant data(int role = Qt::UserRole + 1) const override; - int type() const { + int type() const override { return QStandardItem::UserType; }; From 02de7df28aec3840ae1fa73b24f0f98d0cc843c3 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Tue, 6 Apr 2021 20:07:27 +0200 Subject: [PATCH 04/18] ColorPaletteEditorModel: fix loading index 0-based --- src/preferences/colorpaletteeditormodel.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/preferences/colorpaletteeditormodel.cpp b/src/preferences/colorpaletteeditormodel.cpp index 4a0487f5b85..e41554412e8 100644 --- a/src/preferences/colorpaletteeditormodel.cpp +++ b/src/preferences/colorpaletteeditormodel.cpp @@ -144,7 +144,7 @@ void ColorPaletteEditorModel::setColorPalette(const ColorPalette& palette) { QList colorIndicesByHotcue = palette.getIndicesByHotcue(); for (int i = 0; i < colorIndicesByHotcue.size(); i++) { int colorIndex = colorIndicesByHotcue.at(i); - hotcueColorIndicesMap.insert(colorIndex, i); + hotcueColorIndicesMap.insert(colorIndex, i + 1); } for (int i = 0; i < palette.size(); i++) { From 20df43cc1a6bcbb050c80d190c29ac4073cedc97 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Thu, 8 Apr 2021 16:54:04 +0200 Subject: [PATCH 05/18] ColorPaletteEditorModel: add range index assign Allow hotcue index assignments to consist of a mix of regular indicies and ranges of indicies. For example "1 - 3, 5" to express the list `[1, 2, 3, 5]`. --- src/preferences/colorpaletteeditormodel.cpp | 109 +++++++++++++++----- 1 file changed, 86 insertions(+), 23 deletions(-) diff --git a/src/preferences/colorpaletteeditormodel.cpp b/src/preferences/colorpaletteeditormodel.cpp index e41554412e8..93b8fa1e62b 100644 --- a/src/preferences/colorpaletteeditormodel.cpp +++ b/src/preferences/colorpaletteeditormodel.cpp @@ -12,14 +12,92 @@ namespace { +const auto groupSeparator = QStringLiteral(", "); +const auto rangeSeparator = QStringLiteral(" - "); +// when changing groupSeparator or rangeSeparator, rangeListMatchingRegex must +// be adjusted as well. +const QRegularExpression rangeListMatchingRegex( + QStringLiteral("(?:(\\d+)(?:\\s*-\\s*(\\d+))?)[\\s,]*")); + +/// parses a comma-separated list of positive ints and range if ints (eg `n - m`) +/// and returns a sorted list of all the ints described. +/// inverse counterpart of `stringifyRangeList` +QList parseRangeList(const QString& input) { + QList intList; + auto matchGroups = rangeListMatchingRegex.globalMatch(input); + while (matchGroups.hasNext()) { + const auto group = matchGroups.next(); + const QString rangeStart = group.captured(1); + const QString rangeEnd = group.captured(2); + bool startOk, endOk; + int startIndex = rangeStart.toInt(&startOk); + if (!startOk) { + continue; + } + int endIndex = rangeEnd.toInt(&endOk); + if (!endOk) { + endIndex = startIndex; + } + for (int currentIndex = startIndex; currentIndex <= endIndex; currentIndex++) { + intList.append(currentIndex); + } + } + + std::sort(intList.begin(), intList.end()); + const auto end = std::unique(intList.begin(), intList.end()); + intList.erase(end, intList.end()); + + return intList; +} + +/// take a list of positive integers and stringify them into a neat +/// user friendly representation (eg {1, 2, 3} => "1 - 3"). +/// inverse counterpart of `parseRangeList`. +/// rangeList must be sorted! +QString stringifyRangeList(QList rangeList) { + DEBUG_ASSERT(std::is_sorted(rangeList.cbegin(), rangeList.cend())); + + QString stringifiedRangeList; + + for (int i = 0; i < rangeList.size();) { + int rangeStartIndex = i; + int rangeStartValue = rangeList.at(i); + + while (i < rangeList.size() && rangeList.at(i) == rangeStartValue + (i - rangeStartIndex)) { + i++; + } + + int rangeEndIndex = i - 1; + + stringifiedRangeList += QString::number(rangeStartValue); + + switch (rangeEndIndex - rangeStartIndex) { + case 0: + // not a range + break; + case 1: + // treat ranges of (i..i+1) as separate groups: "i, i+1" + stringifiedRangeList += groupSeparator + QString::number(rangeList.at(rangeEndIndex)); + break; + default: + // range where the end is >=2 than the start + stringifiedRangeList += rangeSeparator + QString::number(rangeList.at(rangeEndIndex)); + break; + } + + if (i < rangeList.size()) { + stringifiedRangeList += groupSeparator; + } + } + return stringifiedRangeList; +} + QIcon toQIcon(const QColor& color) { QPixmap pixmap(50, 50); pixmap.fill(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? @@ -192,14 +270,7 @@ 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); + return QVariant(stringifyRangeList(m_hotcueIndexList)); break; } default: @@ -211,20 +282,12 @@ QVariant HotcueIndexListItem::data(int role) const { 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()); + QList newHotcueIndicies = parseRangeList(value.toString()); - emitDataChanged(); + if (m_hotcueIndexList != newHotcueIndicies) { + m_hotcueIndexList = newHotcueIndicies; + emitDataChanged(); + } break; } default: From 4e80be103986270487c9bf541a0bd7e499e84e03 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Thu, 8 Apr 2021 19:19:31 +0200 Subject: [PATCH 06/18] ColorPaletteEditorModel: change index dedup alg previously duplicate indicies were handled by rejecting the duplicate index. The new strategy removes those new indicies from the other cells, which results in behavior closer to what is expected by the user. --- src/preferences/colorpaletteeditormodel.cpp | 70 ++++++++++++--------- src/preferences/colorpaletteeditormodel.h | 4 +- 2 files changed, 44 insertions(+), 30 deletions(-) diff --git a/src/preferences/colorpaletteeditormodel.cpp b/src/preferences/colorpaletteeditormodel.cpp index 93b8fa1e62b..bf98c0c44a5 100644 --- a/src/preferences/colorpaletteeditormodel.cpp +++ b/src/preferences/colorpaletteeditormodel.cpp @@ -54,7 +54,7 @@ QList parseRangeList(const QString& input) { /// user friendly representation (eg {1, 2, 3} => "1 - 3"). /// inverse counterpart of `parseRangeList`. /// rangeList must be sorted! -QString stringifyRangeList(QList rangeList) { +QString stringifyRangeList(const QList& rangeList) { DEBUG_ASSERT(std::is_sorted(rangeList.cbegin(), rangeList.cend())); QString stringifiedRangeList; @@ -147,46 +147,39 @@ bool ColorPaletteEditorModel::dropMimeData(const QMimeData* data, Qt::DropAction } bool ColorPaletteEditorModel::setData(const QModelIndex& modelIndex, const QVariant& value, int role) { + setDirty(true); if (modelIndex.isValid() && modelIndex.column() == 1) { - const auto rollbackData = itemFromIndex(modelIndex)->data(role); - - // parse and validate in color-context const bool initialAttemptSuccessful = QStandardItemModel::setData(modelIndex, value, role); - QList allHotcueIndicies; - allHotcueIndicies.reserve(rowCount()); + const auto* hotcueIndexListItem = toHotcueIndexListItem(itemFromIndex(modelIndex)); + VERIFY_OR_DEBUG_ASSERT(hotcueIndexListItem) { + return false; + } + + auto hotcueIndexList = hotcueIndexListItem->getHotcueIndexList(); + + // make sure no index is outside of range + DEBUG_ASSERT(std::is_sorted(hotcueIndexList.cbegin(), hotcueIndexList.cend())); + auto endUpper = std::upper_bound( + hotcueIndexList.begin(), hotcueIndexList.end(), rowCount()); + hotcueIndexList.erase(endUpper, hotcueIndexList.end()); + auto endLower = std::upper_bound(hotcueIndexList.begin(), hotcueIndexList.end(), 0); + hotcueIndexList.erase(hotcueIndexList.begin(), endLower); for (int i = 0; i < rowCount(); ++i) { auto* hotcueIndexListItem = toHotcueIndexListItem(item(i, 1)); if (hotcueIndexListItem) { - allHotcueIndicies.append(hotcueIndexListItem->getHotcueIndexList()); + if (i == modelIndex.row()) { + hotcueIndexListItem->setHotcueIndexList(hotcueIndexList); + } else { + hotcueIndexListItem->removeIndicies(hotcueIndexList); + } } } - // 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; - } + return initialAttemptSuccessful; } - - setDirty(true); return QStandardItemModel::setData(modelIndex, value, role); } @@ -295,3 +288,22 @@ void HotcueIndexListItem::setData(const QVariant& value, int role) { break; } } + +void HotcueIndexListItem::removeIndicies(const QList& otherIndicies) { + DEBUG_ASSERT(std::is_sorted(otherIndicies.cbegin(), otherIndicies.cend())); + DEBUG_ASSERT(std::is_sorted(m_hotcueIndexList.cbegin(), m_hotcueIndexList.cend())); + + QList hotcueIndiciesWithOthersRemoved; + hotcueIndiciesWithOthersRemoved.reserve(m_hotcueIndexList.size()); + + std::set_difference(m_hotcueIndexList.cbegin(), + m_hotcueIndexList.cend(), + otherIndicies.cbegin(), + otherIndicies.cend(), + std::back_inserter(hotcueIndiciesWithOthersRemoved)); + + if (m_hotcueIndexList != hotcueIndiciesWithOthersRemoved) { + m_hotcueIndexList = hotcueIndiciesWithOthersRemoved; + emitDataChanged(); + } +} diff --git a/src/preferences/colorpaletteeditormodel.h b/src/preferences/colorpaletteeditormodel.h index 131688cbdcb..d57f7ace10e 100644 --- a/src/preferences/colorpaletteeditormodel.h +++ b/src/preferences/colorpaletteeditormodel.h @@ -62,9 +62,11 @@ class HotcueIndexListItem : public QStandardItem { return m_hotcueIndexList; } void setHotcueIndexList(const QList& list) { - m_hotcueIndexList = std::move(list); + m_hotcueIndexList = QList(list); } + void removeIndicies(const QList& otherIndicies); + private: QList m_hotcueIndexList; }; From 15e76e8e91622f6fb1ca5ddd2579843690f98929 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Thu, 8 Apr 2021 22:12:32 +0200 Subject: [PATCH 07/18] Apply suggestions from code review Co-authored-by: Jan Holthuis --- src/preferences/colorpaletteeditormodel.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/preferences/colorpaletteeditormodel.cpp b/src/preferences/colorpaletteeditormodel.cpp index bf98c0c44a5..1612048407f 100644 --- a/src/preferences/colorpaletteeditormodel.cpp +++ b/src/preferences/colorpaletteeditormodel.cpp @@ -12,11 +12,11 @@ namespace { -const auto groupSeparator = QStringLiteral(", "); -const auto rangeSeparator = QStringLiteral(" - "); +const QString kGroupSeparator = QStringLiteral(", "); +const QString kRangeSeparator = QStringLiteral(" - "); // when changing groupSeparator or rangeSeparator, rangeListMatchingRegex must // be adjusted as well. -const QRegularExpression rangeListMatchingRegex( +const QRegularExpression kRangeListMatchingRegex( QStringLiteral("(?:(\\d+)(?:\\s*-\\s*(\\d+))?)[\\s,]*")); /// parses a comma-separated list of positive ints and range if ints (eg `n - m`) @@ -24,7 +24,7 @@ const QRegularExpression rangeListMatchingRegex( /// inverse counterpart of `stringifyRangeList` QList parseRangeList(const QString& input) { QList intList; - auto matchGroups = rangeListMatchingRegex.globalMatch(input); + auto matchGroups = kRangeListMatchingRegex.globalMatch(input); while (matchGroups.hasNext()) { const auto group = matchGroups.next(); const QString rangeStart = group.captured(1); @@ -77,16 +77,16 @@ QString stringifyRangeList(const QList& rangeList) { break; case 1: // treat ranges of (i..i+1) as separate groups: "i, i+1" - stringifiedRangeList += groupSeparator + QString::number(rangeList.at(rangeEndIndex)); + stringifiedRangeList += kGroupSeparator + QString::number(rangeList.at(rangeEndIndex)); break; default: // range where the end is >=2 than the start - stringifiedRangeList += rangeSeparator + QString::number(rangeList.at(rangeEndIndex)); + stringifiedRangeList += kRangeSeparator + QString::number(rangeList.at(rangeEndIndex)); break; } if (i < rangeList.size()) { - stringifiedRangeList += groupSeparator; + stringifiedRangeList += kGroupSeparator; } } return stringifiedRangeList; From f2d9a3938f2bdfde2b47d2cbe8ca0b2b3dba4a44 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Thu, 8 Apr 2021 22:22:49 +0200 Subject: [PATCH 08/18] ColorPaletteEditorModel: fix hotcue index limit Fix hotcue indicies available for default color assignment being limited to number of colors in palette --- src/preferences/colorpaletteeditormodel.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/preferences/colorpaletteeditormodel.cpp b/src/preferences/colorpaletteeditormodel.cpp index 1612048407f..0ad70dd3fd9 100644 --- a/src/preferences/colorpaletteeditormodel.cpp +++ b/src/preferences/colorpaletteeditormodel.cpp @@ -8,6 +8,7 @@ #include #include +#include "engine/controls/cuecontrol.h" #include "moc_colorpaletteeditormodel.cpp" namespace { @@ -161,7 +162,7 @@ bool ColorPaletteEditorModel::setData(const QModelIndex& modelIndex, const QVari // make sure no index is outside of range DEBUG_ASSERT(std::is_sorted(hotcueIndexList.cbegin(), hotcueIndexList.cend())); auto endUpper = std::upper_bound( - hotcueIndexList.begin(), hotcueIndexList.end(), rowCount()); + hotcueIndexList.begin(), hotcueIndexList.end(), NUM_HOT_CUES); hotcueIndexList.erase(endUpper, hotcueIndexList.end()); auto endLower = std::upper_bound(hotcueIndexList.begin(), hotcueIndexList.end(), 0); hotcueIndexList.erase(hotcueIndexList.begin(), endLower); From c2f56686b7d9ba392384f36b045384698eab0af1 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Thu, 15 Apr 2021 22:21:09 +0200 Subject: [PATCH 09/18] ColorPaletteEditorModel move rangelist to own file --- src/preferences/colorpaletteeditormodel.cpp | 86 +------------------- src/util/rangelist.h | 90 +++++++++++++++++++++ 2 files changed, 93 insertions(+), 83 deletions(-) create mode 100644 src/util/rangelist.h diff --git a/src/preferences/colorpaletteeditormodel.cpp b/src/preferences/colorpaletteeditormodel.cpp index 0ad70dd3fd9..3e0ba2d794e 100644 --- a/src/preferences/colorpaletteeditormodel.cpp +++ b/src/preferences/colorpaletteeditormodel.cpp @@ -1,11 +1,11 @@ #include "preferences/colorpaletteeditormodel.h" #include +#include #include #include #include -#include #include #include "engine/controls/cuecontrol.h" @@ -13,86 +13,6 @@ namespace { -const QString kGroupSeparator = QStringLiteral(", "); -const QString kRangeSeparator = QStringLiteral(" - "); -// when changing groupSeparator or rangeSeparator, rangeListMatchingRegex must -// be adjusted as well. -const QRegularExpression kRangeListMatchingRegex( - QStringLiteral("(?:(\\d+)(?:\\s*-\\s*(\\d+))?)[\\s,]*")); - -/// parses a comma-separated list of positive ints and range if ints (eg `n - m`) -/// and returns a sorted list of all the ints described. -/// inverse counterpart of `stringifyRangeList` -QList parseRangeList(const QString& input) { - QList intList; - auto matchGroups = kRangeListMatchingRegex.globalMatch(input); - while (matchGroups.hasNext()) { - const auto group = matchGroups.next(); - const QString rangeStart = group.captured(1); - const QString rangeEnd = group.captured(2); - bool startOk, endOk; - int startIndex = rangeStart.toInt(&startOk); - if (!startOk) { - continue; - } - int endIndex = rangeEnd.toInt(&endOk); - if (!endOk) { - endIndex = startIndex; - } - for (int currentIndex = startIndex; currentIndex <= endIndex; currentIndex++) { - intList.append(currentIndex); - } - } - - std::sort(intList.begin(), intList.end()); - const auto end = std::unique(intList.begin(), intList.end()); - intList.erase(end, intList.end()); - - return intList; -} - -/// take a list of positive integers and stringify them into a neat -/// user friendly representation (eg {1, 2, 3} => "1 - 3"). -/// inverse counterpart of `parseRangeList`. -/// rangeList must be sorted! -QString stringifyRangeList(const QList& rangeList) { - DEBUG_ASSERT(std::is_sorted(rangeList.cbegin(), rangeList.cend())); - - QString stringifiedRangeList; - - for (int i = 0; i < rangeList.size();) { - int rangeStartIndex = i; - int rangeStartValue = rangeList.at(i); - - while (i < rangeList.size() && rangeList.at(i) == rangeStartValue + (i - rangeStartIndex)) { - i++; - } - - int rangeEndIndex = i - 1; - - stringifiedRangeList += QString::number(rangeStartValue); - - switch (rangeEndIndex - rangeStartIndex) { - case 0: - // not a range - break; - case 1: - // treat ranges of (i..i+1) as separate groups: "i, i+1" - stringifiedRangeList += kGroupSeparator + QString::number(rangeList.at(rangeEndIndex)); - break; - default: - // range where the end is >=2 than the start - stringifiedRangeList += kRangeSeparator + QString::number(rangeList.at(rangeEndIndex)); - break; - } - - if (i < rangeList.size()) { - stringifiedRangeList += kGroupSeparator; - } - } - return stringifiedRangeList; -} - QIcon toQIcon(const QColor& color) { QPixmap pixmap(50, 50); pixmap.fill(color); @@ -264,7 +184,7 @@ QVariant HotcueIndexListItem::data(int role) const { switch (role) { case Qt::DisplayRole: case Qt::EditRole: { - return QVariant(stringifyRangeList(m_hotcueIndexList)); + return QVariant(mixxx::stringifyRangeList(m_hotcueIndexList)); break; } default: @@ -276,7 +196,7 @@ QVariant HotcueIndexListItem::data(int role) const { void HotcueIndexListItem::setData(const QVariant& value, int role) { switch (role) { case Qt::EditRole: { - QList newHotcueIndicies = parseRangeList(value.toString()); + QList newHotcueIndicies = mixxx::parseRangeList(value.toString()); if (m_hotcueIndexList != newHotcueIndicies) { m_hotcueIndexList = newHotcueIndicies; diff --git a/src/util/rangelist.h b/src/util/rangelist.h new file mode 100644 index 00000000000..993dd9ca160 --- /dev/null +++ b/src/util/rangelist.h @@ -0,0 +1,90 @@ +#include + +#include +#include +#include +#include + +namespace mixxx { + +const QString kGroupSeparator = QStringLiteral(", "); +const QString kRangeSeparator = QStringLiteral(" - "); +// when changing groupSeparator or rangeSeparator, rangeListMatchingRegex must +// be adjusted as well. +const QRegularExpression kRangeListMatchingRegex( + QStringLiteral("(?:(\\d+)(?:\\s*-\\s*(\\d+))?)[\\s,]*")); + +/// parses a comma-separated list of positive ints and range if ints (eg `n - m`) +/// and returns a sorted list of all the ints described. +/// inverse counterpart of `stringifyRangeList` +QList parseRangeList(const QString& input) { + QList intList; + auto matchGroups = kRangeListMatchingRegex.globalMatch(input); + while (matchGroups.hasNext()) { + const auto group = matchGroups.next(); + const QString rangeStart = group.captured(1); + const QString rangeEnd = group.captured(2); + bool startOk, endOk; + int startIndex = rangeStart.toInt(&startOk); + if (!startOk) { + continue; + } + int endIndex = rangeEnd.toInt(&endOk); + if (!endOk) { + endIndex = startIndex; + } + for (int currentIndex = startIndex; currentIndex <= endIndex; currentIndex++) { + intList.append(currentIndex); + } + } + + std::sort(intList.begin(), intList.end()); + const auto end = std::unique(intList.begin(), intList.end()); + intList.erase(end, intList.end()); + + return intList; +} + +/// take a list of positive integers and stringify them into a neat +/// user friendly representation (eg {1, 2, 3} => "1 - 3"). +/// inverse counterpart of `parseRangeList`. +/// rangeList must be sorted! +QString stringifyRangeList(const QList& rangeList) { + DEBUG_ASSERT(std::is_sorted(rangeList.cbegin(), rangeList.cend())); + + QString stringifiedRangeList; + + for (int i = 0; i < rangeList.size();) { + int rangeStartIndex = i; + int rangeStartValue = rangeList.at(i); + + while (i < rangeList.size() && rangeList.at(i) == rangeStartValue + (i - rangeStartIndex)) { + i++; + } + + int rangeEndIndex = i - 1; + + stringifiedRangeList += QString::number(rangeStartValue); + + switch (rangeEndIndex - rangeStartIndex) { + case 0: + // not a range + break; + case 1: + // treat ranges of (i..i+1) as separate groups: "i, i+1" + stringifiedRangeList += kGroupSeparator + QString::number(rangeList.at(rangeEndIndex)); + break; + default: + // range where the end is >=2 than the start + stringifiedRangeList += kRangeSeparator + QString::number(rangeList.at(rangeEndIndex)); + break; + } + + if (i < rangeList.size()) { + stringifiedRangeList += kGroupSeparator; + } + } + return stringifiedRangeList; +} + +} // namespace mixxx From bc50748a40277eb22d4954c34490892c694bad40 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Thu, 15 Apr 2021 22:22:04 +0200 Subject: [PATCH 10/18] rangelist: add some unit tests --- CMakeLists.txt | 1 + src/test/rangelist_test.cpp | 74 +++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+) create mode 100644 src/test/rangelist_test.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index be67c340d09..e3e06238c55 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1513,6 +1513,7 @@ add_executable(mixxx-test src/test/portmidicontroller_test.cpp src/test/portmidienumeratortest.cpp src/test/queryutiltest.cpp + src/test/rangelist_test.cpp src/test/readaheadmanager_test.cpp src/test/replaygaintest.cpp src/test/rescalertest.cpp diff --git a/src/test/rangelist_test.cpp b/src/test/rangelist_test.cpp new file mode 100644 index 00000000000..334588cf3c1 --- /dev/null +++ b/src/test/rangelist_test.cpp @@ -0,0 +1,74 @@ +#include "util/rangelist.h" + +#include + +#include +#include + +#include "test/mixxxtest.h" + +void roundTripTestStr(const QString& in, const QString* out = nullptr) { + const QString roundTrip = mixxx::stringifyRangeList(mixxx::parseRangeList(in)); + if (out == nullptr) { + EXPECT_QSTRING_EQ(in, roundTrip); + } else { + EXPECT_QSTRING_EQ(*out, roundTrip); + } +} + +void roundTripTestList(const QList& in, const QList* out = nullptr) { + const QList roundTrip = mixxx::parseRangeList(mixxx::stringifyRangeList(in)); + if (out == nullptr) { + EXPECT_EQ(in, roundTrip); + } else { + EXPECT_EQ(*out, roundTrip); + } +} + +TEST(DisplayIntListTest, ListEmpty) { + roundTripTestList({}); + roundTripTestStr(""); +} + +TEST(DisplayIntListTest, smallContinousRangeIsSeparate) { + const QList list = mixxx::parseRangeList(QStringLiteral("1 - 2")); + EXPECT_EQ(list, QList({1, 2})); + EXPECT_QSTRING_EQ("1, 2", mixxx::stringifyRangeList(list)); +} + +TEST(DisplayIntListTest, whiteSpaceAreIgnored) { + EXPECT_EQ(QList({1, 2, 3, 4}), mixxx::parseRangeList(" 1,2 , 3, 4")); +} + +TEST(DisplayIntListTest, trailingCommaIsIgnored) { + EXPECT_EQ(QList({1}), mixxx::parseRangeList("1,")); +} +TEST(DisplayIntListTest, largeRangesAreExpanded) { + EXPECT_EQ(QList({1, 2, 3, 4, 5, 6, 7}), mixxx::parseRangeList("1 - 7")); +} + +TEST(DisplayIntListTest, duplicateValuesAreIgnored) { + EXPECT_EQ(QList({1, 2, 3}), mixxx::parseRangeList("1, 1, 1, 1, 2, 2, 3")); + + // currently only works from string to list, not the other way around + // EXPECT_QSTRING_EQ("1, 2, 3", mixxx::stringifyRangeList(QList({1, 1, 1, 1, 2, 2, 3}))); +} + +// TEST(DisplayIntListTest, negativeValuesAreIgnored) { +// EXPECT_EQ(QList({}), mixxx::parseRangeList("-1,-2,-3")); +// EXPECT_QSTRING_EQ("", mixxx::stringifyRangeList(QList({-1, -2, -3}))); +// } + +TEST(DisplayIntListTest, resultingListIsSortedAscending) { + const auto list = mixxx::parseRangeList("4, 2, 3, 1, 6, 5"); + EXPECT_TRUE(std::is_sorted(list.cbegin(), list.cend())); +} +TEST(DisplayIntListTest, consequitiveValuesAreRanges) { + EXPECT_QSTRING_EQ("1 - 4", mixxx::stringifyRangeList(QList({1, 2, 3, 4}))); +} +TEST(DisplayIntListTest, adjacentRangeAndLiteralGetsCollapsed) { + EXPECT_EQ(QList({1, 2, 3, 4, 5}), mixxx::parseRangeList("1, 2 - 4, 5")); +} +TEST(DisplayIntListTest, overLappingRangesGetUnionized) { + EXPECT_EQ(QList({1, 2, 3, 4}), mixxx::parseRangeList("1 - 3, 2 - 4")); +} From 08c842fdb649c0b302672e635e6e6f195d11e998 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Thu, 15 Apr 2021 22:26:17 +0200 Subject: [PATCH 11/18] ColorPaletteEditorModel remove unnecessary comment --- src/preferences/colorpaletteeditormodel.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/preferences/colorpaletteeditormodel.cpp b/src/preferences/colorpaletteeditormodel.cpp index 3e0ba2d794e..ed1046e68ec 100644 --- a/src/preferences/colorpaletteeditormodel.cpp +++ b/src/preferences/colorpaletteeditormodel.cpp @@ -21,7 +21,6 @@ QIcon toQIcon(const QColor& color) { HotcueIndexListItem* toHotcueIndexListItem(QStandardItem* from) { VERIFY_OR_DEBUG_ASSERT(from->type() == QStandardItem::UserType) { - // does std::optional make sense for pointers? return nullptr; } return static_cast(from); From 7c5b0de8e4062f8916d9200cc9c21439921604ca Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Sun, 18 Apr 2021 20:24:55 +0200 Subject: [PATCH 12/18] rangelist: separate declaration and definition --- CMakeLists.txt | 1 + src/util/rangelist.cpp | 87 ++++++++++++++++++++++++++++++++++++++++++ src/util/rangelist.h | 77 +------------------------------------ 3 files changed, 90 insertions(+), 75 deletions(-) create mode 100644 src/util/rangelist.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index e3e06238c55..44adaf96a09 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -864,6 +864,7 @@ add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL src/util/mac.cpp src/util/movinginterquartilemean.cpp src/util/performancetimer.cpp + src/util/rangelist.cpp src/util/readaheadsamplebuffer.cpp src/util/rlimit.cpp src/util/rotary.cpp diff --git a/src/util/rangelist.cpp b/src/util/rangelist.cpp new file mode 100644 index 00000000000..60499ea425a --- /dev/null +++ b/src/util/rangelist.cpp @@ -0,0 +1,87 @@ +#include "rangelist.h" + +#include + +#include +#include + +namespace { + +const QString kGroupSeparator = QStringLiteral(", "); +const QString kRangeSeparator = QStringLiteral(" - "); +// when changing groupSeparator or rangeSeparator, rangeListMatchingRegex must +// be adjusted as well. +const QRegularExpression kRangeListMatchingRegex( + QStringLiteral("(?:(\\d+)(?:\\s*-\\s*(\\d+))?)[\\s,]*")); + +} // namespace + +namespace mixxx { + +QList parseRangeList(const QString& input) { + QList intList; + auto matchGroups = kRangeListMatchingRegex.globalMatch(input); + while (matchGroups.hasNext()) { + const auto group = matchGroups.next(); + const QString rangeStart = group.captured(1); + const QString rangeEnd = group.captured(2); + bool startOk, endOk; + int startIndex = rangeStart.toInt(&startOk); + if (!startOk) { + continue; + } + int endIndex = rangeEnd.toInt(&endOk); + if (!endOk) { + endIndex = startIndex; + } + for (int currentIndex = startIndex; currentIndex <= endIndex; currentIndex++) { + intList.append(currentIndex); + } + } + + std::sort(intList.begin(), intList.end()); + const auto end = std::unique(intList.begin(), intList.end()); + intList.erase(end, intList.end()); + + return intList; +} + +QString stringifyRangeList(const QList& rangeList) { + DEBUG_ASSERT(std::is_sorted(rangeList.cbegin(), rangeList.cend())); + + QString stringifiedRangeList; + + for (int i = 0; i < rangeList.size();) { + int rangeStartIndex = i; + int rangeStartValue = rangeList.at(i); + + while (i < rangeList.size() && rangeList.at(i) == rangeStartValue + (i - rangeStartIndex)) { + i++; + } + + int rangeEndIndex = i - 1; + + stringifiedRangeList += QString::number(rangeStartValue); + + switch (rangeEndIndex - rangeStartIndex) { + case 0: + // not a range + break; + case 1: + // treat ranges of (i..i+1) as separate groups: "i, i+1" + stringifiedRangeList += kGroupSeparator + QString::number(rangeList.at(rangeEndIndex)); + break; + default: + // range where the end is >=2 than the start + stringifiedRangeList += kRangeSeparator + QString::number(rangeList.at(rangeEndIndex)); + break; + } + + if (i < rangeList.size()) { + stringifiedRangeList += kGroupSeparator; + } + } + return stringifiedRangeList; +} + +} // namespace mixxx diff --git a/src/util/rangelist.h b/src/util/rangelist.h index 993dd9ca160..b372a18a0ec 100644 --- a/src/util/rangelist.h +++ b/src/util/rangelist.h @@ -1,90 +1,17 @@ -#include - #include -#include #include -#include namespace mixxx { -const QString kGroupSeparator = QStringLiteral(", "); -const QString kRangeSeparator = QStringLiteral(" - "); -// when changing groupSeparator or rangeSeparator, rangeListMatchingRegex must -// be adjusted as well. -const QRegularExpression kRangeListMatchingRegex( - QStringLiteral("(?:(\\d+)(?:\\s*-\\s*(\\d+))?)[\\s,]*")); - /// parses a comma-separated list of positive ints and range if ints (eg `n - m`) /// and returns a sorted list of all the ints described. /// inverse counterpart of `stringifyRangeList` -QList parseRangeList(const QString& input) { - QList intList; - auto matchGroups = kRangeListMatchingRegex.globalMatch(input); - while (matchGroups.hasNext()) { - const auto group = matchGroups.next(); - const QString rangeStart = group.captured(1); - const QString rangeEnd = group.captured(2); - bool startOk, endOk; - int startIndex = rangeStart.toInt(&startOk); - if (!startOk) { - continue; - } - int endIndex = rangeEnd.toInt(&endOk); - if (!endOk) { - endIndex = startIndex; - } - for (int currentIndex = startIndex; currentIndex <= endIndex; currentIndex++) { - intList.append(currentIndex); - } - } - - std::sort(intList.begin(), intList.end()); - const auto end = std::unique(intList.begin(), intList.end()); - intList.erase(end, intList.end()); - - return intList; -} +QList parseRangeList(const QString& input); /// take a list of positive integers and stringify them into a neat /// user friendly representation (eg {1, 2, 3} => "1 - 3"). /// inverse counterpart of `parseRangeList`. /// rangeList must be sorted! -QString stringifyRangeList(const QList& rangeList) { - DEBUG_ASSERT(std::is_sorted(rangeList.cbegin(), rangeList.cend())); - - QString stringifiedRangeList; - - for (int i = 0; i < rangeList.size();) { - int rangeStartIndex = i; - int rangeStartValue = rangeList.at(i); - - while (i < rangeList.size() && rangeList.at(i) == rangeStartValue + (i - rangeStartIndex)) { - i++; - } - - int rangeEndIndex = i - 1; - - stringifiedRangeList += QString::number(rangeStartValue); - - switch (rangeEndIndex - rangeStartIndex) { - case 0: - // not a range - break; - case 1: - // treat ranges of (i..i+1) as separate groups: "i, i+1" - stringifiedRangeList += kGroupSeparator + QString::number(rangeList.at(rangeEndIndex)); - break; - default: - // range where the end is >=2 than the start - stringifiedRangeList += kRangeSeparator + QString::number(rangeList.at(rangeEndIndex)); - break; - } - - if (i < rangeList.size()) { - stringifiedRangeList += kGroupSeparator; - } - } - return stringifiedRangeList; -} +QString stringifyRangeList(const QList& rangeList); } // namespace mixxx From ffaa010a0f82d1c8be47dff427cc311e38c4e9fc Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Sun, 18 Apr 2021 20:30:45 +0200 Subject: [PATCH 13/18] ColorPaletteEditorModel: adhere to naming scheme variables of pointers should be prefixed with `p` --- src/preferences/colorpaletteeditormodel.cpp | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/preferences/colorpaletteeditormodel.cpp b/src/preferences/colorpaletteeditormodel.cpp index ed1046e68ec..f0c8d9a0969 100644 --- a/src/preferences/colorpaletteeditormodel.cpp +++ b/src/preferences/colorpaletteeditormodel.cpp @@ -19,11 +19,11 @@ QIcon toQIcon(const QColor& color) { return QIcon(pixmap); } -HotcueIndexListItem* toHotcueIndexListItem(QStandardItem* from) { - VERIFY_OR_DEBUG_ASSERT(from->type() == QStandardItem::UserType) { +HotcueIndexListItem* toHotcueIndexListItem(QStandardItem* pFrom) { + VERIFY_OR_DEBUG_ASSERT(pFrom->type() == QStandardItem::UserType) { return nullptr; } - return static_cast(from); + return static_cast(pFrom); } } // namespace @@ -71,12 +71,12 @@ bool ColorPaletteEditorModel::setData(const QModelIndex& modelIndex, const QVari if (modelIndex.isValid() && modelIndex.column() == 1) { const bool initialAttemptSuccessful = QStandardItemModel::setData(modelIndex, value, role); - const auto* hotcueIndexListItem = toHotcueIndexListItem(itemFromIndex(modelIndex)); - VERIFY_OR_DEBUG_ASSERT(hotcueIndexListItem) { + const auto* pHotcueIndexListItem = toHotcueIndexListItem(itemFromIndex(modelIndex)); + VERIFY_OR_DEBUG_ASSERT(pHotcueIndexListItem) { return false; } - auto hotcueIndexList = hotcueIndexListItem->getHotcueIndexList(); + auto hotcueIndexList = pHotcueIndexListItem->getHotcueIndexList(); // make sure no index is outside of range DEBUG_ASSERT(std::is_sorted(hotcueIndexList.cbegin(), hotcueIndexList.cend())); @@ -87,13 +87,13 @@ bool ColorPaletteEditorModel::setData(const QModelIndex& modelIndex, const QVari hotcueIndexList.erase(hotcueIndexList.begin(), endLower); for (int i = 0; i < rowCount(); ++i) { - auto* hotcueIndexListItem = toHotcueIndexListItem(item(i, 1)); + auto* pHotcueIndexListItem = toHotcueIndexListItem(item(i, 1)); - if (hotcueIndexListItem) { + if (pHotcueIndexListItem) { if (i == modelIndex.row()) { - hotcueIndexListItem->setHotcueIndexList(hotcueIndexList); + pHotcueIndexListItem->setHotcueIndexList(hotcueIndexList); } else { - hotcueIndexListItem->removeIndicies(hotcueIndexList); + pHotcueIndexListItem->removeIndicies(hotcueIndexList); } } } From 14b7560f8bca1d04abf01bb01f53ec3dadf596e8 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Sun, 18 Apr 2021 20:31:54 +0200 Subject: [PATCH 14/18] ColorPaletteEditorModel: reduce nesting --- src/preferences/colorpaletteeditormodel.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/preferences/colorpaletteeditormodel.cpp b/src/preferences/colorpaletteeditormodel.cpp index f0c8d9a0969..8c4d302d738 100644 --- a/src/preferences/colorpaletteeditormodel.cpp +++ b/src/preferences/colorpaletteeditormodel.cpp @@ -89,12 +89,14 @@ bool ColorPaletteEditorModel::setData(const QModelIndex& modelIndex, const QVari for (int i = 0; i < rowCount(); ++i) { auto* pHotcueIndexListItem = toHotcueIndexListItem(item(i, 1)); - if (pHotcueIndexListItem) { - if (i == modelIndex.row()) { - pHotcueIndexListItem->setHotcueIndexList(hotcueIndexList); - } else { - pHotcueIndexListItem->removeIndicies(hotcueIndexList); - } + if (pHotcueIndexListItem == nullptr) { + continue; + } + + if (i == modelIndex.row()) { + pHotcueIndexListItem->setHotcueIndexList(hotcueIndexList); + } else { + pHotcueIndexListItem->removeIndicies(hotcueIndexList); } } From d989d73709e1ea188768352d3675cf20b0901c06 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Sun, 18 Apr 2021 20:34:17 +0200 Subject: [PATCH 15/18] ColorPaletteEditorModel: fix if branch braces --- src/preferences/colorpaletteeditormodel.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/preferences/colorpaletteeditormodel.cpp b/src/preferences/colorpaletteeditormodel.cpp index 8c4d302d738..f726639c9c5 100644 --- a/src/preferences/colorpaletteeditormodel.cpp +++ b/src/preferences/colorpaletteeditormodel.cpp @@ -157,8 +157,9 @@ ColorPalette ColorPaletteEditorModel::getColorPalette( QStandardItem* pColorItem = item(i, 0); auto* pHotcueIndexItem = toHotcueIndexListItem(item(i, 1)); - if (!pHotcueIndexItem) + if (!pHotcueIndexItem) { continue; + } mixxx::RgbColor::optional_t color = mixxx::RgbColor::fromQString(pColorItem->text()); From f1aa399eb67f0c8ece97af253fd2d1fbb60a0da0 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Sun, 18 Apr 2021 20:39:08 +0200 Subject: [PATCH 16/18] ColorPaletteEditorModel: improve const-correctness --- src/preferences/colorpaletteeditormodel.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/preferences/colorpaletteeditormodel.cpp b/src/preferences/colorpaletteeditormodel.cpp index f726639c9c5..dfc01b3bf8f 100644 --- a/src/preferences/colorpaletteeditormodel.cpp +++ b/src/preferences/colorpaletteeditormodel.cpp @@ -156,7 +156,7 @@ ColorPalette ColorPaletteEditorModel::getColorPalette( for (int i = 0; i < rowCount(); i++) { QStandardItem* pColorItem = item(i, 0); - auto* pHotcueIndexItem = toHotcueIndexListItem(item(i, 1)); + const auto* pHotcueIndexItem = toHotcueIndexListItem(item(i, 1)); if (!pHotcueIndexItem) { continue; } @@ -165,10 +165,10 @@ ColorPalette ColorPaletteEditorModel::getColorPalette( mixxx::RgbColor::fromQString(pColorItem->text()); if (color) { - QList hotcueIndexes = pHotcueIndexItem->getHotcueIndexList(); + const QList hotcueIndexes = pHotcueIndexItem->getHotcueIndexList(); colors << *color; - for (int index : qAsConst(hotcueIndexes)) { + for (int index : hotcueIndexes) { hotcueColorIndices.insert(index - 1, colors.size() - 1); } } @@ -198,7 +198,7 @@ QVariant HotcueIndexListItem::data(int role) const { void HotcueIndexListItem::setData(const QVariant& value, int role) { switch (role) { case Qt::EditRole: { - QList newHotcueIndicies = mixxx::parseRangeList(value.toString()); + const QList newHotcueIndicies = mixxx::parseRangeList(value.toString()); if (m_hotcueIndexList != newHotcueIndicies) { m_hotcueIndexList = newHotcueIndicies; From b549298b3be4d2acb17332c92193704abe2fbe84 Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Sun, 18 Apr 2021 21:10:34 +0200 Subject: [PATCH 17/18] rangelist: add debug assert testing invalid lists enable detection of list containing duplicates --- src/test/rangelist_test.cpp | 3 --- src/util/rangelist.cpp | 7 +++++++ src/util/rangelist.h | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/test/rangelist_test.cpp b/src/test/rangelist_test.cpp index 334588cf3c1..462f8688294 100644 --- a/src/test/rangelist_test.cpp +++ b/src/test/rangelist_test.cpp @@ -49,9 +49,6 @@ TEST(DisplayIntListTest, largeRangesAreExpanded) { TEST(DisplayIntListTest, duplicateValuesAreIgnored) { EXPECT_EQ(QList({1, 2, 3}), mixxx::parseRangeList("1, 1, 1, 1, 2, 2, 3")); - - // currently only works from string to list, not the other way around - // EXPECT_QSTRING_EQ("1, 2, 3", mixxx::stringifyRangeList(QList({1, 1, 1, 1, 2, 2, 3}))); } // TEST(DisplayIntListTest, negativeValuesAreIgnored) { diff --git a/src/util/rangelist.cpp b/src/util/rangelist.cpp index 60499ea425a..40dc2a85f66 100644 --- a/src/util/rangelist.cpp +++ b/src/util/rangelist.cpp @@ -48,6 +48,13 @@ QList parseRangeList(const QString& input) { QString stringifyRangeList(const QList& rangeList) { DEBUG_ASSERT(std::is_sorted(rangeList.cbegin(), rangeList.cend())); + DEBUG_ASSERT([&rangeList]() { + // Immediately-invoked function expression + + // assumes list is sorted! + auto first_duplicate = std::adjacent_find(rangeList.cbegin(), rangeList.cend()); + return first_duplicate == rangeList.cend(); + }()); QString stringifiedRangeList; diff --git a/src/util/rangelist.h b/src/util/rangelist.h index b372a18a0ec..94e2ff07016 100644 --- a/src/util/rangelist.h +++ b/src/util/rangelist.h @@ -11,7 +11,7 @@ QList parseRangeList(const QString& input); /// take a list of positive integers and stringify them into a neat /// user friendly representation (eg {1, 2, 3} => "1 - 3"). /// inverse counterpart of `parseRangeList`. -/// rangeList must be sorted! +/// assumes rangeList is well-formed (sorted and not containing duplicates) QString stringifyRangeList(const QList& rangeList); } // namespace mixxx From cd95da5be1f8fcceba8a68549e4fa12e48cd898d Mon Sep 17 00:00:00 2001 From: Swiftb0y <12380386+Swiftb0y@users.noreply.github.com> Date: Sun, 18 Apr 2021 21:20:10 +0200 Subject: [PATCH 18/18] rangelist: debug assert on negative numbers --- src/test/rangelist_test.cpp | 5 ----- src/util/rangelist.cpp | 3 +++ src/util/rangelist.h | 3 ++- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/test/rangelist_test.cpp b/src/test/rangelist_test.cpp index 462f8688294..907054b28a9 100644 --- a/src/test/rangelist_test.cpp +++ b/src/test/rangelist_test.cpp @@ -51,11 +51,6 @@ TEST(DisplayIntListTest, duplicateValuesAreIgnored) { EXPECT_EQ(QList({1, 2, 3}), mixxx::parseRangeList("1, 1, 1, 1, 2, 2, 3")); } -// TEST(DisplayIntListTest, negativeValuesAreIgnored) { -// EXPECT_EQ(QList({}), mixxx::parseRangeList("-1,-2,-3")); -// EXPECT_QSTRING_EQ("", mixxx::stringifyRangeList(QList({-1, -2, -3}))); -// } - TEST(DisplayIntListTest, resultingListIsSortedAscending) { const auto list = mixxx::parseRangeList("4, 2, 3, 1, 6, 5"); EXPECT_TRUE(std::is_sorted(list.cbegin(), list.cend())); diff --git a/src/util/rangelist.cpp b/src/util/rangelist.cpp index 40dc2a85f66..23576b8e8de 100644 --- a/src/util/rangelist.cpp +++ b/src/util/rangelist.cpp @@ -55,6 +55,9 @@ QString stringifyRangeList(const QList& rangeList) { auto first_duplicate = std::adjacent_find(rangeList.cbegin(), rangeList.cend()); return first_duplicate == rangeList.cend(); }()); + DEBUG_ASSERT(std::all_of(rangeList.cbegin(), rangeList.cend(), [](int val) { + return val >= 0; + })); QString stringifiedRangeList; diff --git a/src/util/rangelist.h b/src/util/rangelist.h index 94e2ff07016..edd54dbdbbc 100644 --- a/src/util/rangelist.h +++ b/src/util/rangelist.h @@ -11,7 +11,8 @@ QList parseRangeList(const QString& input); /// take a list of positive integers and stringify them into a neat /// user friendly representation (eg {1, 2, 3} => "1 - 3"). /// inverse counterpart of `parseRangeList`. -/// assumes rangeList is well-formed (sorted and not containing duplicates) +/// assumes rangeList is well-formed +/// (sorted, not containing duplicates, and positive integers only) QString stringifyRangeList(const QList& rangeList); } // namespace mixxx