After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 565767 - Auto DJ
Auto DJ
Status: RESOLVED FIXED
Product: banshee
Classification: Other
Component: Playback
git master
Other All
: Normal enhancement
: 1.6
Assigned To: Alexander Kojevnikov
Banshee Maintainers
: 571970 (view as bug list)
Depends on:
Blocks: 553399
 
 
Reported: 2008-12-27 11:49 UTC by tim
Modified: 2009-09-10 02:19 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Auto DJ patch (13.22 KB, patch)
2008-12-27 12:10 UTC, tim
none Details | Review
using sqlite Random() function to add random tracks to play queue (13.32 KB, patch)
2008-12-27 20:45 UTC, tim
reviewed Details | Review
First iteration (21.28 KB, patch)
2009-05-11 06:47 UTC, Alexander Kojevnikov
none Details | Review
Screen-shot demonstrating the feature (197.56 KB, image/png)
2009-05-11 06:52 UTC, Alexander Kojevnikov
  Details
Updated to git master + new features (60.72 KB, patch)
2009-06-09 12:15 UTC, Alexander Kojevnikov
needs-work Details | Review
Changed population options + fixed flickering (65.56 KB, patch)
2009-06-10 07:05 UTC, Alexander Kojevnikov
none Details | Review
Population options in the config pane (222.54 KB, image/png)
2009-06-10 07:06 UTC, Alexander Kojevnikov
  Details
Don't remove songs from the PQ (68.35 KB, patch)
2009-06-12 10:37 UTC, Alexander Kojevnikov
none Details | Review
Updated to git master + added 'By Rating' population option (68.43 KB, patch)
2009-06-14 03:06 UTC, Alexander Kojevnikov
needs-work Details | Review
Screenshot showing space needed between queue config pane dropdowns (91.17 KB, image/png)
2009-06-16 19:33 UTC, Gabriel Burt
  Details
Misc fixes (69.46 KB, patch)
2009-06-17 03:42 UTC, Alexander Kojevnikov
needs-work Details | Review
Fixed a NRE (69.53 KB, patch)
2009-06-18 04:39 UTC, Alexander Kojevnikov
none Details | Review
Mockup sketch (34.83 KB, image/jpeg)
2009-06-18 22:29 UTC, Gabriel Burt
  Details
Fixed refresh when PQ is not playing (69.80 KB, patch)
2009-06-19 05:02 UTC, Alexander Kojevnikov
none Details | Review
Removed the config pane (75.09 KB, patch)
2009-07-28 09:03 UTC, Alexander Kojevnikov
none Details | Review
Play Queue options (134.33 KB, image/png)
2009-07-28 09:13 UTC, Alexander Kojevnikov
  Details
Updated to git master (75.22 KB, patch)
2009-08-01 00:27 UTC, Alexander Kojevnikov
none Details | Review
Screenshot showing the problem (202.66 KB, image/png)
2009-08-06 20:38 UTC, Michael Martin-Smucker
  Details
Updated to git master + fixes (76.45 KB, patch)
2009-08-10 02:42 UTC, Alexander Kojevnikov
needs-work Details | Review
debug output when banshee crashes (2.14 KB, text/plain)
2009-08-12 23:45 UTC, Michael Martin-Smucker
  Details
fixes (79.46 KB, patch)
2009-08-17 13:58 UTC, Alexander Kojevnikov
none Details | Review
more fixes (79.61 KB, patch)
2009-08-18 03:40 UTC, Alexander Kojevnikov
committed Details | Review

Description tim 2008-12-27 11:49:21 UTC
I have been using Media Monkey and Windows and one of the great features is Auto-DJ. When the all tracks have been played "Auto DJ" automatically adds random tracks from the whole library or from a play list.

Great feature that keeps the music playing and can reveal some great songs you haven't heard for a while.
Comment 1 tim 2008-12-27 12:10:22 UTC
Created attachment 125376 [details] [review]
Auto DJ patch

Patch add more options to play queue:
* Auto DJ
* Random Tracks


When auto dj is turned on 5 new randomly selected tracks are added to the play queue when all songs have been played.

Random Tracks adds 5 random tracks to the play queue.

Fixed the references in MonoDevelop.
Comment 2 Bertrand Lorentz 2008-12-27 14:10:59 UTC
Thanks for the patch.

Please use spaces for indentation. Check out the HACKING file for all code formatting guidelines.

I think it would be better to get the random tracks through an SQL query, using "order by random", like what is done for the shuffle mode.
Comment 3 tim 2008-12-27 20:45:31 UTC
Created attachment 125401 [details] [review]
using sqlite Random() function to add random tracks to play queue
Comment 4 tim 2008-12-27 20:47:30 UTC
Comment on attachment 125401 [details] [review]
using sqlite Random() function to add random tracks to play queue

tabs also replaced with spaces.
Comment 5 Gabriel Burt 2009-01-13 22:20:06 UTC
Thanks for the patch Tim.  This is a good start for adding some nice features to the Play Queue.  Eventually I'd like to see it add tracks not just randomly but based on suggestions from Mirage or Last.fm.
Comment 6 Alexander Kojevnikov 2009-03-09 10:49:00 UTC
*** Bug 571970 has been marked as a duplicate of this bug. ***
Comment 7 Alexander Kojevnikov 2009-03-09 11:00:28 UTC
(In reply to comment #5)
> Thanks for the patch Tim.  This is a good start for adding some nice features
> to the Play Queue.  Eventually I'd like to see it add tracks not just randomly
> but based on suggestions from Mirage or Last.fm.
> 
We could add an option to randomly populate the play queue from a playlist, the entire library, or just any source (as suggested in the duplicate bug 571970).

Bug 553399 is related to this one, we can use the same config panel to specify the source to populate the play queue from and the number of previous and next tracks to show (see the attachment in bug 571970)
Comment 8 Alexander Kojevnikov 2009-05-11 06:47:57 UTC
Created attachment 134374 [details] [review]
First iteration

This patch extends patch 133623 from bug 553399, it should be applied on top of it.

The patch adds four controls to the PlayQueue's configuration pane:

 * A check box that turns on the random population of the play queue.
 * A combo box with the list of sources to populate from.
 * The number of upcoming tracks.
 * A refresh button that re-populates the play queue from the selected source.

I will attach a screen-shot demonstrating the feature.
Comment 9 Alexander Kojevnikov 2009-05-11 06:52:34 UTC
Created attachment 134376 [details]
Screen-shot demonstrating the feature
Comment 10 Alexander Kojevnikov 2009-06-09 12:15:40 UTC
Created attachment 136209 [details] [review]
Updated to git master + new features

This patch includes the patch from bug 553399.

Changes compared to the previous version:

* Play Queue now supports all new shuffle modes: by song, by album, by artist and by rating (available as a patch in bug 544680). When shuffle is off, the play queue behaves like it is now. When shuffle is enabled, the play queue will be populated from the selected source using the specified shuffle algorithm.

* Play Queue can now distinguish tracks manually added to it from those that were auto-generated. Hitting Refresh will replace only the generated tracks. Also, manually added tracks will be inserted before any generated tracks.

* I also fixed a couple of minor bugs.

I've been using this patch for a few months now and I must say I'm pretty happy with how it works. I encourage everyone interested in this feature to try it out and report any bugs you find.

Cheers,
Alex
Comment 11 Gabriel Burt 2009-06-09 19:04:44 UTC
Very cool Alexander, thanks!

Some feedback from testing this patch out:

1. With my Play Queue empty, I switched to it, and expected to be able to change the 'Randomly populate' option, but it's not sensitive.  I didn't expect and found it quite confusing that you need to change the Random option to enable this.

Maybe just disable the random-playback options (to not mislead and make the user think it will play what is in the Queue in a random fashion) and just duplicate the dropdown/menu within the config pane?  Would be great, of course, to also have suggested-by-[last.fm/mirage] options - but that can be done separately, and maybe even as part of the normal random-playback options too.

2. Hitting next in the play queue, I notice that it redraws w/ the 10 not-enabled tracks drawn opaque, then again w/ them drawn !opaque - gives a flashing/sluggish feel to it.

Here's another idea, wouldn't have to be done as part of this patch, though maybe would be best:  never actually remove items from the play queue join-table (unless the user explicitly asks us to) - then only show the last 10 that aren't enabled (as you're doing now) - this would 1) let the user adjust the # of recently played tracks and have it update in real time  2) give us the ability to save the entire play queue as a playlist
Comment 12 Alexander Kojevnikov 2009-06-10 07:05:11 UTC
Created attachment 136255 [details] [review]
Changed population options + fixed flickering

(In reply to comment #11)
> Very cool Alexander, thanks!
> 
> Some feedback from testing this patch out:
> 
> 1. With my Play Queue empty, I switched to it, and expected to be able to
> change the 'Randomly populate' option, but it's not sensitive.  I didn't expect
> and found it quite confusing that you need to change the Random option to
> enable this.
> 
> Maybe just disable the random-playback options (to not mislead and make the
> user think it will play what is in the Queue in a random fashion) and just
> duplicate the dropdown/menu within the config pane?

I agree. This patch disables shuffle options for the play queue (again) and adds a "Populate by X" combobox to the config pane. I will attach a screen-shot.

> Would be great, of course,
> to also have suggested-by-[last.fm/mirage] options - but that can be done
> separately, and maybe even as part of the normal random-playback options too.

Is it the same as bug 372396? That would make a neat feature.

> 2. Hitting next in the play queue, I notice that it redraws w/ the 10
> not-enabled tracks drawn opaque, then again w/ them drawn !opaque - gives a
> flashing/sluggish feel to it.

I noticed this a few times, it's a timing issue and my PC is probably too fast so I didn't see it often :) Anyway, the underlying problem should be fixed in this patch, could you check?

> Here's another idea, wouldn't have to be done as part of this patch, though
> maybe would be best:  never actually remove items from the play queue
> join-table (unless the user explicitly asks us to) - then only show the last 10
> that aren't enabled (as you're doing now) - this would 1) let the user adjust
> the # of recently played tracks and have it update in real time  2) give us the
> ability to save the entire play queue as a playlist
> 

I like the idea. Shouldn't be too hard to implement now that the play queue implements a custom TrackListModel.

Cheers,
Alex
Comment 13 Alexander Kojevnikov 2009-06-10 07:06:24 UTC
Created attachment 136256 [details]
Population options in the config pane

...as implemented in the last patch
Comment 14 Alexander Kojevnikov 2009-06-12 10:37:32 UTC
Created attachment 136410 [details] [review]
Don't remove songs from the PQ

(In reply to comment #12)
> (In reply to comment #11)
> > Here's another idea, wouldn't have to be done as part of this patch, though
> > maybe would be best:  never actually remove items from the play queue
> > join-table (unless the user explicitly asks us to) - then only show the last 10
> > that aren't enabled (as you're doing now) - this would 1) let the user adjust
> > the # of recently played tracks and have it update in real time  2) give us the
> > ability to save the entire play queue as a playlist
> > 
> 
> I like the idea. Shouldn't be too hard to implement now that the play queue
> implements a custom TrackListModel.

This patch does it. Songs are never removed from the playlist associated with the play queue, but rather offset so that only the specified number of played songs are visible.
Comment 15 Alexander Kojevnikov 2009-06-14 03:06:21 UTC
Created attachment 136535 [details] [review]
Updated to git master + added 'By Rating' population option
Comment 16 Gabriel Burt 2009-06-16 19:32:52 UTC
Some more comments:

1. PlayQueueTrackListModel.cs's header has the wrong filename

2. IsQueueable should probably only check if is DatabaseSource, no?  Actually, even better would be to just call play_queue.AcceptsInputFromSource

3. the two dropdowns in the config pane need some spacing - will attach a sshot

4. I queued maybe 7 songs, then changed the random-from source to a smart playlist, [maybe hit refresh, may have happened immediately] and I think all but the first/playing manually-queued song were replaced by random ones.

5. What do you think about moving the Refresh button to the toolbar next to Clear (or, perhaps, replacing it when Random != Off)?

6. I think when the user hits Refresh, we should pass the skipped=true arg to the track model GetRandom function, so that if it had tracks 1 - 10 of an album, pressing refresh doesn't bring in track 11 first like it currently does if random-by-album on.
Comment 17 Gabriel Burt 2009-06-16 19:33:50 UTC
Created attachment 136762 [details]
Screenshot showing space needed between queue config pane dropdowns
Comment 18 Alexander Kojevnikov 2009-06-17 01:36:38 UTC
Gabriel, thanks for reviewing it! I will address these issues in the next patch.

Swapping dependencies with bug 553399 as I now mainly work on this bug's branch. If you want to finalise bug 553399 before this one, I can prepare a cherry-picked patch.
Comment 19 Alexander Kojevnikov 2009-06-17 03:42:26 UTC
Created attachment 136796 [details] [review]
Misc fixes

(In reply to comment #16)
> Some more comments:
> 
> 1. PlayQueueTrackListModel.cs's header has the wrong filename

Fixed.

> 2. IsQueueable should probably only check if is DatabaseSource, no?  Actually,
> even better would be to just call play_queue.AcceptsInputFromSource

Well, support for other sources would make a great feature which I think should be added as a separate bug. At the moment the play queue is not really compatible with anything other than the Music and Video libraries and their playlists (see an old TODO on line 100 in PlayQueueSource.cs)

The TracksChanged/Deleted handlers is only part of the problem, the other part is proper GetRandom() implementation by the sources. The Podcasting source works fine in my tests while the Last.fm's GetRandom() needs major tweaking. I didn't try other sources but I would say random population of Radio streams is rather pointless - they never stop.

> 3. the two dropdowns in the config pane need some spacing - will attach a sshot

Added a 6px spacing as suggested by the HIG.

> 4. I queued maybe 7 songs, then changed the random-from source to a smart
> playlist, [maybe hit refresh, may have happened immediately] and I think all
> but the first/playing manually-queued song were replaced by random ones.

I cannot reproduce this, the PQ keeps the manually added songs for me when I hit Refresh or change the population source. Could you post the exact steps?

> 5. What do you think about moving the Refresh button to the toolbar next to
> Clear (or, perhaps, replacing it when Random != Off)?

Great idea! Clear in population mode really doesn't make much sense, the patch replaces it with the Refresh button. It also updates the context menu.

> 6. I think when the user hits Refresh, we should pass the skipped=true arg to
> the track model GetRandom function, so that if it had tracks 1 - 10 of an
> album, pressing refresh doesn't bring in track 11 first like it currently does
> if random-by-album on.

Agree, the patch fixes this.
Comment 20 Gabriel Burt 2009-06-17 19:39:50 UTC
I got a crash trying to manually remove all the songs from the queue:

System.NullReferenceException: Object reference not set to an instance of an object
  at Banshee.PlayQueue.PlayQueueTrackListModel.get_Item (Int32 index) [0x00016] in /home/gabe/Projects/banshee/src/Extensions/Banshee.PlayQueue/Banshee.PlayQueue/PlayQueueTrackListModel.cs:52 
  at Hyena.Data.Gui.ListView`1[T].PaintRow (Int32 row_index, Rectangle area, StateType state) [0x00000] 
  at Hyena.Data.Gui.ListView`1[T].PaintRows (Rectangle clip) [0x00000] 
  at Hyena.Data.Gui.ListView`1[T].OnExposeEvent (Gdk.EventExpose evnt) [0x00000] 
  at Gtk.Widget.exposeevent_cb (IntPtr widget, IntPtr evnt) [0x0000d] in /home/gabe/Projects/gtk-sharp-2-12-branch/gtk/generated/Widget.cs:1979 

Restarting the app, the queue is empty.  The crash is reproducible, doesn't seem to happen if I only remove one track not all.

Right before triggering the crash, I thought I selected 9 songs in my library and (with the queue empty) hit 'q' and only one (the last one) was added, but I'm not able to reproduce that atm.
Comment 21 Alexander Kojevnikov 2009-06-18 04:39:25 UTC
Created attachment 136890 [details] [review]
Fixed a NRE

(In reply to comment #20)
> I got a crash trying to manually remove all the songs from the queue:
> ...

I cannot reproduce the crash but the null check was indeed required, the patch adds it.
Comment 22 Gabriel Burt 2009-06-18 18:34:40 UTC
I have it set to populate by-artist, but when I click Refresh, the first track never changes, but all the others do (and are correct)
Comment 23 Gabriel Burt 2009-06-18 18:38:58 UTC
Note that I'm not playing from the queue when this happens (if I were, then it would make sense to me that the first/playing song wouldn't get removed)
Comment 24 Gabriel Burt 2009-06-18 22:29:03 UTC
Created attachment 136957 [details]
Mockup sketch

Here's a rough sketch of an idea I had.  We could get rid of the config pane entirely, putting the random-by and source dropdowns in the spot where the search bar usually is, and we could move the show-#-tracks options in the Play Queue's preferences.  Thoughts?
Comment 25 Gabriel Burt 2009-06-18 22:34:30 UTC
The [by track] dropdown would have a 'manual' entry (instead of 'Off'), which would disable the second dropdown.
Comment 26 Alexander Kojevnikov 2009-06-19 04:16:05 UTC
(In reply to comment #24)
> Created an attachment (id=136957) [edit]
> Mockup sketch
> 
> Here's a rough sketch of an idea I had.  We could get rid of the config pane
> entirely, putting the random-by and source dropdowns in the spot where the
> search bar usually is, and we could move the show-#-tracks options in the Play
> Queue's preferences.  Thoughts?

I like this, saves a lot of screen real-estate. I will update the patch in the coming days.
Comment 27 Alexander Kojevnikov 2009-06-19 05:02:42 UTC
Created attachment 136972 [details] [review]
Fixed refresh when PQ is not playing

(In reply to comment #22)
> I have it set to populate by-artist, but when I click Refresh, the first track
> never changes, but all the others do (and are correct)
> 

This patch should fix it.
Comment 28 Alexander Kojevnikov 2009-07-28 09:03:50 UTC
Created attachment 139365 [details] [review]
Removed the config pane

This patch completely removes the config pane as suggested by Gabriel in comment #24. The fill by X from Y is moved to the header and the number of played and upcoming tracks is moved to the preferences dialogue.

I will attach the screen-shot.

I'm also going to commit new strings used by this patch in anticipation of the forthcoming string freeze.
Comment 29 Alexander Kojevnikov 2009-07-28 09:13:03 UTC
Created attachment 139366 [details]
Play Queue options
Comment 30 Alexander Kojevnikov 2009-08-01 00:27:15 UTC
Created attachment 139661 [details] [review]
Updated to git master
Comment 31 Bertrand Lorentz 2009-08-01 13:45:56 UTC
The patch seems to works well, but I think I've spotted a potential performance issue :
For example, if you select "Fill by rating from Unheard", when banshee generates the list of the 10 tracks to play next, it will take all CPU for several seconds.
The easiest way to spot that is to set the options as indicated above, generate a playqueue, and then double-click on the last track. The UI then "lags" for a few seconds.

I've tracked it down to the "random by slot" query :

[Debug 12:38:02.335] Executed in 271ms 
SELECT
    (CoreTracks.Rating - 1) AS Slot, COUNT(*)
FROM
    CoreTracks, CoreCache , CoreSmartPlaylistEntries
WHERE
    CoreCache.ItemId = CoreTracks.TrackID AND
    CoreCache.ModelID = 41 AND
    CoreTracks.LastStreamError = 0 AND
    (CoreTracks.LastPlayedStamp < 1249126656 OR CoreTracks.LastPlayedStamp IS NULL) AND
    (CoreTracks.LastSkippedStamp < 1249126656 OR CoreTracks.LastSkippedStamp IS NULL)
    AND  CoreSmartPlaylistEntries.TrackID = CoreTracks.TrackID AND CoreSmartPlaylistEntries.SmartPlaylistID = 4 
GROUP BY Slot

It's executed 10 times in the scenario I described (once for each track), causing the high CPU usage.

If you select "from the Music library", instead of "from <a smart playlist>", the problem isn't noticeable.
Comment 32 Alexander Kojevnikov 2009-08-02 08:46:23 UTC
Re-adding the CoreSmartPlaylistEntriesIndex speeds up the query quite a bit, but probably introduces bug 581103 again (even though I cannot reproduce it). I will investigate this further this week.
Comment 33 Michael Martin-Smucker 2009-08-06 20:38:09 UTC
Created attachment 140060 [details]
Screenshot showing the problem

While testing this patch, I noticed that it seems to incorrectly update Last Played information for tracks when they are added to the queue rather than when they finish playing.  Notice in the screenshot how all tracks say they were played today, including the track under my mouse pointer which has a play count of zero.

As a new user of the patch, I have to point out that it isn't immediately obvious to me what "Fill by Some-Criteria" means.  Fill by Rating is especially confusing, as the songs it chooses for the queue seem to have no connection.

Other than that, I'm absolutely loving this patch.  Thanks for your work.
Comment 34 Alexander Kojevnikov 2009-08-09 07:30:43 UTC
(In reply to comment #33)
> While testing this patch, I noticed that it seems to incorrectly update Last
> Played information for tracks when they are added to the queue rather than when
> they finish playing.  Notice in the screenshot how all tracks say they were
> played today, including the track under my mouse pointer which has a play count
> of zero.

This is kind of intentional, if we don't update the LastPlayed timestamp for the tracks added to the queue, the shuffle algorithm won't work correctly. For example, the "by artist" and "by album" modes will return the same song over and over because the shuffler uses LastPlayed timestamp to find the last added song from the artist/album.

I agree that the side-effect of such behaviour can be annoying in some scenarios. Unfortunately I don't see how to fix this elegantly. We could eventually split the LastPlayedStamp into two stamps: one for the actual playback, the other one - for the shuffler, but I'm not too fond of this change. Any ideas are more than welcome!

> As a new user of the patch, I have to point out that it isn't immediately
> obvious to me what "Fill by Some-Criteria" means.  Fill by Rating is especially
> confusing, as the songs it chooses for the queue seem to have no connection.

It's basically "fill by song" but preferring higher rated songs. Same for "by score". They are also available as shuffle modes, see bug 544680 and bug 585613. I agree these shuffle modes are a bit confusing, feel free to suggest documenting them here: http://mail.gnome.org/archives/banshee-list/2009-July/msg00089.html

> Other than that, I'm absolutely loving this patch.  Thanks for your work.

Thanks for testing! ;)
Comment 35 Alexander Kojevnikov 2009-08-10 02:42:20 UTC
Created attachment 140291 [details] [review]
Updated to git master + fixes
Comment 36 Michael Martin-Smucker 2009-08-10 13:09:06 UTC
(In reply to comment #34)
> 
> This is kind of intentional...
> 
> I agree that the side-effect of such behaviour can be annoying in some
> scenarios. Unfortunately I don't see how to fix this elegantly.

Indeed a bit unfortunate, but understandable.  I actively remove songs from the queue without playing them, so this is starting to throw off several of my smart playlists that depend on Last Played data.

> It's basically "fill by song" but preferring higher rated songs. Same for "by
> score". They are also available as shuffle modes, see bug 544680 and bug
> 585613.

Bug 588889 proposes separating the criteria Song/Album/Artist from the weighting Score/Rating/(maybe play count).  Personally, I think that would make the shuffling options less confusing; maybe Auto DJ could benefit as well.

> I agree these shuffle modes are a bit confusing, feel free to suggest
> documenting them here:
> http://mail.gnome.org/archives/banshee-list/2009-July/msg00089.html

Thanks for the link - a chance to feel useful even with my worthless C# skills! I'll see what I can do.

One last thing - I noticed this in v10 as well as the latest version of this patch - when fill is set to Manual, I can't actually add songs to the Play Queue from the Music Library (probably other sources too?). I'm assuming the problem is related to this patch, but I could be wrong.
Comment 37 Alexander Kojevnikov 2009-08-11 02:05:56 UTC
(In reply to comment #36)
> One last thing - I noticed this in v10 as well as the latest version of this
> patch - when fill is set to Manual, I can't actually add songs to the Play
> Queue from the Music Library (probably other sources too?). I'm assuming the
> problem is related to this patch, but I could be wrong.

I cannot reproduce this, is there anything special that you are doing?

The songs are added after the current track and all manually added tracks, but before all automatically added tracks. So, if your play queue looks like this:

-2 played song
-1 played song
>0 current song
 1 manually added
 2 manually added
 3 auto added
 4 auto added
 5 auto added

...adding songs from the Music Library will insert them between songs 2 and 3.
Comment 38 Michael Martin-Smucker 2009-08-11 03:12:07 UTC
The problem had been happening with a completely empty queue, and the problem persisted through multiple restarts of Banshee, and even rebuilding the latest git + this patch.

After reading that you couldn't reproduce the problem, I switched switched back to Auto-DJ, then back to manual, then cleared the queue, and now manually adding songs is working fine.  If it happens again, I'll try to come up with more detailed steps to reproduce it, but for now it doesn't seem reproducible.
Comment 39 Gabriel Burt 2009-08-12 20:07:17 UTC
"Fill by Rating" could be "Fill by Preferring Higher Rated Songs".

Or, if the split between song/album/artist and rating/score ever happens (not likely to soon), "Fill by [Song|Album|Artist] [randomly|preferring higher rated|preferring higher score]"
Comment 40 Gabriel Burt 2009-08-12 20:14:04 UTC
Or, even better:

Fill [randomly|preferring high-rated] with [songs|albums|artists]

Alex, what do you think about creating a new table PlayQueueShuffle or something w/ TrackID and LastShuffledAt, and modifying the RandomBy* classes to accept an optional join table and to specify the datetime column(s)?
Comment 41 Gabriel Burt 2009-08-12 20:16:46 UTC
The table could have a third column specifying who's using it, so other plugins etc could reuse it.  Could call it CoreTrackShuffles (ShufflerID, TrackID, LastShuffledAt)
Comment 42 Gabriel Burt 2009-08-12 20:34:24 UTC
1. should change Fill [Manual] to [Manually] in this version

2. Can you try to modify the source combo box so that when it's not sensitive the souce's icon is drawn insensitive (usually means greyed out too, I think).

3. If I change from manual to by song, it fills with 10 random songs; good.  If I then change it to by artist, it leaves the 10 random songs, and appends one new song (now have 11 songs in the queue).  If I push Refresh, things look right (10 songs from an artist are all that's shown).  If I then change to 'by album' the same bad behavior happens; the 10 songs from the artist remain, and one new song is appended.

4. Add Preference context menu item for Play Queue source

5. The track-selection-required actions don't seem to get updated sensitivity-wise on Refresh;  I can still Edit Track Information even though I have no tracks selected.
Comment 43 Michael Martin-Smucker 2009-08-12 23:14:06 UTC
(In reply to comment #40)
> 
> Fill [randomly|preferring high-rated] with [songs|albums|artists]
> 
I like this; it reads very naturally.  Would the "manually" option fit in with the same drop-down as randomly/preferring?

Also, with this patch I'm noticing a general unresponsiveness in the Play Queue.  I've attached a video showing the difference between the Music Library source and the Play Queue source.  Moving my mouse over the Rating column instantly gives me the option to rate each song in the Music library, but the stars lag significantly behind my mouse in the Play Queue.  Also, dragging and dropping to reorder items in the queue also seems slower and less responsive.

...it looks like my video was a little too big, so I've uploaded it here:
http://www.martinsmucker.com/files/auto-dj-unresponsive.ogv
Comment 44 Alexander Kojevnikov 2009-08-12 23:30:21 UTC
(In reply to comment #43)
> Also, with this patch I'm noticing a general unresponsiveness in the Play
> Queue.  I've attached a video showing the difference between the Music Library
> source and the Play Queue source.  Moving my mouse over the Rating column
> instantly gives me the option to rate each song in the Music library, but the
> stars lag significantly behind my mouse in the Play Queue.  Also, dragging and
> dropping to reorder items in the queue also seems slower and less responsive.

Which version of the patch are you using? The slowness should be fixed in v11.
Comment 45 Michael Martin-Smucker 2009-08-12 23:45:58 UTC
Created attachment 140600 [details]
debug output when banshee crashes

v11 is the one I'm using, and it's the only patch that I've applied to a version of git that I cloned a couple hours ago.

As far as drag-and-drop goes, I can actually consistently produce a crash by starting to play a song in the Play Queue, then  dragging a different song to the bottom of the queue.  The UI freezes for a couple seconds, then Banshee crashes.  I can produce this 100% of the time (when recordmydesktop isn't running... figures).  I've attached the part of the terminal output that seems relevant.
Comment 46 Alexander Kojevnikov 2009-08-17 07:12:37 UTC
(In reply to comment #40)
> Alex, what do you think about creating a new table PlayQueueShuffle or
> something w/ TrackID and LastShuffledAt, and modifying the RandomBy* classes to
> accept an optional join table and to specify the datetime column(s)?

(In reply to comment #41)
> The table could have a third column specifying who's using it, so other plugins
> etc could reuse it.  Could call it CoreTrackShuffles (ShufflerID, TrackID,
> LastShuffledAt)

I like this, it would elegantly fix the LastPlayedStamp issue. Adding to my todo list...
Comment 47 Alexander Kojevnikov 2009-08-17 13:58:22 UTC
Created attachment 140964 [details] [review]
fixes

(In reply to comment #42)
> 1. should change Fill [Manual] to [Manually] in this version

Done.

> 2. Can you try to modify the source combo box so that when it's not sensitive
> the souce's icon is drawn insensitive (usually means greyed out too, I think).

I ported the code from gtk_cell_renderer_pixbuf_render(), seems to work fine.

> 3. If I change from manual to by song, it fills with 10 random songs; good.  If
> I then change it to by artist, it leaves the 10 random songs, and appends one
> new song (now have 11 songs in the queue).  If I push Refresh, things look
> right (10 songs from an artist are all that's shown).  If I then change to 'by
> album' the same bad behavior happens; the 10 songs from the artist remain, and
> one new song is appended.

Fixed.

> 4. Add Preference context menu item for Play Queue source

Done.

> 5. The track-selection-required actions don't seem to get updated
> sensitivity-wise on Refresh;  I can still Edit Track Information even though I
> have no tracks selected.

Fixed.

(In reply to comment #45)
> As far as drag-and-drop goes, I can actually consistently produce a crash by
> starting to play a song in the Play Queue, then  dragging a different song to
> the bottom of the queue.  The UI freezes for a couple seconds, then Banshee
> crashes.  I can produce this 100% of the time (when recordmydesktop isn't
> running... figures).  I've attached the part of the terminal output that seems
> relevant.

Thanks Michael, I can reproduce the crash. I will check it later this week.
Comment 48 Alexander Kojevnikov 2009-08-18 03:40:00 UTC
Created attachment 141024 [details] [review]
more fixes

(In reply to comment #45)
> v11 is the one I'm using, and it's the only patch that I've applied to a
> version of git that I cloned a couple hours ago.
> 
> As far as drag-and-drop goes, I can actually consistently produce a crash by
> starting to play a song in the Play Queue, then  dragging a different song to
> the bottom of the queue.  The UI freezes for a couple seconds, then Banshee
> crashes.  I can produce this 100% of the time (when recordmydesktop isn't
> running... figures).  I've attached the part of the terminal output that seems
> relevant.

This patch should fix both issues, please test.
Comment 49 Michael Martin-Smucker 2009-08-18 12:36:48 UTC
> This patch should fix both issues, please test.

The play queue is snappy and responsive as expected, and the crash is gone.  Nice work.

> One last thing - I noticed this in v10 as well as the latest version of this
> patch - when fill is set to Manual, I can't actually add songs to the Play
> Queue from the Music Library (probably other sources too?). I'm assuming the
> problem is related to this patch, but I could be wrong.

And just for some peace of mind, I figured out what was causing this...  I've been testing git + patches with "make run", but I haven't actually installed over 1.5.0.  If I use this patch for awhile, then accidentally launch Banshee 1.5.0 and use the play queue, sometimes I can't add music manually when I switch back to git.  Definitely not something that will happen during normal use of Banshee.
Comment 50 Gabriel Burt 2009-09-09 18:37:46 UTC
Looking pretty good.  One quirk I noticed:

Related, bug separate symptoms at least:  1) If I drag a song into the queue (filled by rating (not manual)), hit Refresh, and then drag the manually-added song down, the songs above it are greyed out.  I hadn't played anything since starting the app.  If I drag it back up, they are un-greyed out.  2)

Even more minor quirk:  The manually-added song was also inserted 2nd in the list when I dropped it on the queue, not first, even though I wasn't playing anything.

Besides those quirks, it seems to be working great.  This is such an awesome feature, Alexander!
Comment 51 Gabriel Burt 2009-09-09 18:54:45 UTC
Alexander, if you want to commit that last patch and work on any bug fixes on top of it, that's fine with me.
Comment 52 Alexander Kojevnikov 2009-09-10 01:36:25 UTC
(In reply to comment #50)
> Looking pretty good.  One quirk I noticed:
> 
> Related, bug separate symptoms at least:  1) If I drag a song into the queue
> (filled by rating (not manual)), hit Refresh, and then drag the manually-added
> song down, the songs above it are greyed out.  I hadn't played anything since
> starting the app.  If I drag it back up, they are un-greyed out.  

Well, this is kind of intentional. The song you are dragging is the current one, everything before it is always grey, even after dragging. I changed the behaviour to make the next unselected song the current one, if the dragged selection contains the current song AND it's not yet playing. Let me know if you have something else in mind.

> 2) 
> Even more minor quirk:  The manually-added song was also inserted 2nd in the
> list when I dropped it on the queue, not first, even though I wasn't playing
> anything.

Fixed
Comment 53 Alexander Kojevnikov 2009-09-10 02:12:44 UTC
(In reply to comment #51)
> Alexander, if you want to commit that last patch and work on any bug fixes on
> top of it, that's fine with me.

The patch has been committed, yay! :D

Thanks everyone for all the testing, that was a long road. Please file new bug reports if you notice any issues.

I also opened bug 594701 for the issue discussed in comments 33, 34, 40 and 41.
Comment 54 Michael Martin-Smucker 2009-09-10 02:16:41 UTC
Would it make sense to make a new component to keep track of auto-dj bugs?  Or will there not be enough to make it worth it? :-)
Comment 55 Alexander Kojevnikov 2009-09-10 02:19:23 UTC
(In reply to comment #54)
> Would it make sense to make a new component to keep track of auto-dj bugs?  Or
> will there not be enough to make it worth it? :-)

Haha, good one :)

Let's wait and see, if there are more than a few it probably makes sense to add a new component.