-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Auto dj crates #18
Auto dj crates #18
Changes from 15 commits
eb6d41b
638c8b7
de8a563
6a30eb3
ab9046c
43360c9
417532a
6e4f2f0
02bd977
dea693d
f1a6d70
9d34467
1ed581f
72cad18
8e03aa0
4a9ffe7
60de69f
1bc835b
5f63af5
d9c096a
8df76eb
6735fbc
d7cf2c1
b3743ef
0d7af53
41d1168
852546d
92bddf1
1c3df49
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,6 +77,14 @@ DlgAutoDJ::DlgAutoDJ(QWidget* parent, ConfigObject<ConfigValue>* pConfig, | |
connect(pushButtonSkipNext, SIGNAL(clicked(bool)), | ||
this, SLOT(skipNextButton(bool))); | ||
|
||
#ifdef __AUTODJCRATES__ | ||
connect(pushButtonAddRandom, SIGNAL(clicked(bool)), | ||
this, SIGNAL(addRandomButton(bool))); | ||
#else // __AUTODJCRATES__ | ||
pushButtonAddRandom->setVisible(false); | ||
horizontalLayout->removeWidget(pushButtonAddRandom); | ||
#endif // __AUTODJCRATES__ | ||
|
||
m_pCOFadeNow = new ControlPushButton( | ||
ConfigKey("[AutoDJ]", "fade_now")); | ||
m_pCOTFadeNow = new ControlObjectThreadMain(m_pCOFadeNow); | ||
|
@@ -676,3 +684,10 @@ void DlgAutoDJ::transitionValueChanged(int value) { | |
ConfigValue(value)); | ||
m_backUpTransition = value; | ||
} | ||
|
||
void DlgAutoDJ::enableRandomButton(bool a_bEnabled) | ||
{ | ||
#ifdef __AUTODJCRATES__ | ||
pushButtonAddRandom->setEnabled (a_bEnabled); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove spaces between function and arguments list (here and elsewhere). |
||
#endif // __AUTODJCRATES__ | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -212,6 +212,42 @@ DlgPrefControls::DlgPrefControls(QWidget * parent, MixxxApp * mixxx, | |
ComboBoxAutoDjRequeue->setCurrentIndex(m_pConfig->getValueString(ConfigKey("[Auto DJ]", "Requeue")).toInt()); | ||
connect(ComboBoxAutoDjRequeue, SIGNAL(activated(int)), this, SLOT(slotSetAutoDjRequeue(int))); | ||
|
||
#ifdef __AUTODJCRATES__ | ||
|
||
// The auto-DJ active-percentage for randomly-selected tracks | ||
autoDjActivePercentageSpinBox->setValue (m_pConfig->getValueString | ||
(ConfigKey("[Auto DJ]", "ActivePercentage"), "20").toInt()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't separate argument lists from function names. The reason for this is so that you can tell that getValueString is a function call without having to parse the entire multi-line statement. |
||
connect(autoDjActivePercentageSpinBox, SIGNAL(valueChanged(int)), | ||
this, SLOT(slotSetAutoDjActivePercentage(int))); | ||
|
||
// The auto-DJ replay-age for randomly-selected tracks | ||
autoDjReplayAgeCheckBox->setChecked ((bool) m_pConfig->getValueString | ||
(ConfigKey("[Auto DJ]", "UseReplayAge"), "0").toInt()); | ||
connect(autoDjReplayAgeCheckBox, SIGNAL(stateChanged(int)), | ||
this, SLOT(slotSetAutoDjUseReplayAge(int))); | ||
autoDjReplayAgeTimeEdit->setTime (QTime::fromString | ||
(m_pConfig->getValueString | ||
(ConfigKey("[Auto DJ]", "ReplayAge"), "23:59"), | ||
autoDjReplayAgeTimeEdit->displayFormat())); | ||
autoDjReplayAgeTimeEdit->setEnabled(autoDjReplayAgeCheckBox->checkState() | ||
== Qt::Checked); | ||
connect(autoDjReplayAgeTimeEdit, SIGNAL(timeChanged(const QTime &)), | ||
this, SLOT(slotSetAutoDjReplayAge(const QTime &))); | ||
|
||
#else // __AUTODJCRATES__ | ||
|
||
// Remove the preferences. | ||
autoDjActivePercentageLabel->setVisible(false); | ||
GridLayout1->removeWidget(autoDjActivePercentageLabel); | ||
autoDjActivePercentageSpinBox->setVisible(false); | ||
GridLayout1->removeWidget(autoDjActivePercentageSpinBox); | ||
autoDjReplayAgeCheckBox->setVisible(false); | ||
GridLayout1->removeWidget(autoDjReplayAgeCheckBox); | ||
autoDjReplayAgeTimeEdit->setVisible(false); | ||
GridLayout1->removeWidget(autoDjReplayAgeTimeEdit); | ||
|
||
#endif // __AUTODJCRATES__ | ||
|
||
// | ||
// Skin configurations | ||
// | ||
|
@@ -253,6 +289,10 @@ DlgPrefControls::DlgPrefControls(QWidget * parent, MixxxApp * mixxx, | |
// | ||
// Tooltip configuration | ||
// | ||
// Set default value in config file, if not present | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this to fix the issue where tooltips are not showing on a new profile? This is fixed in master I believe. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't add that. I've removed it. |
||
if (m_pConfig->getValueString(ConfigKey("[Controls]","Tooltips")).length() == 0) | ||
m_pConfig->set(ConfigKey("[Controls]","Tooltips"), ConfigValue(1)); | ||
|
||
ComboBoxTooltips->addItem(tr("On")); // 1 | ||
ComboBoxTooltips->addItem(tr("On (only in Library)")); // 2 | ||
ComboBoxTooltips->addItem(tr("Off")); // 0 | ||
|
@@ -424,6 +464,33 @@ void DlgPrefControls::slotSetAutoDjRequeue(int) | |
m_pConfig->set(ConfigKey("[Auto DJ]", "Requeue"), ConfigValue(ComboBoxAutoDjRequeue->currentIndex())); | ||
} | ||
|
||
void DlgPrefControls::slotSetAutoDjActivePercentage(int a_iValue) | ||
{ | ||
#ifdef __AUTODJCRATES__ | ||
QString str; | ||
str.setNum (a_iValue); | ||
m_pConfig->set(ConfigKey("[Auto DJ]","ActivePercentage"),str); | ||
#endif // __AUTODJCRATES__ | ||
} | ||
|
||
void DlgPrefControls::slotSetAutoDjUseReplayAge(int a_iState) | ||
{ | ||
#ifdef __AUTODJCRATES__ | ||
bool bChecked = (a_iState == Qt::Checked); | ||
QString strChecked = (bChecked) ? "1" : "0"; | ||
m_pConfig->set (ConfigKey("[Auto DJ]", "UseReplayAge"), strChecked); | ||
autoDjReplayAgeTimeEdit->setEnabled(bChecked); | ||
#endif // __AUTODJCRATES__ | ||
} | ||
|
||
void DlgPrefControls::slotSetAutoDjReplayAge(const QTime &a_rTime) | ||
{ | ||
#ifdef __AUTODJCRATES__ | ||
QString str = a_rTime.toString (autoDjReplayAgeTimeEdit->displayFormat()); | ||
m_pConfig->set(ConfigKey("[Auto DJ]","ReplayAge"),str); | ||
#endif // __AUTODJCRATES__ | ||
} | ||
|
||
void DlgPrefControls::slotSetTooltips(int) | ||
{ | ||
int configValue = (ComboBoxTooltips->currentIndex() + 1) % 3; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ | |
<x>0</x> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this ought to go in a new preferences section for AutoDJ. That will pave the way for more advanced AutoDJ stuff. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, of cause. But the preferences reordering is out of scope of this project. So this could be done after merging the advanced-auto-dj branch as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you, Daniel. I appreciate your reasonableness. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We treat this dialog like a clown car. AutoDJ requeue shouldn't have gone in here in the first place. Doing this with the advanced autodj work sounds good -- thanks Daniel. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I put them here because the "Interface" preferences already had the "re-queue tracks in Auto DJ" entry. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to explain -- that's reasonable. There's a tipping point and we're pretty much at it is all I'm saying. |
||
<y>0</y> | ||
<width>519</width> | ||
<height>728</height> | ||
<height>939</height> | ||
</rect> | ||
</property> | ||
<property name="windowTitle"> | ||
|
@@ -283,6 +283,46 @@ | |
</property> | ||
</widget> | ||
</item> | ||
<item row="11" column="0"> | ||
<widget class="QLabel" name="autoDjActivePercentageLabel"> | ||
<property name="text"> | ||
<string>Auto DJ: Minimum available tracks (percent)</string> | ||
</property> | ||
</widget> | ||
</item> | ||
<item row="11" column="1" colspan="2"> | ||
<widget class="QSpinBox" name="autoDjActivePercentageSpinBox"> | ||
<property name="toolTip"> | ||
<string>This percentage of tracks are always available for selecting, regardless of when they were last played.</string> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's pretty unclear what this setting does from the name and description. Is there a more user-friendly name and description? "When the pool of unplayed tracks drops below this percentage of the total tracks available to AutoDJ, add the least-recently played tracks back into the pool until it is this size." :-/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That wasn't my choice -- daschuer suggested this text. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Way to throw @daschuer under the bus :P. I'm not interested in who wrote what. I'm trying to figure out the best phrasing to maximize comprehensibility. Got any ideas? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was just saying that in case your multitude of petty criticisms was somehow directed at me personally (e.g. because I went ahead and implemented a feature that you wanted kiboshed). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no rush to merge here -- we're nowhere near our next major release. What you're saying is that you want to be done working on this and I understand that but please work with me to get the code and feature in a good, ready to release state because once it is merged I know from experience that you (i.e. you the general you, contributor of a shiny new feature) won't be as interested in fixing tooltip phrasing. Regarding style issues -- there are minor pros and cons of various styles but the most important thing is uniformity. When the style is uniform, I can scan a file much easier and generally eyeball the shape of it -- see how the control flows, the exit points, blocks, etc. I'm sorry for the tedious style comments but they are an important part of a healthy codebase. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Re: rush to merge...the longer we go without merging, the more work I have to do to keep up with trunk changes that affect my code (e.g. getting rid of the top-level mixxx directory, which "git merge" handled badly, requiring me to do a LOT of hand-work to make things right again). This is wasted effort. Re: style comments...in my experience, as long as the code is human-readable, that's more than enough. If one is modifying someone else's code, one should try very hard to mimic the local style, so that the changes don't look jarring next to older code. Anything on top of that is needlessly pedantic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, that top level change was quite rough. Sorry :-/. There won't be anything that drastic to merge with going forward. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ulatekh: You have forked from a version without the root mixxx folder. daschuer@8e03aa0. So the compassion goes to me ;-) |
||
</property> | ||
<property name="maximum"> | ||
<number>100</number> | ||
</property> | ||
<property name="value"> | ||
<number>20</number> | ||
</property> | ||
</widget> | ||
</item> | ||
<item row="12" column="0"> | ||
<widget class="QCheckBox" name="autoDjReplayAgeCheckBox"> | ||
<property name="text"> | ||
<string>Auto DJ: Track ignore time</string> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This name isn't very informative. How about: "AutoDJ minimum time until track replay." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Auto DJ: Track ignore time" is already quite long for a preference NAME. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There isn't really a distinction between being queued and being played because being queued is the same thing as being played in the future. We could say re-queue instead of replay though? The track can't get replayed until it is back in the pool so it makes sense to me to call this the minimum time until a track is replayed/requeued. Or at least I think it would be clearer to a first time user than "Track ignore time". RE: name length. We already have preference names like "Re-queue tracks in AutoDJ". I don't think this is too long. When this all moves into its own preference dialog they will have more room to breathe. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, "Auto DJ: minimum time until track re-queue." would be fine for me, If it is this what a native speaker understand the best. Since this new name describes the underlying methods fine, I am just have concerns if this is the best description from th use case point of view. IMHO The feature will be used most radio radio DJs where it is common to replay tracks after a hour or so. De: "Auto-DJ: Titel ausschließen für ..." I would be happy if a native speaker will propose the ultimate solution ;-) |
||
</property> | ||
</widget> | ||
</item> | ||
<item row="12" column="1" colspan="2"> | ||
<widget class="QTimeEdit" name="autoDjReplayAgeTimeEdit"> | ||
<property name="toolTip"> | ||
<string>Time after a track is available for selecting again</string> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really a sentence... how about: "Duration after which a track is eligible for selection by AutoDJ." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Duration after which a track is eligible for selection by AutoDJ again."? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good |
||
</property> | ||
<property name="displayFormat"> | ||
<string>hh:mm</string> | ||
</property> | ||
<property name="timeSpec"> | ||
<enum>Qt::LocalTime</enum> | ||
</property> | ||
</widget> | ||
</item> | ||
</layout> | ||
</item> | ||
<item row="2" column="0"> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now outdated (v19 is taken).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This, as well as the formatting issues and goto usage, have been taken care of.
Please let me know if there's anything else preventing this pull request from being merged.