Auto dj crates #18
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ | |
| <x>0</x> | ||
|
There was a problem hiding this comment. 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. 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. Thank you, Daniel. I appreciate your reasonableness. There was a problem hiding this comment. 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. 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. 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>939</height> | ||
| </rect> | ||
| </property> | ||
| <property name="windowTitle"> | ||
|
|
@@ -283,6 +283,49 @@ | |
| </property> | ||
| </widget> | ||
| </item> | ||
| <item row="11" column="0"> | ||
| <widget class="QLabel" name="autoDjMinimumAvailableLabel"> | ||
| <property name="text"> | ||
| <string>Auto DJ: Minimum available tracks [%]</string> | ||
| </property> | ||
| </widget> | ||
| </item> | ||
| <item row="11" column="1" colspan="2"> | ||
| <widget class="QSpinBox" name="autoDjMinimumAvailableSpinBox"> | ||
| <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. 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. That wasn't my choice -- daschuer suggested this text. There was a problem hiding this comment. 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. 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. 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. 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. 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. @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="autoDjIgnoreTimeCheckBox"> | ||
| <property name="toolTip"> | ||
| <string>Uncheck, to ignore all played tracks.</string> | ||
| </property> | ||
| <property name="text"> | ||
| <string>Auto DJ: Suspend track from re-queue</string> | ||
| </property> | ||
| </widget> | ||
| </item> | ||
| <item row="12" column="1" colspan="2"> | ||
| <widget class="QTimeEdit" name="autoDjIgnoreTimeEdit"> | ||
| <property name="toolTip"> | ||
| <string>Duration after which a track is eligible for selection by Auto DJ again</string> | ||
| </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.
QString::number() would work better here.
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.
Actually, it works exactly the same. Your issue is purely stylistic.
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.
Yep -- could you change it please?
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.
I can't believe this multitude of minor style issues are enough to hold up merging these changes.
Your focus on style over substance is hard to understand.
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.
(It's something I would change myself once your branch is merged as part of cleanup that I do whenever a branch merges. Now that we have GitHub line-by-line commenting I get to ask you to do it instead of me!)
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.
@ulatekh Coding style, especially in a language like C++, is wide open to interpretation and variation. We could use camelCase, lpHungarianNotation, or some_other_style_. If every developer used whatever style they wanted, the code would be harder to read because it wouldn't be consistent from module to module. In fact, mixxx already has major problems because of this (the vinylcontrol code, which I'm in charge of, comes to mind :/).
Therefore, style is actually a pretty important part of our review process, and we're actually a lot more lax than other organizations. We don't enforce column limits, we allow lone braces on following lines, etc -- even though it might help readability to be more strict. The old adage applies: code is read more often than written. As a project that is starting to get quite large and with a large developer community, having a core style is becoming more important. So while it's absolutely true that RJ's suggestions are functionally equivalent to what you wrote, there are two more important things to keep in mind: 1) RJ is not criticizing your ability or opinions directly -- there's nothing personal involved; and 2) RJ knows how most of the code looks in mixxx and knows what we other developers are used to seeing. I know it's difficult to slog through a ton of comments that seem nit-picky, but the result will be more consistent code for everyone to read.
I understand that you might prefer certain patterns, and goodness knows there are some patterns I'm asked to write in my paying work that I don't agree with, but we have good reasons for asking for the style changes we ask for and we hope that you can give us the benefit of the doubt and roll with the nitpicks. If you feel like we're removing some evidence of your personality from your code, that's not entirely false -- we'd like all of the code to feel like Mixxx code, regardless of which developer wrote it. But the upside is that the codebase feels more cohesive and is easier for others to learn.