Merge lp://qastaging/~ulatekh/mixxx/auto-dj-crates into lp://qastaging/~mixxxdevelopers/mixxx/trunk

Here comments from my first partial review:

* The Add random button schould work without Connected Crate.
What about pick a random song from Library in this case?
* I would like to have an option to auto add tracks when auto DJ is running out of tracks.
This is the same usecase than Reqeue tracks in Auto DJ, so we can take a common control:
"Top Up Auto DJ" Off / Requeue / Random
* Rename "Add Crate to Auto DJ" to something else like "Connect with Auto DJ" to distingisch it from "Add Tracks to Auto DJ Playlist"
* Auto DJ active percentage" is not self explaining. What about "Auto DJ minimum track replay time"

http://www.mixxx.org/wiki/doku.php/coding_guidelines

"} else {" ..

AutoDJCratesDAO::AutoDJCratesDAO(QSqlDatabase& a_rDatabase,
        TrackDAO& a_rTrackDAO, CrateDAO& a_rCrateDAO, PlaylistDAO &a_rPlaylistDAO,
        ConfigObject<ConfigValue>* a_pConfig)
    : m_rDatabase(a_rDatabase),
      m_rTrackDAO(a_rTrackDAO),

These definitions are unused:
 +#define SETTINGS_TABLE "settings"
2074 +
2075 +#define SETTINGSTABLE_NAME "name"
2076 +#define SETTINGSTABLE_VALUE "value"
2077 +#define SETTINGSTABLE_LOCKED "locked"
2078 +#define SETTINGSTABLE_HIDDEN "hidden"

It is still open to review the temporary tables. I will try to do it soon.

But so far it looks good to me.
Also I would like to have it anabled by default so that we will get feedback from bleeding edge tester.

@RJ: what do you think?

If the "Add random" button worked without any connected crates, or if tracks could get added automatically when auto-DJ is running out of tracks, then it seems like that would make Mixxx too much like a media player, which was RJ Ryan's main objection to my original design.

Auto-DJ crates _are_ enabled by default; that's on line 1107 of the branch's build/features.py .

Renaming "Add crate to Auto-DJ" to "Connect with Auto DJ" seems sensible enough.

"Auto DJ minimum track replay time" doesn't really explain it either. It's the percentage of potential auto-DJ-crate tracks that will actually be selected from, unless there are enough never-played tracks, in which case it's a minimum. We can try to work on a better name.

The unused settings-table definitions can be removed if you want, however, to make the DAOs easier to use, perhaps they should be left in (so that people don't hardcode those names in their own code).

In any case, these seem like minor issues, or matters of opinion (and thus subject to wider debate), and I believe that they shouldn't hold up merging auto-DJ-crates into the trunk. All these issues can be fixed easily post-merge, once we all decide what direction to take with them.

Finally I have relieved all the code. Sorry for the delay.

It is an interesting approach to use sqlite temporary tables for calculating the next song.
For me it was hard to read and understand all the sql statements.
But I am not sure if a pure C++ implementation would be cleaner, so it is fine.

While you had really a lot off comments, I miss one in the place where you calculate the last played from the set log table.

here my reply to some of your comments:

> If the "Add random" button worked without any connected crates, or if tracks
> could get added automatically when auto-DJ is running out of tracks, then it
> seems like that would make Mixxx too much like a media player, which was RJ
> Ryan's main objection to my original design.
Yes, the whole feature branch is a step in the jukebox direction, enable the button without a connected crate will not hurt anyone. But OK, can be fixed later.

> "Auto DJ minimum track replay time" doesn't really explain it either. It's
> the percentage of potential auto-DJ-crate tracks that will actually be
> selected from, unless there are enough never-played tracks, in which case it's
> a minimum. We can try to work on a better name.
"Auto DJ active percentage" aound very developer driven. I have only understand it after I understood the whole concept. And the "active percentage" concept is not my favourite one. Because it possible ignores tracks that are never played.
On extreme case is if you have a two new crate with never played songs and the active percentage of 50 %, you will only hears tracks from first crate, because its tracks are on top of the temp table.
I think the user expect a mix from both crates.
A "Auto DJ minimum track replay time", the time which have to be expired until the song becomes active again.
... will solve this problem. The other advantage to me is, that this time would be independent of the count of tracks in the auto DJ crate list. So we could set it to 12 h by default and you will never hear a track again if in a current gig. We can still have a fixed "active percentage", in case no track meets the "Auto DJ minimum track replay time" criteria, this might be a radio use case.

> In any case, these seem like minor issues, or matters of opinion (and thus
> subject to wider debate), and I believe that they shouldn't hold up merging
> auto-DJ-crates into the trunk. All these issues can be fixed easily post-
> merge, once we all decide what direction to take with them.
Of cores you are right, but we made bad experiences in the past to commit half baked code.
So lets fix most issues in this branch.
At least the coding style issues should be fixed, because it is hard to track logic changes in bzr if someone has reformatted the code in between.

>Finally I have reviewed all the code. Sorry for the delay.

No problem. Thanks for reviewing it!

>It is an interesting approach to use sqlite temporary tables for
>calculating the next song. For me it was hard to read and understand
>all the sql statements. But I am not sure if a pure C++
>implementation would be cleaner, so it is fine.

The original version (from my old djserver program) was pure C++, but that involved keeping all the data in memory. I figured that keeping it in temporary tables was more memory-efficient and more defensible; plus, it made sqlite do all the heavy lifting.

The SQL statements might be long, but that's how you use SQL in an efficient manner. In most of Mixxx's code, SQL seems to be used in a very simplistic manner, as a mere external data store. That approach is prone to severe performance problems; for instance, on April 21st, I submitted a change to how we import playlists that contain songs that are already in the library -- it changed my test case (a playlist with 1900+ songs) from a 5-minute operation that hung the GUI, to a 2-second operation. I did the same thing recently with merging set-logs.

>While you had really a lot of comments, I miss one in the place where
>you calculate the last played from the set log table.

That was the most recent addition, so you probably didn't miss it -- it just wasn't there when you first reviewed the code.

>> If the "Add random" button worked without any connected crates, or if tracks
>> could get added automatically when auto-DJ is running out of tracks, then it
>> seems like that would make Mixxx too much like a media player, which was RJ
>> Ryan's main objection to my original design.
>
>Yes, the whole feature branch is a step in the jukebox direction,
>enable the button without a connected crate will not hurt anyone. But
>OK, can be fixed later.

I just want to make sure I don't implement it in a way that RJ Ryan will object to. That was the entire reason why I went in this direction.

>>>"Auto DJ minimum track replay time" doesn't really explain it either.
>>
>>It's the percentage of potential auto-DJ-crate tracks that will
>>actually be selected from, unless there are enough never-played
>>tracks, in which case it's a minimum. We can try to work on a better
>>name.
>
>"Auto DJ active percentage" sounds very developer driven. I have only
>understand it after I understood the whole concept. And the "active
>percentage" concept is not my favourite one. Because it possible
>ignores tracks that are never played. On extreme case is if you have a
>two new crate with never played songs and the active percentage of 50
>%, you will only hears tracks from first crate, because its tracks are
>on top of the temp table. I think the user expect a mix from both
>crates.

No, that's not how it works. The active-percentage is a minimum. If you have two crates with never-played songs, "add random" will select from both crates. The active-percentage only comes into play when 100-minus-active percent of the tracks have been played.

Hopefully, resolving this misunderstanding makes an "Auto DJ minimum track replay time" setting unnecessary.

>> In any case, these...

> >>>"Auto DJ minimum track replay time" doesn't really explain it either.
> >>
> >>It's the percentage of potential auto-DJ-crate tracks that will
> >>actually be selected from, unless there are enough never-played
> >>tracks, in which case it's a minimum. We can try to work on a better
> >>name.
> >
> >"Auto DJ active percentage" sounds very developer driven. I have only
> >understand it after I understood the whole concept. And the "active
> >percentage" concept is not my favourite one. Because it possible
> >ignores tracks that are never played. On extreme case is if you have a
> >two new crate with never played songs and the active percentage of 50
> >%, you will only hears tracks from first crate, because its tracks are
> >on top of the temp table. I think the user expect a mix from both
> >crates.
>
> No, that's not how it works. The active-percentage is a minimum. If you have
> two crates with never-played songs, "add random" will select from both crates.
> The active-percentage only comes into play when 100-minus-active percent of
> the tracks have been played.
>
> Hopefully, resolving this misunderstanding makes an "Auto DJ minimum track
> replay time" setting unnecessary.
Ok, Is it correct that your approach is only looking at the current Mixxx run?

But anyhow, you see how hard it is to understand "active percentage".
So it is finally only a naming issue.

Btw: Does your code track the case if one joins the current setlog with the previous one and the play count is adopted?

IMHO is much cleaner to configure the time, after it is allowed to play a track again!

What about this idea?
* discard the "times played" cloumn and only take "last played" into account"
* Two settings in preferences
** Auto DJ selector: Track ignore time" Tooltip: "Time after a track is available for selecting again"
** Auto DJ selector: Minimum available tracks" Tooltip: "This prcentage of tracks are always available for selecting, regardless of when they were last played."

This would be nice for resident DJs where previous set logs are of interest.

> I hope my code is better than half-baked. ;-)
Yes it is, sorry for the unfair compare.
>
> I fixed the coding-style issues in src/library/dao/autodjcrates.cpp, which I
> think were just curly-brace placement. Is there anything else holding up the
> merge?
I would like to have a final solution for the "active percentage" issue first
and I would also like to give RJ and Christopher the chance to add a comments.

>Is it correct that your approach is only looking at the current
>Mixxx run?

No, it takes all existing set-logs into account.

>Btw: Does your code track the case if one joins the current setlog
>with the previous one and the play count is adopted?

Yes, my code tracks that, but since joining setlogs doesn't affect which tracks have been played and when, only which setlog they're recorded in, it has no net effect.

>IMHO is much cleaner to configure the time, after it is allowed to
>play a track again!

I'm pretty sure that's just an additional database query, so it shouldn't be hard to implement.

>What about this idea?
>* discard the "times played" column and only take "last played" into account"

I'm not thrilled with that, but if that's what you really want, I can do it easily.

>* Two settings in preferences
>** Auto DJ selector: Track ignore time" Tooltip: "Time after a track
> is available for selecting again"
>** Auto DJ selector: Minimum available tracks" Tooltip: "This
> percentage of tracks are always available for selecting, regardless
> of when they were last played."

I can do that easily.

>> Is there anything else holding up the merge?
>
>I would like to have a final solution for the "active percentage" issue
>first and I would also like to give RJ and Christopher the chance to
>add a comments.

Let me know if there's anything except for the above changes.

OK, you can now specify a time range (from 0 to 23 hours 59 minutes, i.e. the limit of QTimeEdit) that tracks can be considered for add-random. You can also enable/disable the feature completely with a checkbox. The times-played column is not considered when this feature is enabled (since that interferes with finding tracks that haven't been played since a given date/time), but the times-played column is still taken into account if this feature is disabled.

Let me know if this is how you wanted this done.

Hi Steven,

Thank you for the quick response and for providing a patch and I am really sorry for still not being satisfied.

I have tested your recent version yesterday and it still not acting as I am expecting.

I have a crate connected to Auto DJ and it has four tracks, two of them are not played and the other two are palyed about ~8. All tracks are not played in the current session. With Minimum available tracks at 50 %, I have expected that if I press four "Add" and I get all tracks in random order. But only the never played tracks are queued after that the button does not react. The Minimum available tracks parameter should guarantee that there are allays tracks "Available".

By the way, my idea with two parameters was intended for discussion. It is always nice to have something working, but if you are not thrilled with this, lets find a better solution.

Here some thoughts for discussion:
* the "timesplayed" counter is an all time played counter. So this is not a good indicator for selecting a track in our case.
* the "played" flag is a slightly better indicator but also not very good for usecase where Mixxx is not restarted (like radio broadcasting)
* IMHO lastplayed is the best indicator

Here a possible solution:

the temp_autodj_crates table is sorted like this:

At the bottom the current tracks in the Auto DJ playlist, in the same order like in the list.
Above all the tracks from the connected crates not in the playlist sorted by lastplayed.
The least recent played track at the top. Above all unplayed tracks.

1. newer played a
2. newer played b
3. newer played c
4. played 20h ago
5. played 15h ago
6. played 10h ago
7. Audo DJ #1 (last played is in the future -> -3:30 ago)
8. Audo DJ #2
9. Audo DJ #3
10. Auto DJ #4 <- Last random pick

* the % of Minimum available tracks from the top are available in any case.
* in addition all tracks, where now() - lastplayed > "Track ignore time" are also available.

With this we will never run out of tracks and we will get the best choice also in weird conditions.

In case all tracks are played within "Track ignore time", the "Minimum available" takes place.
In case all tracks are already queued, the top "Minimum available" Tracks are queued again.

In this scenario, "Track ignore time" must be always enabled or we drop it entirely and use the played flag instead.

How does fit to your ideas?

Kind regards,

Daniel

>I have tested your recent version yesterday and it still not acting as
>I am expecting.

At this point, I must protest. :-)

The auto-DJ-crates feature is not intended for situations where there are very few tracks available -- it's meant for the exact opposite situation. When a DJ (as they commonly do) has far more tracks available than can ever be comprehended, it's nice to have a way to get automatic suggestions for tracks that conform to some criteria (i.e. after they've been organized into crates). No sane DJ is going to need help choosing between four tracks.

Having said that, I found the problem you encountered, and fixed it. The number of available tracks was underflowing, so in that situation, I let it choose from all available tracks.

>By the way, my idea with two parameters was intended for discussion.
>It is always nice to have something working, but if you are not
>thrilled with this, let's find a better solution.

What I wrote is acceptable to me -- the active-tracks list is still sorted by times-played if your replay-age feature is turned off.

>IMHO lastplayed is the best indicator

And that's what you have when your replay-age feature is turned on.

>Here a possible solution:
>[...]
>How does fit to your ideas?

As it stands now, auto-DJ-crates does everything you ask for, with the exception of allowing tracks that are already in the auto-DJ queue, or loaded into decks, to be selected randomly. I see no value in allowing an already-queued song to be queued again via random selection.

The auto-DJ-crates feature was intended to cope with the exact opposite situation under which you're testing it, and no sane DJ will ever use it with a small number of tracks, so I must protest that your evaluation criteria are unfair.

"Congratulations...you passed the test. It takes courage to say that the test is unfair." -Lt. Worf to Ensign Sito, Star Trek: The Next Generation, "Lower Decks"

:-)

Yes, I have missed to write that my test case was one unrealsistic case just for testing purpose :-)

OK, you are right, adding tracks which are allready in the Auto DJ playlist might be also no good idea. My intention was to make the behaviour of "Minimum available tracks" easy to understand with no special cases.

Can you explain why "int iUnplayedTracks" is still importandt for you. I still not like it because a good DJ will have listen his tracks at least one time, so the case that you have unplayed tracks is only when you have a fresh scaned library. But for our long term users it will always 0. Doesn' t i make sence to go with played flag here?

For me it is fine for a clean GUI, to expose only one solution: the played flag or the always active ignore time.

>Can you explain why "int iUnplayedTracks" is still important for you.
>I still not like it because a good DJ will have listen his tracks at
>least one time, so the case that you have unplayed tracks is only when
>you have a fresh scanned library. But for our long term users it will
>always 0.

The DJ wouldn't have necessarily used Mixxx to listen to his tracks. Also, if the DJs I know are any indication, they have several thousand MP3s, and they haven't come anywhere close to listening to all of them, which was the whole reason I wrote auto-DJ-crates in the first place.

One DJ friend of mine (the one I've been helping out) bought a system from a guy who was getting out of the business. Do you really think he's listened to all ~60,000 MP3s that it came with? He didn't even have them categorized. I did a quick pass over them, trying to find the songs that were obviously 1950's related, so that we could play them during the car show. I imported early versions of that playlist, and selected random tracks out of them, while I worked quickly to build a large playlist of relevant songs.

Maybe you're thinking of German DJs. In my experience, American DJs are nowhere near this hard-working or detail-oriented. :-)

Also, sometimes the venue has their own songs they'd like to play; that was the case for the surf contest I DJ'd years ago. I hardly knew any of the songs; I just played the categories that the boss wanted, and took requests.

"int iUnplayedTracks" is there for the reasons I explained earlier: to let a DJ get suggestions from a track collection that's too large to know, but that has at least been categorized into crates. Tracks that have never been played are a prime candidate for this sort of suggestion.

>For me it is fine for a clean GUI, to expose only one solution: the
>played flag or the always active ignore time.

At this point, I think it's important that it's more than just you and me deciding what the best solution is. I think you should merge the auto-DJ-crates branch with the trunk and let others play with it for a while. Then we can have a wide-ranging discussion about how it should evolve. I don't think the remaining issues should hang up the merge.

On Thu, Jun 6, 2013 at 8:32 PM, Steven Boswell <email address hidden> wrote:

> >For me it is fine for a clean GUI, to expose only one solution: the
> >played flag or the always active ignore time.
>
> At this point, I think it's important that it's more than just you and me
> deciding what the best solution is. I think you should merge the
> auto-DJ-crates branch with the trunk and let others play with it for a
> while. Then we can have a wide-ranging discussion about how it should
> evolve. I don't think the remaining issues should hang up the merge.

Hey Steven,

The way we do feature branches is that they need to be in release-shape
before they merge. I'll add your branch to the build server so you can
distribute win/mac/linux builds on e.g. the forums if you want input from
the community.

Thanks,
RJ

>The way we do feature branches is that they need to be in release-shape
>before they merge.

Given that the feature can be turned off from the scons command line, that it's not buggy, and that the direction of its future development will be driven heavily by opinion and experience, can't it be merged into the trunk?

If not...could you at least cherry-pick res/schema.xml and src/trackcollection.cpp, so that I can distribute builds to my DJ friends, knowing that their databases will be compatible with future versions of Mixxx?

Hi Steve,

"int iUnplayedTracks":

Your use case is a very valid one, that is not the problem. It would be fully covered, if you just use the the played flag or the ignore time, because the unplayed tracks are among the available tracks in both of this cases. Your approach just excludes the played tracks from your last car show before and I think that is not what the user expect, because there is no reason for not playing a track again because it was already played on a different gig.

--

I have also thought about the case if the count of available track falls below 1. For me it would be a better solution to fix this value to 1, instead of using all tracks.
This is of cause a corner case but it can take place if a user has 99 tracks and a Minimum of 1%.
The value 0 % should stay 0 because this would be a natural way to disable the minimum available feature.

--

It is allays a good idea to have a third opinion. I hope that Chis, the GSoC student will find time to review the code, because there might be dependences. But for my feeling we are just before to find solution that suits to both of us.

--

I am really thrilled by this feature and I would like to have it in the 1.12 release. But from the Mixxx point of view there is no need to hurry.

Kind regards,

Daniel

>Your approach just excludes the played tracks from your last car show
>before and I think that is not what the user expect, because there is
>no reason for not playing a track again because it was already played
>on a different gig.

It's what I would expect -- as I've said before, the point of auto-DJ-crates is to allow the DJ to explore the unexplored parts of his/her track collection.

And already-played tracks are not excluded -- they're simply de-emphasized in favor of never-played tracks.

>I have also thought about the case if the count of available track
>falls below 1. For me it would be a better solution to fix this value
>to 1, instead of using all tracks.

Done.

>from the Mixxx point of view there is no need to hurry.

If you'd be willing to cherry-pick the changes to res/schema.xml and src/trackcollection.cpp, so that I could distribute copies of Mixxx whose database will be compatible with other versions of Mixxx, then I'd agree with you. Any chance of this happening?

OK, You got me :-)
The usecases are now clear and valid to me.

But there are still some issues:

---

Do you have any Idea to improve the Ignore time flag presentation?
It sets the ignor time somehow to infinity. "Ingnore all played tracks"
We could replace the grayed out time with a suitable text ...

Please add [%] to
"Auto DJ: Minimum available tracks"

By the way: I had no tooltis in a fresh mixxx installation. They were enabled after a roundtrip of the tooltip settings. That must be a sepearte issue.

----

1397 + // If there are still no tracks to choose from, use one.
1398 + if (iActiveTracks == 0)
1399 + iActiveTracks = qMin(iTotalTracks, 1);
1400 +

should only effect the minimum available. I would like to have tis solution

int iMinAvailable = 0;
if (iActivePercentage) {
    // if minimum is not disabled, have a min of one at least
    iMinAvailable = qMax((iTotalTracks * iActivePercentage / 100), 1);
}
int iActiveTracks = qMax(iUnplayedTracks, iMinAvailable);

---

Please unify the Gui text, the config Key names and the variables "ActivePercentage" vs "Minimum Available".

---

Finaly I have no other objections. So IMHO It is a candiate for merging.
Do others have any objections?

>Do you have any idea to improve the Ignore time flag presentation? It
>sets the ignore time somehow to infinity. "Ignore all played tracks"
>We could replace the grayed out time with a suitable text ...

The ignore-time is effectively set to infinity if the checkbox is unchecked, since that prevents the time from being applied at all. And it doesn't ignore all played tracks; as I've said before, it merely de-emphasizes them. I'm not sure what you're asking for here.

>Please add [%] to "Auto DJ: Minimum available tracks"

Done.

>By the way: I had no tooltips in a fresh mixxx installation. They
>were enabled after a roundtrip of the tooltip settings. That must be
>a separate issue.

All I did was add a tooltip clause to the relevant parts of src/dlgprefcontrolsdlg.ui -- if that doesn't work on your end, I have no explanation.

>1397 + // If there are still no tracks to choose from, use one.
>1398 + if (iActiveTracks == 0)
>1399 + iActiveTracks = qMin(iTotalTracks, 1);
>1400 +
>
>should only affect the minimum available. I would like to have this
>solution
>
>int iMinAvailable = 0;
>if (iActivePercentage) {
> // if minimum is not disabled, have a min of one at least
> iMinAvailable = qMax((iTotalTracks * iActivePercentage / 100), 1);
>}
>int iActiveTracks = qMax(iUnplayedTracks, iMinAvailable);

That doesn't change the behavior -- iTotalTracks will only be zero if "iTotalTracks * iActivePercentage / 100" underflowed. Remember, "iTotalTracks" is the total active-tracks, not the total tracks in all connected crates.

>Please unify the Gui text, the config Key names and the variables
>"ActivePercentage" vs "Minimum Available".

I'm not sure what you're asking for here.

> >Do you have any idea to improve the Ignore time flag presentation? It
> >sets the ignore time somehow to infinity. "Ignore all played tracks"
> >We could replace the grayed out time with a suitable text ...
>
> The ignore-time is effectively set to infinity if the checkbox is unchecked,
> since that prevents the time from being applied at all. And it doesn't ignore
> all played tracks; as I've said before, it merely de-emphasizes them. I'm not
> sure what you're asking for here.
The GUI does not explain well what will happen. I am asking for improving that.

Currently we see:
Auto DJ: Minimum available tracks (percent)
[X] Auto DJ: Track ignore time

Both settings are working independently. If you Set "Minimum available" to 0 % and uncheck "[X]", actually all played tracks are ignored. The second setting really ignores tracks if the first setting is low enough.
But what is your idea to improve this issue?

>
> >1397 + // If there are still no tracks to choose from, use one.
> >1398 + if (iActiveTracks == 0)
> >1399 + iActiveTracks = qMin(iTotalTracks, 1);
> >1400 +
> >
> >should only affect the minimum available. I would like to have this
> >solution
> >
> >int iMinAvailable = 0;
> >if (iActivePercentage) {
> > // if minimum is not disabled, have a min of one at least
> > iMinAvailable = qMax((iTotalTracks * iActivePercentage / 100), 1);
> >}
> >int iActiveTracks = qMax(iUnplayedTracks, iMinAvailable);
>
> That doesn't change the behavior -- iTotalTracks will only be zero if
> "iTotalTracks * iActivePercentage / 100" underflowed. Remember,
> "iTotalTracks" is the total active-tracks, not the total tracks in all
> connected crates.
With my code sample it is allowed to set "Minimum available" to 0 % for have only "ignore time" working.
It was intended to place it above "if (m_bUseReplayAge) {" so it does also not effect the "ignore time".
The result is that there is still track selected if all tracks where played within the track ignore time.

> >Please unify the Gui text, the config Key names and the variables
> >"ActivePercentage" vs "Minimum Available".
>
> I'm not sure what you're asking for here.
It is simply misleading to have the same thing differently named. So please rename all "ActivePercentage" occurrences to Minimum Available.

Thank you,

Daniel

merge request has moved to https://github.com/mixxxdj/mixxx/pull/18