diff options
author | Giuseppe D'Angelo <giuseppe.dangelo@kdab.com> | 2022-03-21 14:13:26 +0100 |
---|---|---|
committer | Giuseppe D'Angelo <giuseppe.dangelo@kdab.com> | 2022-03-23 23:06:23 +0100 |
commit | 6880b7c39b2c7474be1c9adcabbe0b7d6596822f (patch) | |
tree | 2ad5298062452dfdd3d87a07355ea003e46edddf | |
parent | af875e88f46a2947c313bd724ab9b10f3e7268df (diff) |
Itemviews: start fixing mixups of int/enum for Qt's item roles
A model is supposed to return a Qt::CheckState for a CheckStateRole,
and a Qt::Alignment for a Qt::TextAlignmentRole. This is what the
documentation says (and what makes sense), but unfortunately
Qt's default delegate expected a plain `int` instead.
This sometimes worked (via QVariant conversions, e.g. when using a plain
enum) and sometimes didn't (e.g. when using a flag type).
This is confusing for end-users (and type unsafe, killing the whole
point of using enums and flags in the first place).
Adding some automatic flags<->int conversions through QVariant is
frowned upon, so I don't want to go there. Instead, add some private
convenience functions that extract either the right type from a variant,
or try to extract an `int` and convert it to the expected type.
Use these from within itemviews code.
Change-Id: I44bee98c4a26a1ef6c3b2fa1b8de2edfee7aef32
Pick-to: 6.2 6.3
Fixes: QTBUG-75172
Task-number: QTBUG-74639
Reviewed-by: David Faure <david.faure@kdab.com>
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
-rw-r--r-- | src/corelib/itemmodels/qabstractitemmodel_p.h | 46 | ||||
-rw-r--r-- | src/widgets/itemviews/qabstractitemdelegate_p.h | 3 | ||||
-rw-r--r-- | src/widgets/itemviews/qheaderview.cpp | 7 | ||||
-rw-r--r-- | src/widgets/itemviews/qitemdelegate.cpp | 7 | ||||
-rw-r--r-- | src/widgets/itemviews/qstyleditemdelegate.cpp | 7 |
5 files changed, 61 insertions, 9 deletions
diff --git a/src/corelib/itemmodels/qabstractitemmodel_p.h b/src/corelib/itemmodels/qabstractitemmodel_p.h index e8f00975ed..edc5ad6116 100644 --- a/src/corelib/itemmodels/qabstractitemmodel_p.h +++ b/src/corelib/itemmodels/qabstractitemmodel_p.h @@ -157,6 +157,52 @@ public: }; Q_DECLARE_TYPEINFO(QAbstractItemModelPrivate::Change, Q_RELOCATABLE_TYPE); +namespace QtPrivate { + +/*! + \internal + This is a workaround for QTBUG-75172. + + Some predefined model roles are supposed to use certain enum/flag + types (e.g. fetching Qt::TextAlignmentRole is supposed to return a + variant containing a Qt::Alignment object). + + For historical reasons, a plain `int` was used sometimes. This is + surprising to end-users and also sloppy on Qt's part; users were + forced to use `int` rather than the correct datatype. + + This function tries both the "right" type and plain `int`, for a + given QVariant. This fixes the problem (using the correct datatype) + but also keeps compatibility with existing code using `int`. + + ### Qt 7: get rid of this. Always use the correct datatype. +*/ +template <typename T> +T legacyEnumValueFromModelData(const QVariant &data) +{ + static_assert(std::is_enum_v<T>); + if (data.userType() == qMetaTypeId<T>()) + return data.value<T>(); + else if (data.userType() == qMetaTypeId<int>()) + return T(data.toInt()); + + return T(); +} + +template <typename T> +T legacyFlagValueFromModelData(const QVariant &data) +{ + if (data.userType() == qMetaTypeId<T>()) + return data.value<T>(); + else if (data.userType() == qMetaTypeId<int>()) + return T::fromInt(data.toInt()); + + return T(); +} + +} // namespace QtPrivate + + QT_END_NAMESPACE #endif // QABSTRACTITEMMODEL_P_H diff --git a/src/widgets/itemviews/qabstractitemdelegate_p.h b/src/widgets/itemviews/qabstractitemdelegate_p.h index da76d31e8b..fc0aa9a73f 100644 --- a/src/widgets/itemviews/qabstractitemdelegate_p.h +++ b/src/widgets/itemviews/qabstractitemdelegate_p.h @@ -55,6 +55,9 @@ #include "qabstractitemdelegate.h" #include <private/qobject_p.h> +#include <qvariant.h> +#include <qmetatype.h> + QT_REQUIRE_CONFIG(itemviews); QT_BEGIN_NAMESPACE diff --git a/src/widgets/itemviews/qheaderview.cpp b/src/widgets/itemviews/qheaderview.cpp index 27866ecbb0..8fc0df84e2 100644 --- a/src/widgets/itemviews/qheaderview.cpp +++ b/src/widgets/itemviews/qheaderview.cpp @@ -60,6 +60,7 @@ #endif #include <private/qheaderview_p.h> #include <private/qabstractitemmodel_p.h> +#include <private/qabstractitemdelegate_p.h> #ifndef QT_NO_DATASTREAM #include <qdatastream.h> @@ -2936,9 +2937,9 @@ void QHeaderView::initStyleOptionForIndex(QStyleOptionHeader *option, int logica Qt::TextAlignmentRole); opt.section = logicalIndex; opt.state |= state; - opt.textAlignment = Qt::Alignment(textAlignment.isValid() - ? Qt::Alignment(textAlignment.toInt()) - : d->defaultAlignment); + opt.textAlignment = textAlignment.isValid() + ? QtPrivate::legacyFlagValueFromModelData<Qt::Alignment>(textAlignment) + : d->defaultAlignment; opt.iconAlignment = Qt::AlignVCenter; opt.text = d->model->headerData(logicalIndex, d->orientation, diff --git a/src/widgets/itemviews/qitemdelegate.cpp b/src/widgets/itemviews/qitemdelegate.cpp index 2640d7163d..e481dcae72 100644 --- a/src/widgets/itemviews/qitemdelegate.cpp +++ b/src/widgets/itemviews/qitemdelegate.cpp @@ -58,6 +58,7 @@ #include <qmetaobject.h> #include <qtextlayout.h> #include <private/qabstractitemdelegate_p.h> +#include <private/qabstractitemmodel_p.h> #include <private/qtextengine_p.h> #include <qdebug.h> #include <qlocale.h> @@ -432,7 +433,7 @@ void QItemDelegate::paint(QPainter *painter, Qt::CheckState checkState = Qt::Unchecked; value = index.data(Qt::CheckStateRole); if (value.isValid()) { - checkState = static_cast<Qt::CheckState>(value.toInt()); + checkState = QtPrivate::legacyEnumValueFromModelData<Qt::CheckState>(value); checkRect = doCheck(opt, opt.rect, value); } @@ -1182,7 +1183,7 @@ bool QItemDelegate::editorEvent(QEvent *event, return false; } - Qt::CheckState state = static_cast<Qt::CheckState>(value.toInt()); + Qt::CheckState state = QtPrivate::legacyEnumValueFromModelData<Qt::CheckState>(value); if (flags & Qt::ItemIsUserTristate) state = ((Qt::CheckState)((state + 1) % 3)); else @@ -1209,7 +1210,7 @@ QStyleOptionViewItem QItemDelegate::setOptions(const QModelIndex &index, // set text alignment value = index.data(Qt::TextAlignmentRole); if (value.isValid()) - opt.displayAlignment = Qt::Alignment(value.toInt()); + opt.displayAlignment = QtPrivate::legacyFlagValueFromModelData<Qt::Alignment>(value); // set foreground brush value = index.data(Qt::ForegroundRole); diff --git a/src/widgets/itemviews/qstyleditemdelegate.cpp b/src/widgets/itemviews/qstyleditemdelegate.cpp index ddf706c0e6..6379031f6a 100644 --- a/src/widgets/itemviews/qstyleditemdelegate.cpp +++ b/src/widgets/itemviews/qstyleditemdelegate.cpp @@ -66,6 +66,7 @@ #include <qmetaobject.h> #include <qtextlayout.h> #include <private/qabstractitemdelegate_p.h> +#include <private/qabstractitemmodel_p.h> #include <private/qtextengine_p.h> #include <private/qlayoutengine_p.h> #include <qdebug.h> @@ -302,7 +303,7 @@ void QStyledItemDelegate::initStyleOption(QStyleOptionViewItem *option, value = modelRoleDataSpan.dataForRole(Qt::TextAlignmentRole); if (value->isValid() && !value->isNull()) - option->displayAlignment = Qt::Alignment(value->toInt()); + option->displayAlignment = QtPrivate::legacyFlagValueFromModelData<Qt::Alignment>(*value); value = modelRoleDataSpan.dataForRole(Qt::ForegroundRole); if (value->canConvert<QBrush>()) @@ -311,7 +312,7 @@ void QStyledItemDelegate::initStyleOption(QStyleOptionViewItem *option, value = modelRoleDataSpan.dataForRole(Qt::CheckStateRole); if (value->isValid() && !value->isNull()) { option->features |= QStyleOptionViewItem::HasCheckIndicator; - option->checkState = static_cast<Qt::CheckState>(value->toInt()); + option->checkState = QtPrivate::legacyEnumValueFromModelData<Qt::CheckState>(*value); } value = modelRoleDataSpan.dataForRole(Qt::DecorationRole); @@ -633,7 +634,7 @@ bool QStyledItemDelegate::editorEvent(QEvent *event, return false; } - Qt::CheckState state = static_cast<Qt::CheckState>(value.toInt()); + Qt::CheckState state = QtPrivate::legacyEnumValueFromModelData<Qt::CheckState>(value); if (flags & Qt::ItemIsUserTristate) state = ((Qt::CheckState)((state + 1) % 3)); else |