Fix cue points being assigned invalid value of -1.0

[ywwg]

Fixes two issues in cue point creation:

• cue point positions are stored as doubles, not ints, so they should be loaded that way from the db.
• in the conversion to engine sample position, avoid creating invalid -1.0 values.

Fixes #10993

[ywwg]

Sister fix of #10998

[ywwg]

verified this fixes the issue with my problem tracks. In very very very rare cases a user might have a cue point set at exactly -1.0 frame, in which case they should right click the track and select Reset / Cue Point and when they create it it will work correctly.

[daschuer]

Great, that looks like a good minimum impact fix.

I have just tested to create a Cue point at a fractional position but without success.
Even though I have such fractional Cue points already in my library. Did we re-add rounding to a full frame when storing cue points at one point?

[daschuer]

Got it: The column is set to int and it is read back as int

mixxx/src/library/dao/cuedao.cpp

Line 46 in 074c938

row.value(row.indexOf("position")).toInt());

The maincue is written and read as double:

mixxx/src/library/dao/trackdao.cpp

Line 1170 in 074c938

record.value(column).toDouble()));

mixxx/src/library/dao/trackdao.cpp

Line 563 in 074c938

pTrackLibraryQuery->bindValue(":cuepoint",

We need to check how a shifted cue is handled in a database round-trip.

[Holzhaus]

In very very very rare cases a user might have a cue point set at exactly -1.0 frame

You mean at -0.5 frame = -1.0 sample, right?

[daschuer]

The near -1 cue must not become -1 after reading back from the library.

[Holzhaus]

Code looks good, did not have time to test though.

[ywwg]

Cue points are currently written as double and read as int. This PR fixes that so they are read back as doubles.

mixxx/src/library/dao/cuedao.cpp

Line 172 in b071d6e

query.bindValue(":position", cue->getPosition().toEngineSamplePosMaybeInvalid());

[ywwg]

The near -1 cue must not become -1 after reading back from the library.

The toInt was causing this. By changing it to doubles, this is fixed

[Holzhaus]

Cue points are currently written as double and read as int. This PR fixes that so they are read back as doubles.

mixxx/src/library/dao/cuedao.cpp

Line 172 in b071d6e

query.bindValue(":position", cue->getPosition().toEngineSamplePosMaybeInvalid());

The DB schema says the column is an int. If we really want to store fractional cue positions in the DB, we need a schema migration IMHO. I know it works without it, because SQLite does not enforce column types by default, but marking the column as int and then storing a different type is really confusing IMHO.

[ywwg]

yeah I'm ok adding a db schema migration. I think that's out of scope for 2.3 though... I'd prefer to let the schema be broken in that version since it works fine, rather than surprise users with a schema difference in a "stable" version. Is that ok?

[daschuer]

LGTM. The question of the schema migration is orthogonal to this PR, because we already store frictional cues in the in column. Here we only read and fix the wrong int rounding.
@Holzhaus: merge?

[daschuer]

We have also the question whether a default is appropriated, moved from:
#10998 (comment)
#10998 (comment)

      CREATE TABLE IF NOT EXISTS cues (
        ...
        position INTEGER DEFAULT -1 NOT NULL,
        length INTEGER DEFAULT 0 NOT NULL,
        ...

I think no, because we will never add a cue with an invalid position, or do I miss something?

[daschuer]

Reading the sqlite docs: https://www.sqlite.org/flextypegood.html
We may argue that the Type "INTEGER" is just correct, because we want to store the "INTEGER" -1 and the "REAL" double position. So we may ditch the schema migration completely.
Since this is still a source of confusion we should place a big warning at here explaining our decision, with a reference to the sqlite manual, here: https://github.com/mixxxdj/mixxx/blob/main/res/schema.xml#L83
@ywwg can you do this in in this PR?

The other issue that should also be fixed in this PR that the "length" column should be also read as double.
This can be also done in this PR.

[daschuer]

Please add a comment at schema.xml and apply the fix to "length" as well.

[daschuer]

A good reason for the column migration would be the move from our stereo sample based format to an frame based format. Introducing another lstalkec egacy column for that would be odd. We have also discussed to use sample rate independent format ... Not sure about that though.

[ywwg]

sqlite does not support changing column type:
https://sqlite.org/lang_altertable.html
https://stackoverflow.com/questions/2083543/modify-a-columns-type-in-sqlite3

so this migration would require:

1. Create a new clean table with the correct types
2. Copy all of the data from the old track table to the new one
3. delete the old track table
4. (hope we don't crash here)
5. rename the new track table to the proper track table name.

Is that really worth the trouble? I think I would prefer to add annotations to the code. Since SQLite is well known for not having strict types, it doesn't seem like a big deal that this is wrong.

[ywwg]

With the update from 2.3 this now just includes the comment in the schema about double values, and the check to ensure cuepoints are not created at exactly -1.0

[daschuer]

LGTM, Thank you.

Since you have debased anyway, can you rebase this to main and get rid of the not working commit?

[daschuer]

Thank you.