Hmm. 1.3.3 has a way to sort the playlists from the context menu of the Music Library.

Reopening. The order is not preserved between restarts. Moreover, the UI is a bit clumsy (well hidden in a context menu, while no functionality should be accessible from a context menu only).

(In reply to comment #2)
> Moreover, the UI is a
> bit clumsy (well hidden in a context menu, while no functionality should be
> accessible from a context menu only).

I'd think Alphabetical sort order would be a good default. Also, I filed bug 558423 to capture the lack of "memory" for the sort order.

Associated issue: When creating new playlist, it appears at the top of the list. After restart, the position is changed. It should appear at that position right after creation (user won't have troubles finding it, since there is an animation making it easy to spot).

Created attachment 127741 [details] [review]
Keep playlists sorted by name, and manual/smart status

Created attachment 127742 [details] [review]
Keep playlists sorted by name

Alternative to the first patch. Changes behavior to ignore distinctions between manual and smart playlists. I made it a separate patch so it'll be easier for a core dev to decide which one they like.

Hey John, one thing to note, instead of this:

pl.Renamed += new SourceRenamedHandler (OnChildRenamed);

you can just write

pl.Renamed += OnChildRenamed;

I think maybe C# 1.0 didn't allow the latter.

John, Source's Rename method already raises the Source.Updated event - so let's use that as our trigger for resorting.  That way we can have sort-by-count and otehrs and trigger them all off that.

Also, there is already a NameComparer class in Source - you can change it to be case insensitive if you want (probably a good idea).

Instead of modifying PrimarySource, let's make the change in Source.cs itself - change AddChildSource to listen for Updated on each child (and to trigger an immediately resort) and stop listening in RemoveChildSource.

It would be good to have a way for sources to identify the ways that their children can be sorted.  Name and Size are pretty common, but Last.fm has Play Count and Station Type too - would be good if Sources could specify the ways they can be sorted, and the Gtk# context menus/items etc would all be created automatically/consistently.

So you could have something like
struct SourceSortType
{
    public IComparer<Source> Comparer;
    public string Name;  // a not-translated string we can store in gconf to remember the user's pref
    public string Label; // the translated name we show in the context menu
    public bool? ForceDirection; // null if asc/desc are options, or true/false if always forced to asc(true) or desc
}

SourceSortType name = new SourceSortType (new NameComparer (), "name", Catalog.GetString ("name"), null);
SourceSortType station_type = new SourceSortType (new StationTypeComparer (), "station-type", Catalog.GetString ("Station Type"), true);

I like how the Last.fm source has an icon, the extra " by", and its sort menuitems are radio items.

See src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Radio/LastfmSource.cs for how Last.fm currently does it.

Given all the above, you could then put Name and Count as default options for LibrarySources (Music/Video/Podcast).  Maybe could work in a checkbox pref for whether to group by type - maybe could be generic and that's what Last.fm would use too.

Created attachment 128752 [details] [review]
Keep playlists sorted when inserting or renaming them

Resort playlists (in fact, all sources) automatically based on `Updated` events from their children.

A few notes on this patch, where I modified classes not directly related to sorting:

> Core/Banshee.ThickClient/Banshee.Sources.Gui/CellEditEntry.cs
The `EditingDone` signal was being sent twice in some cases. Normally this isn't a problem, but with playlists reordered on rename it was causing two playlists to be renamed at once. Now each editor will only fire the signal once.

> Core/Banshee.ThickClient/Banshee.Sources.Gui/SourceView.cs
The `SourceView` immediately updated the `SourceModel`'s order field when a source was modified. With updates to the sort order now coming in immediately after source creation, this caused assertions to be raised in `gtk_tree_view_bin_expose()`. I modified the source view to set a flag indicating resorting is needed, and then resorting the entire model after the next expose event.

> Core/Banshee.ThickClient/Resources/core-ui-actions-layout.xml
Cosmetic change to put the "sort children" menu in library sources in roughly the same place as in Last.FM, which I think looks nicer.

> It would be good to have a way for sources to identify the ways that their
> children can be sorted.  Name and Size are pretty common, but Last.fm has Play
> Count and Station Type too - would be good if Sources could specify the ways
> they can be sorted, and the Gtk# context menus/items etc would all be created
> automatically/consistently.
This patch has a beginning on this, but generating dynamic menus in a GtkAction-based UI turned out to be tremendously difficult. The only working examples I could find relied on being able to override `Gtk.Action:CreateMenuItem()`, which is not possible in Gtk#. I found two upstream bug reports regarding this, <https://bugzilla.novell.com/show_bug.cgi?id=381744> and <https://bugzilla.novell.com/show_bug.cgi?id=318131>. Once those are solved, dynamic sorting menus will be possible to implement.

> a not-translated string we can store in gconf to remember the user's pref
There's a separate bug about this, bug 558423. It will probably be easier to use integer indexes of the active sort order, like the Last.FM plugin currently does.

(In reply to comment #0)
> Sorting playlists by name would be much more useful. Grouping manual
> and automatic playlists is *not* desirable (it seems to work that way now),
> since as a user I do not care about that when selecting the playlist I want to
> listen to.
> 

Just to throw in my two cents from a "normal user" point of view:  I personally like grouping manual and smart playlists separately (visually I like that the icons go together, plus it's just what I'm used to).  Within those two divisions, I would prefer grouping the playlists alphabetically.

John,

We generate a dynamic menu for 'Add to Playlist', see TrackActions.cs.  Let's use a id/string based key to store which sort in gconf, too - indexes are too fragile/inhibit future flexibility.

Does anybody use "Name Descending"?  I think we should just remove it; makes finding what 99% of people really want (sort by name) much easier too.

Source should probably default to an empty list of sort optiosn; the Name and Size should probably only be set as default for PrimarySources.  Let's make the default SourceSortType objects static - no reason to have multiple ones around; and maybe make them public static readonly so we can pick and choose which to reuse in Last.fm etc.

Instead of having ActiveChildSortIndex let's just have a ActiveChildSort property that returns the actual SourceSortType object (so we can get the label etc from it).

Thanks, this is looking quite good.

*** Bug 558423 has been marked as a duplicate of this bug. ***

Created attachment 129109 [details] [review]
Keep playlists sorted when inserting or renaming them (v2)

Implements gabaug's above suggestions. I left NameDescending in as a sort choice for primary sources, because I didn't want to include potentially contentious choices in the patch.

What is the point of the None sort option?  I could see possibly having a 'Manual' sort option where the user could reorder them, but if there aren't any other options I'd rather the Sort menu be empty and the list of sort options null.

Expand "if (!(item != null && item.Active)) { return; }" etc out onto three lines, please.

Regarind the resorting happening in the OnExposeEvent override - why?  Are you sure the problem you were seeing before wasn't calling into Gtk+ code from a non-mainloop thread?  You should be able to modify the model anytime you want - from the main thread.  Use Banshee.Base.ThreadAssist.ProxyToMain if you need to.

Put a newline between things like this:

+        };
+        public override SourceSortType[] S

Did you mean to include the CellEditEntry.cs fix?  I don't really care, but thought I'd mention it in case you didn't.

You might/should test it, but I think instead of doing the BuildSortEventHandler thing, you can declare the delegate inline and it will capture the variable appropriately.  It won't capture a variable declared in a foreach () as I learned a few weeks ago (makes sense context-wise) though, just fyi.

Thanks John, this is really close to perfect.

> What is the point of the None sort option?  I could see possibly having a
> 'Manual' sort option where the user could reorder them, but if there aren't any
> other options I'd rather the Sort menu be empty and the list of sort options
> null.
The "None" option provides a default sort type for sources that don't support sorting. I don't intend it to be displayed in menus, it's just for implementing a reasonable `DefaultChildSort`.

> Regarind the resorting happening in the OnExposeEvent override - why?  Are you
> sure the problem you were seeing before wasn't calling into Gtk+ code from a
> non-mainloop thread?  You should be able to modify the model anytime you want -
> from the main thread.  Use Banshee.Base.ThreadAssist.ProxyToMain if you need
> to.
The `SourceView` class *does* use `ProxyToMain()` when setting the order. I don't know enough about the innards of GObject/GTK+/Gtk# to confidently diagnose the error, but if you want to try, it's quite reproducible: leave `Banshee.Sources.Gui/SourceView.cs` unmodified, and create a new playlist that will be sorted *below* an existing playlist.

> Did you mean to include the CellEditEntry.cs fix?  I don't really care, but
> thought I'd mention it in case you didn't.
Yes, see the notes in the post for the first version of the patch. Without the fix, renaming a playlist will rename, resort, and rename *again* -- potentially renaming some other playlist.

> You might/should test it, but I think instead of doing the
> BuildSortEventHandler thing, you can declare the delegate inline and it will
> capture the variable appropriately.  It won't capture a variable declared in a
> foreach () as I learned a few weeks ago (makes sense context-wise) though,
> just fyi.
This doesn't work; it will detect every menu item as selecting the last available sort option. Otherwise, it would be quite difficult to use delegates at all because updating the variables they reference would not propagate to the delegate.

I'd say less is more here. There is no point in having "No sorting", and there's no point in having Z-A sorting either.

> I'd say less is more here. There is no point in having "No sorting", and
> there's no point in having Z-A sorting either.
As I said above, the "no sorting" option is not exposed to users. It is merely a default sorting value, an internal implementation detail.

Z -> A sorting is supported in the main track list, so I assume *somebody* finds it useful.

(In reply to comment #17)
> As I said above, the "no sorting" option is not exposed to users. It is merely
> a default sorting value, an internal implementation detail.

Thanks for pointing that out, I missed that earlier on.

> Z -> A sorting is supported in the main track list, so I assume *somebody*
> finds it useful.

Yes, but there it's a column that can be sorted on either "this way" or "the other way around". Even I find it useful to e.g. easily find properties which have a "" or NULL value, or the other way around, i.e. find some rare metadata values that *have* been provided in only few of my files :)

Created attachment 129515 [details] [review]
Keep playlists sorted when inserting or renaming them (v3)

Apologies for the delay.

Fixes indentation style, removes Z-A sorting from the available options for primary sources.

Thanks John, I fixed a few things up and committed:

1) Moved SourceSortType to its own file
2) Checked in Radio toggled event that the radio item is Active to avoid sorting unnecessarily by the one that isn't active anymore
3) Changed Last.fm to use the same SortChildrenAction, and just override the action's label
4) removed the SortNone stuff, default sort is now just null, and added checks for SortTypes.Len > 0 where needed


Things still to do:
1) fall back to sorting by name if size (etc) equal
2) checkbox to separate by type (maybe on by default?)

Created attachment 129689 [details] [review]
Fallback to .Name if .Count of two sources is equal

Many thanks for working on this!

One question remains for me:

(In reply to comment #2)
> Moreover, the UI is a
> bit clumsy (well hidden in a context menu, while no functionality should be
> accessible from a context menu only).

Has this also been addressed?

Created attachment 129693 [details] [review]
Patch to add separate-by-type checkbox

This patch adds a separate-by-type checkbox to the sort menus, and falls back to Name if other comparisons are equal (eg size).

(In reply to comment #22)
> > bit clumsy (well hidden in a context menu, while no functionality should be
> > accessible from a context menu only).
> 
> Has this also been addressed?

No, not yet.  Should it go in the Media menu by the new playlist options?  I guess that makes the most sense with how we currently do things.  Thinking ahead/bigger picture, would it make sense to have a toplevel menu for Source and for Tracks or something?  I don't like how we sort of cram everything into Edit etc.

(In reply to comment #24)
> (In reply to comment #22)
> > > bit clumsy (well hidden in a context menu, while no functionality should be
> > > accessible from a context menu only).
> > Has this also been addressed?

> No, not yet.  Should it go in the Media menu by the new playlist options?  I
> guess that makes the most sense with how we currently do things.

Hard to decide. It's a bit of a mess right now indeed.

> Thinking
> ahead/bigger picture, would it make sense to have a toplevel menu for Source
> and for Tracks or something?  I don't like how we sort of cram everything into
> Edit etc.

Sounds like a good idea. Having a main menu item for each "thing" in the user interface, e.g. "Source" and "Song" makes sense imho.

Would you file a new bug, Wouter?  This one is really fixed.

Eh, this one is not yet fixed:

(In reply to comment #24)
> (In reply to comment #22)
> > > bit clumsy (well hidden in a context menu, while no functionality should be
> > > accessible from a context menu only).
> > Has this also been addressed?
> No, not yet.

Functionality that is only available from a context menu is not following Gnome standards (this is also in the HIG).

Wrt. the menu rethinking, that is indeed a separate bug.

(In reply to comment #27)
> Wrt. the menu rethinking, that is indeed a separate bug.

I filed bug 573526 to track the menu rethinking.

*** Bug 527285 has been marked as a duplicate of this bug. ***

Wouter, please file a separate bug for the context/menu bug.  The playlist
order itself is resolved.