BaseTrackTableModel: Fix column header mapping #13782

daschuer · 2024-10-19T08:52:00Z

This finishes the work of @uklotzde in #13682 to fix the column header mapping in case using additional columns in his fork, not used in Mixxx.

I avoid a redundant initialization loop and in addition this improves the maintenance a lot by using a const kColumnPropertiesByEnum[] table to initialize thee header properties. This has the advantage that the compiler (tested with gcc) can check weather the table is complete.

If an entry is missing or not in consecutive order we will get:

src/library/columncache.cpp:168:92: sorry, unimplemented: non-trivial designated initializers not supported
  168 |         [ColumnCache::COLUMN_REKORDBOX_ANALYZE_PATH] = {REKORDBOX_ANALYZE_PATH, nullptr, 0}};
      |                                                                                            ^
src/library/columncache.cpp:169:50: error: static assertion failed
  169 | static_assert(std::size(kColumnPropertiesByEnum) == ColumnCache::NUM_COLUMNS);
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~

Note: This requires a Transifex roundtrip to move the header translations to the new translation scope, to the new common place for all.

Co-authored-by: Daniel Schürmann <[email protected]>

…n properties for all track tables. This simplifies the maintenance of track properties, and cannot go wrong, because th GCC compiler does not allow missing columns or resorting within the table.

JoergAtGithub · 2024-10-19T08:57:48Z

Note: This requires a Transifex roundtrip to move the header translations to the new translation scope, to the new common place for all.

We are past string freeze for 2.5 - Shouldn't this go into main instead?

daschuer · 2024-10-19T10:42:31Z

We may decide. I have no particular demand. It is at 2.5 because @uklotzde has picked that target.
Since we just have moved strings, my hope is that Transifex fixes i automatically. If it fails a manual fix would be trivial.This should be done in main anyway to not loose any already translated strings.

Swiftb0y · 2024-10-19T11:55:05Z

src/library/columncache.cpp

-        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));


could be a static assert.

Swiftb0y · 2024-10-19T11:59:32Z

src/library/columncache.cpp

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


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.

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.

With a pointer it works.

Swiftb0y · 2024-10-19T12:01:15Z

src/library/baseexternaltrackmodel.cpp

+            index.sibling(index.row(),
+                         fieldIndex(ColumnCache::COLUMN_LIBRARYTABLE_ARTIST))
+                    .data()


this deserves a utility function IMO.

Swiftb0y · 2024-10-19T12:01:59Z

src/library/baseexternaltrackmodel.cpp

+                    .toString()
+                    .toFloat();


Suggested change

.toString()

.toFloat();

.toFloat();

Swiftb0y · 2024-10-19T12:04:16Z

src/library/columncache.cpp

+        [ColumnCache::COLUMN_LIBRARYTABLE_ID] = LIBRARYTABLE_ID,
+        [ColumnCache::COLUMN_LIBRARYTABLE_ARTIST] = LIBRARYTABLE_ARTIST,


this is unfortunately not valid C++.

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

:-(

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, ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``

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.

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?

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).

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.

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

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

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

uklotzde · 2024-10-22T08:38:51Z

src/library/basesqltablemodel.cpp

            return m_tableColumns.size() + sourceTableIndex - 1;
        }
    }
    return tableIndex;
 }

+int BaseSqlTableModel::maxFieldIndex() const {
+    // Subtract one to remove the id column which is in both
+    return m_tableColumns.size() + (m_trackSource ? m_trackSource->maxFieldIndex() : 0);


That doesn't look correct to me.

The brittle max index = size minus one semantics are error prone.

maxFieldIndex() should better become endFieldIndex() which would be more consistent with how ranges are represented in C++.

daschuer · 2024-11-03T15:41:14Z

Done

uklotzde · 2024-11-03T22:09:14Z

No objections. I didn't review the additional changes in detail because they don't seem to affect my use case.

I'll update my fork after the changes have reached main.

Hint: This could have been achieved by merging smaller, independent PRs from different contributors. One at a time. Would cause less friction and frustration.

uklotzde and others added 7 commits October 15, 2024 00:04

BaseTrackTableModel: Fix column header mapping

b7fa49b

Co-authored-by: Daniel Schürmann <[email protected]>

Initalize ColumnCache::kColumnNameByEnum at compile time

1f977bf

prefer fieldIndex enum overload to look up column index

5c3692e

Use Column enum instead String

3f0d912

Improve comment for m_columnIndexByEnum

1bba2bc

Introduce a common table as kColumnNameByEnum[], that holds the colum…

77f9632

…n properties for all track tables. This simplifies the maintenance of track properties, and cannot go wrong, because th GCC compiler does not allow missing columns or resorting within the table.

Speed up control flow when initializing track table headers

6cf045a

github-actions bot added library rekordbox labels Oct 19, 2024

Swiftb0y reviewed Oct 19, 2024

View reviewed changes

daschuer added 11 commits October 22, 2024 08:13

Introduce maxFieldIndex()

b4ce99e

Hide array designated initializers from MSVC

c2f6b63

Use static_assert

a623af1

ColumCache use move semantic in c-tors

6c9700c

make insertColumnSortByEnum() private

fa8c704

Use more QStringLiteral

d4ea3c6

Move getFieldVariant and getFieldString to BaseTrackTableModel

077a2b7

Use getFieldVariant and getFieldString where possible

22b3442

Replace strings by enums

3940070

reduce dependency on ColumnCache::NUM_COLUMNS

31473f6

make kColumnPropertiesByEnum a constexpr

80d69e5

daschuer force-pushed the basetracktablemodel branch from 03fd6d9 to 80d69e5 Compare October 22, 2024 06:13

uklotzde reviewed Oct 22, 2024

View reviewed changes

replace maxFieldIndex by endFieldIndex pointing behind the maximum index

dcddaa9

daschuer force-pushed the basetracktablemodel branch from 98227f7 to dcddaa9 Compare October 27, 2024 20:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BaseTrackTableModel: Fix column header mapping #13782

BaseTrackTableModel: Fix column header mapping #13782

daschuer commented Oct 19, 2024

JoergAtGithub commented Oct 19, 2024

daschuer commented Oct 19, 2024

Swiftb0y Oct 19, 2024

Swiftb0y Oct 19, 2024

daschuer Oct 19, 2024

daschuer Oct 22, 2024

Swiftb0y Oct 19, 2024

Swiftb0y Oct 19, 2024

Swiftb0y Oct 19, 2024

daschuer Oct 19, 2024

daschuer Oct 19, 2024

Swiftb0y Oct 19, 2024

daschuer Oct 19, 2024

Swiftb0y Oct 20, 2024 •

edited

Loading

daschuer Oct 21, 2024

daschuer Oct 21, 2024

Swiftb0y Oct 21, 2024

uklotzde Oct 22, 2024

daschuer commented Nov 3, 2024

uklotzde commented Nov 3, 2024

		[ColumnCache::COLUMN_LIBRARYTABLE_ID] = LIBRARYTABLE_ID,
		[ColumnCache::COLUMN_LIBRARYTABLE_ARTIST] = LIBRARYTABLE_ARTIST,

BaseTrackTableModel: Fix column header mapping #13782

Are you sure you want to change the base?

BaseTrackTableModel: Fix column header mapping #13782

Conversation

daschuer commented Oct 19, 2024

JoergAtGithub commented Oct 19, 2024

daschuer commented Oct 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y Oct 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Nov 3, 2024

uklotzde commented Nov 3, 2024

Swiftb0y Oct 20, 2024 •

edited

Loading