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

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
84caae4
Add FILE line to Cuefile to include file path for each track
presentformyfriends Jun 13, 2024
9b9daca
Merge branch 'mixxxdj:main' into fixing_some_bug
presentformyfriends Jun 29, 2024
c0fadc1
Add CUE file annotation feature logic and UI
presentformyfriends Jul 14, 2024
d6a42bb
Fix formatting issues as per pre-commit hook
presentformyfriends Jul 15, 2024
827c956
Update src/preferences/dialog/dlgprefrecord.cpp
presentformyfriends Jul 17, 2024
7ae6cb7
Update src/preferences/dialog/dlgprefrecord.cpp
presentformyfriends Jul 17, 2024
5a07163
Add files modified by clang-format hook
presentformyfriends Jul 19, 2024
b2327a0
Revert "Add files modified by clang-format hook"
presentformyfriends Jul 19, 2024
c534739
Merge divergent branches
presentformyfriends Jul 19, 2024
77cc748
Fix line length, naming case, redundant function
presentformyfriends Jul 23, 2024
213bfdb
Revert "Fix line length, naming case, redundant function"
presentformyfriends Jul 23, 2024
4d6590f
Fix line length, naming case, redundant function
presentformyfriends Jul 23, 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
10 changes: 9 additions & 1 deletion src/engine/sidechain/enginerecord.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ EngineRecord::EngineRecord(UserSettingsPointer pConfig)
m_recordedDuration(0),
m_iMetaDataLife(0),
m_cueTrack(0),
m_bCueIsEnabled(false) {
m_bCueIsEnabled(false),
m_bCueUsesFileAnnotation(false) {
m_pRecReady = new ControlProxy(RECORDING_PREF_KEY, "status", this);
m_sampleRate = mixxx::audio::SampleRate::fromDouble(m_sampleRateControl.get());
}
Expand All @@ -36,6 +37,7 @@ int EngineRecord::updateFromPreferences() {
m_baAlbum = m_pConfig->getValueString(ConfigKey(RECORDING_PREF_KEY, "Album"));
m_cueFileName = m_pConfig->getValueString(ConfigKey(RECORDING_PREF_KEY, "CuePath"));
m_bCueIsEnabled = m_pConfig->getValueString(ConfigKey(RECORDING_PREF_KEY, "CueEnabled")).toInt();
m_bCueUsesFileAnnotation = m_pConfig->getValueString(ConfigKey(RECORDING_PREF_KEY, "CueFileAnnotationEnabled")).toInt();
m_sampleRate = mixxx::audio::SampleRate::fromDouble(m_sampleRateControl.get());

// Delete m_pEncoder if it has been initialized (with maybe) different bitrate.
Expand Down Expand Up @@ -249,6 +251,12 @@ void EngineRecord::writeCueLine() {
.arg(m_pCurrentTrack->getArtist())
.toUtf8());

if (m_bCueUsesFileAnnotation) {
m_cueFile.write(QString(" FILE \"%1\"\n")
Copy link
Member

Choose a reason for hiding this comment

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

I believe you might be able to use a QStringLiteral

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@presentformyfriends presentformyfriends Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Feel free to also fix other occurrence

Copy link
Author

Choose a reason for hiding this comment

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

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

.arg(m_pCurrentTrack->getLocation())
.toUtf8());
}

// Woefully inaccurate (at the seconds level anyways).
// We'd need a signal fired state tracker
// for the track detection code.
Expand Down
1 change: 1 addition & 0 deletions src/engine/sidechain/enginerecord.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,4 +85,5 @@ class EngineRecord : public QObject, public EncoderCallback, public SideChainWor
QString m_cueFileName;
quint64 m_cueTrack;
bool m_bCueIsEnabled;
bool m_bCueUsesFileAnnotation;
};
37 changes: 36 additions & 1 deletion src/preferences/dialog/dlgprefrecord.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ DlgPrefRecord::DlgPrefRecord(QWidget* parent, UserSettingsPointer pConfig)
CheckBoxRecordCueFile->setChecked(m_pConfig->getValue<bool>(
ConfigKey(RECORDING_PREF_KEY, "CueEnabled"), kDefaultCueEnabled));

CheckBoxUseCueFileAnnotation->setChecked(m_pConfig->getValue<bool>(
ConfigKey(RECORDING_PREF_KEY, "CueFileAnnotationEnabled"), false));
presentformyfriends marked this conversation as resolved.
Show resolved Hide resolved

// Setting split
comboBoxSplitting->addItem(SPLIT_650MB);
comboBoxSplitting->addItem(SPLIT_700MB);
Expand Down Expand Up @@ -119,6 +122,11 @@ DlgPrefRecord::DlgPrefRecord(QWidget* parent, UserSettingsPointer pConfig)
&QAbstractSlider::sliderReleased,
this,
&DlgPrefRecord::slotSliderCompression);

connect(CheckBoxRecordCueFile,
&QCheckBox::stateChanged,
this,
&DlgPrefRecord::slotToggleCueEnabled);
}

DlgPrefRecord::~DlgPrefRecord() {
Expand All @@ -144,6 +152,7 @@ void DlgPrefRecord::slotApply() {
saveMetaData();
saveEncoding();
saveUseCueFile();
saveUseCueFileAnnotation();
saveSplitSize();
}

Expand Down Expand Up @@ -175,10 +184,12 @@ void DlgPrefRecord::slotUpdate() {

loadMetaData();

// Setting miscellaneous
// Setting miscellaneous
CheckBoxRecordCueFile->setChecked(m_pConfig->getValue<bool>(
ConfigKey(RECORDING_PREF_KEY, "CueEnabled"), kDefaultCueEnabled));

updateCueEnabled();

QString fileSizeStr = m_pConfig->getValueString(ConfigKey(RECORDING_PREF_KEY, "FileSize"));
int index = comboBoxSplitting->findText(fileSizeStr);
if (index >= 0) {
Expand All @@ -201,7 +212,12 @@ void DlgPrefRecord::slotResetToDefaults() {

// 4GB splitting is the default
comboBoxSplitting->setCurrentIndex(4);

// Sets 'Create a CUE file' checkbox value
CheckBoxRecordCueFile->setChecked(kDefaultCueEnabled);

// Sets 'Enable File Annotation in CUE file' checkbox value
CheckBoxUseCueFileAnnotation->setChecked(false);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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!

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@presentformyfriends presentformyfriends Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

}

void DlgPrefRecord::slotBrowseRecordingsDir() {
Expand Down Expand Up @@ -303,6 +319,16 @@ void DlgPrefRecord::slotSliderQuality() {
// Settings are only stored when doing an apply so that "cancel" can actually cancel.
}

// Set 'Enable File Annotation in CUE file' checkbox value depending on 'Create a CUE file' checkbox value
void DlgPrefRecord::updateCueEnabled() {
if (CheckBoxRecordCueFile->isChecked()) {
CheckBoxUseCueFileAnnotation->setEnabled(true);
} else {
CheckBoxUseCueFileAnnotation->setEnabled(false);
CheckBoxUseCueFileAnnotation->setChecked(false);
}
presentformyfriends marked this conversation as resolved.
Show resolved Hide resolved
}

void DlgPrefRecord::updateTextQuality() {
EncoderRecordingSettingsPointer settings =
EncoderFactory::getFactory().getEncoderRecordingSettings(
Expand Down Expand Up @@ -429,11 +455,20 @@ void DlgPrefRecord::saveEncoding() {
}
}

void DlgPrefRecord::slotToggleCueEnabled() {
updateCueEnabled();
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

@presentformyfriends presentformyfriends Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Just pushed new commit for this.


void DlgPrefRecord::saveUseCueFile() {
m_pConfig->set(ConfigKey(RECORDING_PREF_KEY, "CueEnabled"),
ConfigValue(CheckBoxRecordCueFile->isChecked()));
}

void DlgPrefRecord::saveUseCueFileAnnotation() {
m_pConfig->set(ConfigKey(RECORDING_PREF_KEY, "CueFileAnnotationEnabled"),
ConfigValue(CheckBoxUseCueFileAnnotation->isChecked()));
}

void DlgPrefRecord::saveSplitSize() {
m_pConfig->set(ConfigKey(RECORDING_PREF_KEY, "FileSize"),
ConfigValue(comboBoxSplitting->currentText()));
Expand Down
5 changes: 5 additions & 0 deletions src/preferences/dialog/dlgprefrecord.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ class DlgPrefRecord : public DlgPreferencePage, public Ui::DlgPrefRecordDlg {
void slotSliderCompression();
void slotGroupChanged();

private slots:
void slotToggleCueEnabled();

signals:
void apply(const QString &);

Expand All @@ -44,6 +47,8 @@ class DlgPrefRecord : public DlgPreferencePage, public Ui::DlgPrefRecordDlg {
void saveMetaData();
void saveEncoding();
void saveUseCueFile();
void saveUseCueFileAnnotation();
void updateCueEnabled();
void saveSplitSize();

// Pointer to config object
Expand Down
15 changes: 15 additions & 0 deletions src/preferences/dialog/dlgprefrecorddlg.ui
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,20 @@
</widget>
</item>


<item row="3" column="0" colspan="3">
<widget class="QCheckBox" name="CheckBoxUseCueFileAnnotation">
<property name="toolTip">
<string>This will include the filepath for each track in the CUE file.
This option makes the CUE file less portable and can reveal personal
information from filepaths (i.e. username)</string>
</property>
<property name="text">
<string>Enable File Annotation in CUE file</string>
</property>
</widget>
</item>

</layout>
</widget>
</item>
Expand Down Expand Up @@ -386,6 +400,7 @@
<tabstop>PushButtonBrowseRecordings</tabstop>
<tabstop>comboBoxSplitting</tabstop>
<tabstop>CheckBoxRecordCueFile</tabstop>
<tabstop>CheckBoxUseCueFileAnnotation</tabstop>
<tabstop>SliderCompression</tabstop>
<tabstop>SliderQuality</tabstop>
<tabstop>LineEditTitle</tabstop>
Expand Down
Loading