Skip to content

Commit

Permalink
(fix) use 'played' track color also for Played delegate
Browse files Browse the repository at this point in the history
Add CheckboxDelegate base class, use it for BPMDelegate and play count delegate.
This allows moving redundant paintItem() code to CheckboxDelegate.
  • Loading branch information
ronso0 committed Aug 5, 2024
1 parent 22be583 commit d1cb360
Show file tree
Hide file tree
Showing 7 changed files with 96 additions and 133 deletions.
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -982,11 +982,11 @@ add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL
src/library/sidebarmodel.cpp
src/library/starrating.cpp
src/library/tabledelegates/bpmdelegate.cpp
src/library/tabledelegates/checkboxdelegate.cpp
src/library/tabledelegates/colordelegate.cpp
src/library/tabledelegates/coverartdelegate.cpp
src/library/tabledelegates/locationdelegate.cpp
src/library/tabledelegates/multilineeditdelegate.cpp
src/library/tabledelegates/playcountdelegate.cpp
src/library/tabledelegates/previewbuttondelegate.cpp
src/library/tabledelegates/stardelegate.cpp
src/library/tabledelegates/stareditor.cpp
Expand Down
4 changes: 2 additions & 2 deletions src/library/basetracktablemodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@
#include "library/dao/trackschema.h"
#include "library/starrating.h"
#include "library/tabledelegates/bpmdelegate.h"
#include "library/tabledelegates/checkboxdelegate.h"
#include "library/tabledelegates/colordelegate.h"
#include "library/tabledelegates/coverartdelegate.h"
#include "library/tabledelegates/locationdelegate.h"
#include "library/tabledelegates/multilineeditdelegate.h"
#include "library/tabledelegates/playcountdelegate.h"
#include "library/tabledelegates/previewbuttondelegate.h"
#include "library/tabledelegates/stardelegate.h"
#include "library/trackcollection.h"
Expand Down Expand Up @@ -482,7 +482,7 @@ QAbstractItemDelegate* BaseTrackTableModel::delegateForColumn(
} else if (index == fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_BPM)) {
return new BPMDelegate(pTableView);
} else if (index == fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_TIMESPLAYED)) {
return new PlayCountDelegate(pTableView);
return new CheckboxDelegate(pTableView, QStringLiteral("LibraryPlayedCheckbox"));
} else if (PlayerManager::numPreviewDecks() > 0 &&
index == fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_PREVIEW)) {
return new PreviewButtonDelegate(pTableView, index);
Expand Down
81 changes: 1 addition & 80 deletions src/library/tabledelegates/bpmdelegate.cpp
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
#include "library/tabledelegates/bpmdelegate.h"

#include <QCheckBox>
#include <QDoubleSpinBox>
#include <QItemEditorCreatorBase>
#include <QItemEditorFactory>
#include <QPainter>
#include <QTableView>

#include "moc_bpmdelegate.cpp"

Expand Down Expand Up @@ -34,14 +31,7 @@ class BpmEditorCreator : public QItemEditorCreatorBase {
};

BPMDelegate::BPMDelegate(QTableView* pTableView)
: TableItemDelegate(pTableView),
m_pCheckBox(new QCheckBox(m_pTableView)) {
m_pCheckBox->setObjectName("LibraryBPMButton");
// NOTE(rryan): Without ensurePolished the first render of the QTableView
// shows the checkbox unstyled. Not sure why -- but this fixes it.
m_pCheckBox->ensurePolished();
m_pCheckBox->hide();

: CheckboxDelegate(pTableView, QStringLiteral("LibraryBPMButton")) {
// Register a custom QItemEditorFactory to override the default
// QDoubleSpinBox editor.
m_pFactory = new QItemEditorFactory();
Expand All @@ -52,72 +42,3 @@ BPMDelegate::BPMDelegate(QTableView* pTableView)
#endif
setItemEditorFactory(m_pFactory);
}

BPMDelegate::~BPMDelegate() {
delete m_pFactory;
}

void BPMDelegate::paintItem(QPainter* painter,const QStyleOptionViewItem &option,
const QModelIndex& index) const {
// NOTE(rryan): Qt has a built-in limitation that we cannot style multiple
// CheckState indicators in the same QAbstractItemView. The CSS rule
// QTableView::indicator:checked applies to all columns with a
// CheckState. This is a big pain if we want to use CheckState roles on two
// columns (i.e. the played column and the BPM column) with different
// styling. We typically want a lock icon for the BPM check-state and a
// check-box for the times-played column and may want more in the future.
//
// This workaround creates a hidden QComboBox named LibraryBPMButton. We use
// the parent QTableView's QStyle with the hidden QComboBox as the source of
// style rules to draw a CE_ItemViewItem.
//
// Here's how you would typically style the LibraryBPMButton:
// #LibraryBPMButton::indicator:checked {
// image: url(:/images/library/ic_library_locked.svg);
// }
// #LibraryBPMButton::indicator:unchecked {
// image: url(:/images/library/ic_library_unlocked.svg);
// }

// Actually QAbstractTableModel::data(index, BackgroundRole) provides the
// correct custom background color (track color).
// Though, since Qt6 the above style rules would not apply for some reason,
// (see bug #11630) which can be fixed by also setting
// #LibraryBPMButton::item { border: 0px;}
// This however enables some default styles and clears the custom background
// color (track color), see bug #12355 ¯\_(ツ)_/¯ Qt is fun!
// Fix that by setting the bg color explicitly here.
paintItemBackground(painter, option, index);

QStyleOptionViewItem opt = option;
initStyleOption(&opt, index);

// The checkbox uses the QTableView's qss style, therefore it's not picking
// up the 'missing' or 'played' text color via ForegroundRole from
// BaseTrackTableModel::data().
// Enforce it with an explicit stylesheet. Note: the stylesheet persists so
// we need to reset it to normal/highlighted.
QColor textColor;
if (option.state & QStyle::State_Selected) {
textColor = option.palette.color(QPalette::Normal, QPalette::HighlightedText);
} else {
auto colorData = index.data(Qt::ForegroundRole);
if (colorData.canConvert<QColor>()) {
textColor = colorData.value<QColor>();
} else {
textColor = option.palette.color(QPalette::Normal, QPalette::Text);
}
}

if (textColor.isValid() && textColor != m_cachedTextColor) {
m_cachedTextColor = textColor;
m_pCheckBox->setStyleSheet(QStringLiteral(
"#LibraryBPMButton::item { color: %1; }")
.arg(textColor.name(QColor::HexRgb)));
}

QStyle* style = m_pTableView->style();
if (style != nullptr) {
style->drawControl(QStyle::CE_ItemViewItem, &opt, painter, m_pCheckBox);
}
}
13 changes: 2 additions & 11 deletions src/library/tabledelegates/bpmdelegate.h
Original file line number Diff line number Diff line change
@@ -1,21 +1,12 @@
#pragma once

#include "library/tabledelegates/tableitemdelegate.h"
#include "library/tabledelegates/checkboxdelegate.h"

class QCheckBox;

class BPMDelegate : public TableItemDelegate {
class BPMDelegate : public CheckboxDelegate {
Q_OBJECT
public:
explicit BPMDelegate(QTableView* pTableView);
virtual ~BPMDelegate();

void paintItem(QPainter* painter,
const QStyleOptionViewItem& option,
const QModelIndex& index) const override;

private:
QCheckBox* m_pCheckBox;
QItemEditorFactory* m_pFactory;
mutable QColor m_cachedTextColor;
};
85 changes: 85 additions & 0 deletions src/library/tabledelegates/checkboxdelegate.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
#include "library/tabledelegates/checkboxdelegate.h"

#include <QCheckBox>
#include <QPainter>
#include <QTableView>

#include "moc_checkboxdelegate.cpp"

CheckboxDelegate::CheckboxDelegate(QTableView* pTableView, const QString& checkboxName)
: TableItemDelegate(pTableView),
m_pCheckBox(new QCheckBox(m_pTableView)),
m_checkboxName(checkboxName) {
m_pCheckBox->setObjectName(checkboxName);
// NOTE(rryan): Without ensurePolished the first render of the QTableView
// shows the checkbox unstyled. Not sure why -- but this fixes it.
m_pCheckBox->ensurePolished();
m_pCheckBox->hide();
}

void CheckboxDelegate::paintItem(QPainter* painter,
const QStyleOptionViewItem& option,
const QModelIndex& index) const {
// NOTE(rryan): Qt has a built-in limitation that we cannot style multiple
// CheckState indicators in the same QAbstractItemView. The CSS rule
// QTableView::indicator:checked applies to all columns with a
// CheckState. This is a big pain if we want to use CheckState roles on two
// columns (i.e. the played column and the BPM column) with different
// styling. We typically want a lock icon for the BPM check-state and a
// check-box for the times-played column and may want more in the future.
//
// This workaround creates a hidden QComboBox named LibraryBPMButton. We use
// the parent QTableView's QStyle with the hidden QComboBox as the source of
// style rules to draw a CE_ItemViewItem.
//
// Here's how you would typically style the LibraryBPMButton:
// #LibraryBPMButton::indicator:checked {
// image: url(:/images/library/ic_library_locked.svg);
// }
// #LibraryBPMButton::indicator:unchecked {
// image: url(:/images/library/ic_library_unlocked.svg);
// }

// Actually QAbstractTableModel::data(index, BackgroundRole) provides the
// correct custom background color (track color).
// Though, since Qt6 the above style rules would not apply for some reason,
// (see bug #11630) which can be fixed by also setting
// #LibraryBPMButton::item { border: 0px;}
// This however enables some default styles and clears the custom background
// color (track color), see bug #12355 ¯\_(ツ)_/¯ Qt is fun!
// Fix that by setting the bg color explicitly here.
paintItemBackground(painter, option, index);

QStyleOptionViewItem opt = option;
initStyleOption(&opt, index);

// The checkbox uses the QTableView's qss style, therefore it's not picking
// up the 'missing' or 'played' text color via ForegroundRole from
// BaseTrackTableModel::data().
// Enforce it with an explicit stylesheet. Note: the stylesheet persists so
// we need to reset it to normal/highlighted.
QColor textColor;
if (option.state & QStyle::State_Selected) {
textColor = option.palette.color(QPalette::Normal, QPalette::HighlightedText);
} else {
auto colorData = index.data(Qt::ForegroundRole);
if (colorData.canConvert<QColor>()) {
textColor = colorData.value<QColor>();
} else {
textColor = option.palette.color(QPalette::Normal, QPalette::Text);
}
}

if (textColor.isValid() && textColor != m_cachedTextColor) {
m_cachedTextColor = textColor;
m_pCheckBox->setStyleSheet(QStringLiteral(
"#%1::item { color: %2; }")
.arg(m_checkboxName,
textColor.name(QColor::HexRgb)));
}

QStyle* style = m_pTableView->style();
if (style != nullptr) {
style->drawControl(QStyle::CE_ItemViewItem, &opt, painter, m_pCheckBox);
}
}
Original file line number Diff line number Diff line change
@@ -1,19 +1,21 @@
#pragma once

#include "library/tabledelegates/tableitemdelegate.h"
#include "util/parented_ptr.h"

class QCheckBox;

class PlayCountDelegate : public TableItemDelegate {
class CheckboxDelegate : public TableItemDelegate {
Q_OBJECT
public:
explicit PlayCountDelegate(QTableView* pTableView);
explicit CheckboxDelegate(QTableView* pTableView, const QString& checkboxName);

void paintItem(QPainter* painter,
const QStyleOptionViewItem& option,
const QModelIndex& index) const override;

private:
QTableView* m_pTableView;
QCheckBox* m_pCheckBox;
const QString m_checkboxName;
mutable QColor m_cachedTextColor;
};
36 changes: 0 additions & 36 deletions src/library/tabledelegates/playcountdelegate.cpp

This file was deleted.

0 comments on commit d1cb360

Please sign in to comment.