Add FILE line to Cuefile to include file path for each track #13365

presentformyfriends · 2024-06-13T03:04:10Z

This PR adds a FILE line to the Cuefile to show a filepath for each track (see issue for more information).

acolombier

Thanks for the contribution, as @fwcd suggested, it would be great to make that an option. To do so, you could add a boolean to EngineRecord (like m_bCueUsesFileAnnotation), and initialise it in the EngineRecord::updateFromPreferences, using m_pConfig->getValue().
You can use Qt Designer to edit src/preferences/dialog/dlgprefrecorddlg.ui and add the logic in src/preferences/dialog/dlgprefrecord.cpp to ahndle the preference edition. Happy to do that part if you would like me to.

ronso0 · 2024-06-13T10:42:32Z

Just a word on Qt Designer: that potentially rearranges the .ui files (for no reason) which produces a unreviewable diff.
If that happens please resort to editing the ui file manually in a text editor.

presentformyfriends · 2024-06-13T19:27:41Z

@ronso0 Thanks for the tip, I will pay attention to the .ui files order and will edit manually if need be

presentformyfriends · 2024-06-13T19:29:20Z

@acolombier Quick question, I just read in the Developer Guidelines to prefer merging over rebasing as rebasing apparently causes problems. I did not know this, and can't remember if I rebased or not. Is there a way for me to check if I rebased? Sorry if this is a dumb question. Thanks!

JoergAtGithub · 2024-06-13T19:35:34Z

Welcome at Mixxx!
As a first-time contributor we need you to sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

ronso0 · 2024-06-13T21:10:14Z

@presentformyfriends Merging is preferred becasue rebasing rewrites the commit history and may detach review comments (which could mean a lot / everything hd to be reviewwd again)

Though, for this minimal PR that's irrelevant IMO.

presentformyfriends · 2024-06-15T04:11:42Z

Thanks @JoergAtGithub just signed it!

Welcome at Mixxx! As a first-time contributor we need you to sign the

presentformyfriends · 2024-06-15T04:12:49Z

Got it, good to know!

Merging is preferred becasue rebasing rewrites the commit history and may detach review comments (which could mean a lot / everything hd to be reviewwd again)

Though, for this minimal PR that's irrelevant IMO.

Add boolean to EngineRecord and initialize it in updateFromPreferences Add checkbox to DlgPrefRecord to enable file annotation in CUE file Add logic to ensure checkbox is disabled by default, include tooltip

presentformyfriends · 2024-07-14T23:23:54Z

Pushed the rest, please let me know if it's ok or if I should change anything. FYI: I edited the .ui file with kDevelop, not Qt Designer.

EDIT: Build failed at pre-commit hook, I made some formatting changes and pushed those again.

presentformyfriends · 2024-07-15T17:22:49Z

Can anyone tell me why my commit keeps failing on clang-format? I ran pre-commit locally and everything passes. When I look at the diff it's formatting issues that I've already fixed? 🤷

Swiftb0y · 2024-07-15T18:11:06Z

when I look at the diff it's formatting issues that I've already fixed? 🤷

Are you sure? The lines that clang-format on CI complains about missing linebreaks in enginerecord.cpp:128 for example.

Also keep in mind that pre-commit only looks at the most recent commit, so if you have not run pre-commit since starting the PR, you might need to do a pre-commit run --from-ref upstream/main --to-ref HEAD so it runs on the entire PR.

acolombier

This is looking good, thanks for the PR!
Just some minor comments

src/preferences/dialog/dlgprefrecord.cpp

acolombier · 2024-07-15T18:25:39Z

src/engine/sidechain/enginerecord.cpp

@@ -249,6 +251,12 @@ void EngineRecord::writeCueLine() {
                            .arg(m_pCurrentTrack->getArtist())
                            .toUtf8());

+    if (m_bCueUsesFileAnnotation) {
+        m_cueFile.write(QString("    FILE \"%1\"\n")


I believe you might be able to use a QStringLiteral

For this one, fyi all the m_cueFile.write lines are existing code and use QString, I just added the if statement on line 254. I assumed whoever wrote that code used QString for a reason but idk.

QStringLiteral is a QString :) (doc)

The value of a QStringLiteral is that it will create the QString at compilte time, instead of instantiating at every function calls. This is a macro optimisation.

The TL;DR is: if you use a static string in your QString (like here with " FILE \"%1\"\n"), you can make it a QStringLiteral.
You are correct that the one above could also be using QStringLiteral tho! Feel free to update them.

Thanks for the explanation, I will update mine and the ones above. Just pushed new commit that should take care of this.

acolombier · 2024-07-15T18:27:11Z

src/preferences/dialog/dlgprefrecord.cpp

    CheckBoxRecordCueFile->setChecked(kDefaultCueEnabled);
+
+    // Sets 'Enable File Annotation in CUE file' checkbox value
+    CheckBoxUseCueFileAnnotation->setChecked(false);


Can you capture that default value in a kConstant like for line 217?

Sure, I did this originally and then scrapped it when I realized I could keep it as false. This builds fine on my end. I was curious why kDefaultCueEnabled was necessary in this case but I could not find it anywhere else in the code. So if you can explain why we capture the default in this way (as opposed to say getValueString), I would greatly appreciate it!

This helps with readability mainly - all defaults get listed at the top of the file, and this helps to understand why this false is being set to CheckBoxUseCueFileAnnotation.

That makes sense, thanks! Just pushed new commit that should take care of this.

src/preferences/dialog/dlgprefrecord.cpp

acolombier · 2024-07-15T18:31:47Z

src/preferences/dialog/dlgprefrecord.cpp

+void DlgPrefRecord::slotToggleCueEnabled() {
+    updateCueEnabled();
+}


I think these two methods are redundant. I would suggest keeping slotToggleCueEnabled and call it directly if you need to.

True, I've made this change but I'll wait til we resolve the others to push again.

Just pushed new commit for this.

presentformyfriends · 2024-07-17T21:32:55Z

you might need to do a pre-commit run --from-ref upstream/main --to-ref HEAD so it runs on the entire PR.

I ran this command but it still fails on clang-format and lists the same files:

fix utf-8 byte order marker..............................................Passed
check for case conflicts.................................................Passed
check json...........................................(no files to check)Skipped
check for merge conflicts................................................Passed
check xml............................................(no files to check)Skipped
check yaml...........................................(no files to check)Skipped
fix end of files.........................................................Passed
mixed line ending........................................................Passed
trim trailing whitespace.................................................Passed
don't commit to branch...................................................Passed
codespell................................................................Passed
eslint...............................................(no files to check)Skipped
clang-format.............................................................Failed
- hook id: clang-format
- files were modified by this hook

[INFO] First pass: Reformatting added/changed lines...
[INFO] Reformatting src/engine/sidechain/enginerecord.cpp [21-22, 40-40, 254-259]...
[INFO] Reformatting src/engine/sidechain/enginerecord.h [88-88]...
[INFO] Reformatting src/preferences/dialog/dlgprefrecord.cpp [79-81, 125-129, 155-155, 187-187, 191-192, 215-216, 218-220, 322-331, 458-461, 467-471]...
[INFO] Reformatting src/preferences/dialog/dlgprefrecord.h [35-37, 50-51]...
[INFO] Second pass: Breaking long added/changed lines...
[INFO] Reformatting src/engine/sidechain/enginerecord.cpp [40-40]...
[INFO] Reformatting src/preferences/dialog/dlgprefrecord.cpp [322-322]...

black................................................(no files to check)Skipped
flake8...............................................(no files to check)Skipped
shellcheck...........................................(no files to check)Skipped
markdownlint-cli2....................................(no files to check)Skipped
Validate GitHub Workflows............................(no files to check)Skipped
prettier.............................................(no files to check)Skipped
reformat QML files...................................(no files to check)Skipped
qsscheck.............................................(no files to check)Skipped
changelog............................................(no files to check)Skipped
qmllint..............................................(no files to check)Skipped
metainfo.............................................(no files to check)Skipped

Co-authored-by: Antoine Colombier <[email protected]>

daschuer · 2024-07-18T06:22:11Z

I ran this command but it still fails on clang-format and lists the same files:

Did you
git commit -a after checking all files?

daschuer · 2024-07-18T06:25:04Z

Alternative is to apply this patch:
https://github.com/mixxxdj/mixxx/actions/runs/9982325217/artifacts/1713542080

presentformyfriends · 2024-07-19T00:21:27Z

Did you git commit -a after checking all files?

No, just tried that and now I'm unable to even push or pull.

git push attempt:

To github.com:presentformyfriends/mixxx.git
 ! [rejected]              fixing_some_bug -> fixing_some_bug (fetch first)
error: failed to push some refs to 'github.com:presentformyfriends/mixxx.git'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

git pull attempt:

remote: Enumerating objects: 12, done.
remote: Counting objects: 100% (12/12), done.
remote: Compressing objects: 100% (7/7), done.
remote: Total 12 (delta 5), reused 8 (delta 5), pack-reused 0
Unpacking objects: 100% (12/12), 11.50 KiB | 3.83 MiB/s, done.
From github.com:presentformyfriends/mixxx
   d6a42bb989..7ae6cb7bb7  fixing_some_bug -> origin/fixing_some_bug
hint: You have divergent branches and need to specify how to reconcile them.
hint: You can do so by running one of the following commands sometime before
hint: your next pull:
hint: 
hint:   git config pull.rebase false  # merge (the default strategy)
hint:   git config pull.rebase true   # rebase
hint:   git config pull.ff only       # fast-forward only
hint: 
hint: You can replace "git config" with "git config --global" to set a default
hint: preference for all repositories. You can also pass --rebase, --no-rebase,
hint: or --ff-only on the command line to override the configured default per
hint: invocation.
fatal: Need to specify how to reconcile divergent branches.

No idea what to make of this despite their "hints"

EDIT: Is it ok to use 'git reset' in this case? I don't know git that well and don't want to screw up anyone else's code

This reverts commit 5a07163.

presentformyfriends · 2024-07-20T03:18:18Z

Ok should be fixed now, just did a git revert on most recent commit and then pulled. Please let me know if there is anything else I need to do to complete the PR, and thanks to all for the help.

daschuer · 2024-07-22T06:05:36Z

src/engine/sidechain/enginerecord.cpp

@@ -249,6 +251,12 @@ void EngineRecord::writeCueLine() {
                            .arg(m_pCurrentTrack->getArtist())
                            .toUtf8());

+    if (m_bCueUsesFileAnnotation) {
+        m_cueFile.write(QString("    FILE \"%1\"\n")


Suggested change

m_cueFile.write(QString(" FILE \"%1\"\n")

m_cueFile.write(QStringLiteral(" FILE \"%1\"\n")

Feel free to also fix other occurrence

Just pushed new commit that should take care of this. I updated the line you suggested as well as the ones above it. Thanks!

Resolve line length issues to pass clang-format hook in pre-commit. Change config key to snake_case. Remove redundant 'updateCueEnabled' function, call slotToggleCueEnabled directly instead.

This reverts commit 77cc748.

Resolve line length issues to pass clang-format hook in pre-commit. Change config key to snake_case. Remove redundant 'updateCueEnabled' function, call slotToggleCueEnabled directly instead.

Add FILE line to Cuefile to include file path for each track

84caae4

github-actions bot added the engine label Jun 13, 2024

acolombier reviewed Jun 13, 2024

View reviewed changes

presentformyfriends and others added 2 commits June 29, 2024 16:53

Merge branch 'mixxxdj:main' into fixing_some_bug

9b9daca

Add CUE file annotation feature logic and UI

c0fadc1

Add boolean to EngineRecord and initialize it in updateFromPreferences Add checkbox to DlgPrefRecord to enable file annotation in CUE file Add logic to ensure checkbox is disabled by default, include tooltip

github-actions bot added ui preferences labels Jul 14, 2024

Fix formatting issues as per pre-commit hook

d6a42bb

acolombier suggested changes Jul 15, 2024

View reviewed changes

presentformyfriends and others added 2 commits July 17, 2024 18:21

Update src/preferences/dialog/dlgprefrecord.cpp

827c956

Co-authored-by: Antoine Colombier <[email protected]>

Update src/preferences/dialog/dlgprefrecord.cpp

7ae6cb7

Co-authored-by: Antoine Colombier <[email protected]>

Add files modified by clang-format hook

5a07163

presentformyfriends added 2 commits July 19, 2024 19:18

Revert "Add files modified by clang-format hook"

b2327a0

This reverts commit 5a07163.

Merge divergent branches

c534739

daschuer requested changes Jul 22, 2024

View reviewed changes

presentformyfriends added 3 commits July 23, 2024 01:46

Fix line length, naming case, redundant function

77cc748

Resolve line length issues to pass clang-format hook in pre-commit. Change config key to snake_case. Remove redundant 'updateCueEnabled' function, call slotToggleCueEnabled directly instead.

Revert "Fix line length, naming case, redundant function"

213bfdb

This reverts commit 77cc748.

Fix line length, naming case, redundant function

4d6590f

Resolve line length issues to pass clang-format hook in pre-commit. Change config key to snake_case. Remove redundant 'updateCueEnabled' function, call slotToggleCueEnabled directly instead.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FILE line to Cuefile to include file path for each track #13365

Add FILE line to Cuefile to include file path for each track #13365

presentformyfriends commented Jun 13, 2024

acolombier left a comment •

edited

Loading

ronso0 commented Jun 13, 2024

presentformyfriends commented Jun 13, 2024

presentformyfriends commented Jun 13, 2024

JoergAtGithub commented Jun 13, 2024

ronso0 commented Jun 13, 2024

presentformyfriends commented Jun 15, 2024

presentformyfriends commented Jun 15, 2024

presentformyfriends commented Jul 14, 2024 •

edited

Loading

presentformyfriends commented Jul 15, 2024

Swiftb0y commented Jul 15, 2024 •

edited

Loading

acolombier left a comment

acolombier Jul 15, 2024

presentformyfriends Jul 17, 2024

acolombier Jul 20, 2024

presentformyfriends Jul 23, 2024 •

edited

Loading

acolombier Jul 15, 2024

presentformyfriends Jul 17, 2024

acolombier Jul 20, 2024

presentformyfriends Jul 23, 2024 •

edited

Loading

acolombier Jul 15, 2024

presentformyfriends Jul 17, 2024 •

edited

Loading

presentformyfriends Jul 23, 2024

presentformyfriends commented Jul 17, 2024 •

edited

Loading

daschuer commented Jul 18, 2024

daschuer commented Jul 18, 2024

presentformyfriends commented Jul 19, 2024 •

edited

Loading

presentformyfriends commented Jul 20, 2024

daschuer Jul 22, 2024

presentformyfriends Jul 23, 2024

	m_cueFile.write(QString(" FILE \"%1\"\n")
	m_cueFile.write(QStringLiteral(" FILE \"%1\"\n")

Add FILE line to Cuefile to include file path for each track #13365

Are you sure you want to change the base?

Add FILE line to Cuefile to include file path for each track #13365

Conversation

presentformyfriends commented Jun 13, 2024

acolombier left a comment • edited Loading

Choose a reason for hiding this comment

ronso0 commented Jun 13, 2024

presentformyfriends commented Jun 13, 2024

presentformyfriends commented Jun 13, 2024

JoergAtGithub commented Jun 13, 2024

ronso0 commented Jun 13, 2024

presentformyfriends commented Jun 15, 2024

presentformyfriends commented Jun 15, 2024

presentformyfriends commented Jul 14, 2024 • edited Loading

presentformyfriends commented Jul 15, 2024

Swiftb0y commented Jul 15, 2024 • edited Loading

acolombier left a 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

presentformyfriends Jul 23, 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

presentformyfriends Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

presentformyfriends Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

presentformyfriends commented Jul 17, 2024 • edited Loading

daschuer commented Jul 18, 2024

daschuer commented Jul 18, 2024

presentformyfriends commented Jul 19, 2024 • edited Loading

presentformyfriends commented Jul 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolombier left a comment •

edited

Loading

presentformyfriends commented Jul 14, 2024 •

edited

Loading

Swiftb0y commented Jul 15, 2024 •

edited

Loading

presentformyfriends Jul 23, 2024 •

edited

Loading

presentformyfriends Jul 23, 2024 •

edited

Loading

presentformyfriends Jul 17, 2024 •

edited

Loading

presentformyfriends commented Jul 17, 2024 •

edited

Loading

presentformyfriends commented Jul 19, 2024 •

edited

Loading