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

BaseTrackTableModel: Fix column header mapping #13782

Open
wants to merge 19 commits into
base: 2.5
Choose a base branch
from
Open

BaseTrackTableModel: Fix column header mapping #13782

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
b7fa49b
BaseTrackTableModel: Fix column header mapping
uklotzde Sep 21, 2024
1f977bf
Initalize ColumnCache::kColumnNameByEnum at compile time
daschuer Oct 15, 2024
5c3692e
prefer fieldIndex enum overload to look up column index
daschuer Oct 15, 2024
3f0d912
Use Column enum instead String
daschuer Oct 16, 2024
1bba2bc
Improve comment for m_columnIndexByEnum
daschuer Oct 16, 2024
77f9632
Introduce a common table as kColumnNameByEnum[], that holds the colum…
daschuer Oct 19, 2024
6cf045a
Speed up control flow when initializing track table headers
daschuer Oct 19, 2024
b4ce99e
Introduce maxFieldIndex()
daschuer Oct 20, 2024
c2f6b63
Hide array designated initializers from MSVC
daschuer Oct 19, 2024
a623af1
Use static_assert
daschuer Oct 19, 2024
6c9700c
ColumCache use move semantic in c-tors
daschuer Oct 20, 2024
fa8c704
make insertColumnSortByEnum() private
daschuer Oct 21, 2024
d4ea3c6
Use more QStringLiteral
daschuer Oct 20, 2024
077a2b7
Move getFieldVariant and getFieldString to BaseTrackTableModel
daschuer Oct 20, 2024
22b3442
Use getFieldVariant and getFieldString where possible
daschuer Oct 20, 2024
3940070
Replace strings by enums
daschuer Oct 20, 2024
31473f6
reduce dependency on ColumnCache::NUM_COLUMNS
daschuer Oct 22, 2024
80d69e5
make kColumnPropertiesByEnum a constexpr
daschuer Oct 22, 2024
dcddaa9
replace maxFieldIndex by endFieldIndex pointing behind the maximum index
daschuer Oct 22, 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
121 changes: 58 additions & 63 deletions src/library/columncache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,56 @@ const QString kSortNoCase = QStringLiteral("lower(%1)");
const QString kSortNoCaseLex = mixxx::DbConnection::collateLexicographically(
QStringLiteral("lower(%1)"));

const QString kColumnNameByEnum[] = {
[ColumnCache::COLUMN_LIBRARYTABLE_ID] = LIBRARYTABLE_ID,
[ColumnCache::COLUMN_LIBRARYTABLE_ARTIST] = LIBRARYTABLE_ARTIST,
Copy link
Member

Choose a reason for hiding this comment

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

this is unfortunately not valid C++.

Copy link
Member Author

Choose a reason for hiding this comment

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

For my understanding this is the C++ 20 feature "designated initializers".

Copy link
Member Author

Choose a reason for hiding this comment

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

:-(

Error: /home/runner/work/mixxx/mixxx/src/library/columncache.cpp:27:9: error: array designators are a C99 extension [-Werror,-Wc99-designator]
        [ColumnCache::COLUMN_LIBRARYTABLE_ID] = {LIBRARYTABLE_ID,
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
``

Copy link
Member

Choose a reason for hiding this comment

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

yup. Designated initializers are cool, but they deliberately work differently to allow for for example default arguments. I'm afraid there is no real way of achieving this trick in C++. You'll implicitly have to assume that the order of the list matches the enum order. The better strategy would instead simply be to not rely on the enum order at all though. Instead of having a bunch of globals arrays, consider having a single array of structs that bundles these together.

Copy link
Member Author

Choose a reason for hiding this comment

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

You'll implicitly have to assume that the order of the list matches the enum order.

A appreciate exactly this behavior a feature. This makes sure the list is complete and consistent.

I don't understand you proposal.

I would like to treat this as a compiler feature we can use like #pragma once, and just silence clazy.

What do you think?

Copy link
Member

@Swiftb0y Swiftb0y Oct 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would like to treat this as a compiler feature we can use like #pragma once, and just silence clazy.

I'm not sure its worth the trouble trying to opt into this extension and maintain it on every compiler. I'm also not sure what the semantics are when the enum suddenly gets different values (are entries in the array that are now empty unitialized, zero or default initialized?) I don't think we should mix C99 extensions with C++ code.

Have you instead considered putting this into a utility function with a switch-case? Performance is likely comparable and this time we don't have issues if the enum suddenly gets gaps and we can rely on -Wswitch for exhaustiveness (also we don't need NUM_COLUMNS anymore, which is always a hack).

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 also not sure what the semantics are when the enum suddenly gets different values

That is the great thing with this "feature". Compiling fails. So you cannot mess up the initalization you can do if this are single cals or an anonymous initalizer like in MSVC.

Copy link
Member Author

Choose a reason for hiding this comment

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

this time we don't have issues if the enum suddenly gets gaps

Than we have an initialized element. This can not happen currently.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I won't stress this for now then. Lets focus on the other stuff first.

[ColumnCache::COLUMN_LIBRARYTABLE_TITLE] = LIBRARYTABLE_TITLE,
[ColumnCache::COLUMN_LIBRARYTABLE_ALBUM] = LIBRARYTABLE_ALBUM,
[ColumnCache::COLUMN_LIBRARYTABLE_ALBUMARTIST] = LIBRARYTABLE_ALBUMARTIST,
[ColumnCache::COLUMN_LIBRARYTABLE_YEAR] = LIBRARYTABLE_YEAR,
[ColumnCache::COLUMN_LIBRARYTABLE_GENRE] = LIBRARYTABLE_GENRE,
[ColumnCache::COLUMN_LIBRARYTABLE_COMPOSER] = LIBRARYTABLE_COMPOSER,
[ColumnCache::COLUMN_LIBRARYTABLE_GROUPING] = LIBRARYTABLE_GROUPING,
[ColumnCache::COLUMN_LIBRARYTABLE_TRACKNUMBER] = LIBRARYTABLE_TRACKNUMBER,
[ColumnCache::COLUMN_LIBRARYTABLE_FILETYPE] = LIBRARYTABLE_FILETYPE,
[ColumnCache::COLUMN_LIBRARYTABLE_COMMENT] = LIBRARYTABLE_COMMENT,
[ColumnCache::COLUMN_LIBRARYTABLE_DURATION] = LIBRARYTABLE_DURATION,
[ColumnCache::COLUMN_LIBRARYTABLE_BITRATE] = LIBRARYTABLE_BITRATE,
[ColumnCache::COLUMN_LIBRARYTABLE_BPM] = LIBRARYTABLE_BPM,
[ColumnCache::COLUMN_LIBRARYTABLE_REPLAYGAIN] = LIBRARYTABLE_REPLAYGAIN,
[ColumnCache::COLUMN_LIBRARYTABLE_CUEPOINT] = LIBRARYTABLE_CUEPOINT,
[ColumnCache::COLUMN_LIBRARYTABLE_URL] = LIBRARYTABLE_URL,
[ColumnCache::COLUMN_LIBRARYTABLE_SAMPLERATE] = LIBRARYTABLE_SAMPLERATE,
[ColumnCache::COLUMN_LIBRARYTABLE_WAVESUMMARYHEX] = LIBRARYTABLE_WAVESUMMARYHEX,
[ColumnCache::COLUMN_LIBRARYTABLE_CHANNELS] = LIBRARYTABLE_CHANNELS,
[ColumnCache::COLUMN_LIBRARYTABLE_MIXXXDELETED] = LIBRARYTABLE_MIXXXDELETED,
[ColumnCache::COLUMN_LIBRARYTABLE_DATETIMEADDED] = LIBRARYTABLE_DATETIMEADDED,
[ColumnCache::COLUMN_LIBRARYTABLE_HEADERPARSED] = LIBRARYTABLE_HEADERPARSED,
[ColumnCache::COLUMN_LIBRARYTABLE_TIMESPLAYED] = LIBRARYTABLE_TIMESPLAYED,
[ColumnCache::COLUMN_LIBRARYTABLE_PLAYED] = LIBRARYTABLE_PLAYED,
[ColumnCache::COLUMN_LIBRARYTABLE_RATING] = LIBRARYTABLE_RATING,
[ColumnCache::COLUMN_LIBRARYTABLE_KEY] = LIBRARYTABLE_KEY,
[ColumnCache::COLUMN_LIBRARYTABLE_KEY_ID] = LIBRARYTABLE_KEY_ID,
[ColumnCache::COLUMN_LIBRARYTABLE_BPM_LOCK] = LIBRARYTABLE_BPM_LOCK,
[ColumnCache::COLUMN_LIBRARYTABLE_PREVIEW] = LIBRARYTABLE_PREVIEW,
[ColumnCache::COLUMN_LIBRARYTABLE_COLOR] = LIBRARYTABLE_COLOR,
[ColumnCache::COLUMN_LIBRARYTABLE_COVERART] = LIBRARYTABLE_COVERART,
[ColumnCache::COLUMN_LIBRARYTABLE_COVERART_SOURCE] = LIBRARYTABLE_COVERART_SOURCE,
[ColumnCache::COLUMN_LIBRARYTABLE_COVERART_TYPE] = LIBRARYTABLE_COVERART_TYPE,
[ColumnCache::COLUMN_LIBRARYTABLE_COVERART_LOCATION] = LIBRARYTABLE_COVERART_LOCATION,
[ColumnCache::COLUMN_LIBRARYTABLE_COVERART_COLOR] = LIBRARYTABLE_COVERART_COLOR,
[ColumnCache::COLUMN_LIBRARYTABLE_COVERART_DIGEST] = LIBRARYTABLE_COVERART_DIGEST,
[ColumnCache::COLUMN_LIBRARYTABLE_COVERART_HASH] = LIBRARYTABLE_COVERART_HASH,
[ColumnCache::COLUMN_LIBRARYTABLE_LAST_PLAYED_AT] = LIBRARYTABLE_LAST_PLAYED_AT,
[ColumnCache::COLUMN_TRACKLOCATIONSTABLE_LOCATION] = TRACKLOCATIONSTABLE_LOCATION,
[ColumnCache::COLUMN_TRACKLOCATIONSTABLE_FSDELETED] = TRACKLOCATIONSTABLE_FSDELETED,
[ColumnCache::COLUMN_PLAYLISTTRACKSTABLE_TRACKID] = PLAYLISTTRACKSTABLE_TRACKID,
[ColumnCache::COLUMN_PLAYLISTTRACKSTABLE_POSITION] = PLAYLISTTRACKSTABLE_POSITION,
[ColumnCache::COLUMN_PLAYLISTTRACKSTABLE_PLAYLISTID] = PLAYLISTTRACKSTABLE_PLAYLISTID,
[ColumnCache::COLUMN_PLAYLISTTRACKSTABLE_DATETIMEADDED] = PLAYLISTTRACKSTABLE_DATETIMEADDED,
[ColumnCache::COLUMN_REKORDBOX_ANALYZE_PATH] = REKORDBOX_ANALYZE_PATH};
static_assert(std::size(kColumnNameByEnum) == ColumnCache::NUM_COLUMNS);

} // namespace

ColumnCache::ColumnCache(const QStringList& columns) {
Expand All @@ -37,69 +87,9 @@ void ColumnCache::setColumns(const QStringList& columns) {
m_columnIndexByName[column] = i;
}

m_columnNameByEnum.clear();
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_ID, LIBRARYTABLE_ID);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_ARTIST, LIBRARYTABLE_ARTIST);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_TITLE, LIBRARYTABLE_TITLE);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_ALBUM, LIBRARYTABLE_ALBUM);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_ALBUMARTIST, LIBRARYTABLE_ALBUMARTIST);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_YEAR, LIBRARYTABLE_YEAR);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_GENRE, LIBRARYTABLE_GENRE);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_COMPOSER, LIBRARYTABLE_COMPOSER);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_GROUPING, LIBRARYTABLE_GROUPING);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_TRACKNUMBER, LIBRARYTABLE_TRACKNUMBER);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_FILETYPE, LIBRARYTABLE_FILETYPE);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_COMMENT, LIBRARYTABLE_COMMENT);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_DURATION, LIBRARYTABLE_DURATION);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_BITRATE, LIBRARYTABLE_BITRATE);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_BPM, LIBRARYTABLE_BPM);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_REPLAYGAIN, LIBRARYTABLE_REPLAYGAIN);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_CUEPOINT, LIBRARYTABLE_CUEPOINT);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_URL, LIBRARYTABLE_URL);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_SAMPLERATE, LIBRARYTABLE_SAMPLERATE);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_WAVESUMMARYHEX, LIBRARYTABLE_WAVESUMMARYHEX);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_CHANNELS, LIBRARYTABLE_CHANNELS);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_MIXXXDELETED, LIBRARYTABLE_MIXXXDELETED);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_DATETIMEADDED, LIBRARYTABLE_DATETIMEADDED);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_HEADERPARSED, LIBRARYTABLE_HEADERPARSED);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_TIMESPLAYED, LIBRARYTABLE_TIMESPLAYED);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_LAST_PLAYED_AT, LIBRARYTABLE_LAST_PLAYED_AT);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_PLAYED, LIBRARYTABLE_PLAYED);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_RATING, LIBRARYTABLE_RATING);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_KEY, LIBRARYTABLE_KEY);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_KEY_ID, LIBRARYTABLE_KEY_ID);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_BPM_LOCK, LIBRARYTABLE_BPM_LOCK);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_PREVIEW, LIBRARYTABLE_PREVIEW);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_COLOR, LIBRARYTABLE_COLOR);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_COVERART, LIBRARYTABLE_COVERART);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_COVERART_SOURCE, LIBRARYTABLE_COVERART_SOURCE);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_COVERART_TYPE, LIBRARYTABLE_COVERART_TYPE);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_COVERART_LOCATION, LIBRARYTABLE_COVERART_LOCATION);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_COVERART_COLOR, LIBRARYTABLE_COVERART_COLOR);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_COVERART_DIGEST, LIBRARYTABLE_COVERART_DIGEST);
insertColumnNameByEnum(COLUMN_LIBRARYTABLE_COVERART_HASH, LIBRARYTABLE_COVERART_HASH);

insertColumnNameByEnum(COLUMN_TRACKLOCATIONSTABLE_LOCATION, TRACKLOCATIONSTABLE_LOCATION);
insertColumnNameByEnum(COLUMN_TRACKLOCATIONSTABLE_FSDELETED, TRACKLOCATIONSTABLE_FSDELETED);

insertColumnNameByEnum(COLUMN_PLAYLISTTRACKSTABLE_TRACKID, PLAYLISTTRACKSTABLE_TRACKID);
insertColumnNameByEnum(COLUMN_PLAYLISTTRACKSTABLE_POSITION, PLAYLISTTRACKSTABLE_POSITION);
insertColumnNameByEnum(COLUMN_PLAYLISTTRACKSTABLE_PLAYLISTID, PLAYLISTTRACKSTABLE_PLAYLISTID);

insertColumnNameByEnum(
COLUMN_PLAYLISTTRACKSTABLE_DATETIMEADDED,
PLAYLISTTRACKSTABLE_DATETIMEADDED);

insertColumnNameByEnum(COLUMN_REKORDBOX_ANALYZE_PATH, REKORDBOX_ANALYZE_PATH);

for (int i = 0; i < NUM_COLUMNS; ++i) {
m_columnIndexByEnum[i] = -1;
}
for (auto it = m_columnNameByEnum.constBegin(); it != m_columnNameByEnum.constEnd(); ++it) {
DEBUG_ASSERT(it.key() >= 0);
DEBUG_ASSERT(it.key() < NUM_COLUMNS);
DEBUG_ASSERT(m_columnIndexByEnum[it.key()] == -1);
m_columnIndexByEnum[it.key()] = fieldIndex(it.value());
DEBUG_ASSERT(std::size(kColumnNameByEnum) == std::size(m_columnIndexByEnum));
Copy link
Member

Choose a reason for hiding this comment

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

could be a static assert.

for (std::size_t i = 0; i < std::size(kColumnNameByEnum); ++i) {
m_columnIndexByEnum[i] = fieldIndex(kColumnNameByEnum[i]);
}

m_columnSortByIndex.clear();
Expand Down Expand Up @@ -154,3 +144,8 @@ void ColumnCache::slotSetKeySortOrder(double notationValue) {
// Replace the existing sort order
m_columnSortByIndex[keyColumnIndex] = keySortSQL;
}

const QString& ColumnCache::columnName(Column column) const {
DEBUG_ASSERT(static_cast<std::size_t>(column) < std::size(kColumnNameByEnum));
return kColumnNameByEnum[column];
}
Copy link
Member

Choose a reason for hiding this comment

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

there may be some constexpr opportunity here by switching to a constexpr friendly type like QStringView and making kColumnNameByEnum constexpr accordingly by storing char16_ts instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Creating a QString form a QStringView is a deep copy.
kColumnNameByEnum uses LIBRARYTABLE_ID and friends a QStringLiteral which is basically a constexpr, but does not have the keyword because of MSVC limitations.
So using a QStringView here for the sake of becoming a constexpr, will introduce more deep copies later where now implicit sharing is used.

Copy link
Member Author

Choose a reason for hiding this comment

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

With a pointer it works.

13 changes: 1 addition & 12 deletions src/library/columncache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,7 @@ class ColumnCache : public QObject {
return m_columnIndexByName.value(columnName, -1);
}

inline QString columnName(Column column) const {
return m_columnNameByEnum[column];
}
const QString& columnName(Column column) const;

inline QString columnNameForFieldIndex(int index) const {
if (index < 0 || index >= m_columnsByIndex.size()) {
Expand All @@ -112,14 +110,6 @@ class ColumnCache : public QObject {
m_columnSortByIndex.insert(index, sortFormat);
}

void insertColumnNameByEnum(
Column column,
const QString& name) {
DEBUG_ASSERT(!m_columnNameByEnum.contains(column) ||
m_columnNameByEnum[column] == name);
m_columnNameByEnum.insert(column, name);
}

KeyUtils::KeyNotation keyNotation() const {
return KeyUtils::keyNotationFromNumericValue(
m_pKeyNotationCP->get());
Expand All @@ -132,7 +122,6 @@ class ColumnCache : public QObject {
QStringList m_columnsByIndex;
QMap<int, QString> m_columnSortByIndex;
QMap<QString, int> m_columnIndexByName;
QMap<Column, QString> m_columnNameByEnum;
// A mapping from column enum to logical index.
int m_columnIndexByEnum[NUM_COLUMNS];

Expand Down