GNOME Bugzilla – Bug 118862
store browser state on playlists across sessions
Last modified: 2012-08-03 23:19:57 UTC
I've started using rhythmbox, and I like it a lot, but I have a GUI suggestion: In the general view I can see Genre, Artist and Album. That's very useful, but in user-created views there is only the song list. Why can't I have an album filter in my own views? I think it could be nice. Thank you for your work.
*** Bug 126780 has been marked as a duplicate of this bug. ***
It would be a really nice feature for me, too. This way you can do different classifications, and you still can see the Artist/Album view. For example, you can create a new play list called "Classical", with all your classical music, and then it will be really useful this "Artist"+"Album" view.
Yes, I don't see why it has to be any different for playlists than for the main library. This also applies to the typable filter, why can't we have this for the playlists? This is my single largest complaint with rhythmbox so far.
*** Bug 322115 has been marked as a duplicate of this bug. ***
retitling to make enhancement clearer
I guess I was imagining that this feature was present in rhythmbox because it appears that this bug was opened back in 2003! I suppose since rhythmbox seems to have come so far in the last few months in CVS with podcast, DAAP support etc., that I have began to use it much more, and therefore assumed that this would have been standard functionality long ago. Really, this is more important than the current "low" priority assigned to it, because it is a major expectation of any user new to rhythmbox that they should be able search and/or browse a playlist. Sorry I don't have code to do, but it should probably be bumped up in priority nonetheless.
I suggest "High" from the GNOME Bugzilla guidelines: "High Seriously broken, but not as high impact. Should be fixed before next major release. Frequently includes cosmetic bugs of particularly high visibility, regressions from functionality provided in previous releases, and more minor bugs that are frequently reported." "Normal Either a fairly straightforward workaround exists or the functionality is not very important and/or not frequently used." "Low Just not all that important. Rarely used in GNOME." This certainly fit "the cosmetic, but frequently reported" (high) and it is certainly not something that would be "rarely used" (low). Just my $0.02.
For automatic playlists this shouldn't be too hard, but normal playlists will be more difficult. Once rb-playlist-source gets split into RBAutomaticPlaylistSource and RBStaticPlaylistSource, I think we should be able to get this done for the former. I think the way to do it would be to factor the browser/searching code out of RBLibrarySource into a helper class, so that RBAutomaticPlaylistSource can use it too. It will need to have the browser widgets, and _get_query method, _search and _reset_filters methods. The other option would be to make RBAutomaticPlaylistSource dervie from RBLibrarySource and make the _construct_query method a class method. The problem would be that it would no longer dervie from RBPlaylistSource.
Even if the browsing can't be done for RBStaticPlaylistSource, we should be able to do searching on it similar to the main window's abilities. Browsing may be able to take a back-burner but searching would be nice?
From an implementation point of view, browsing and searching are equivalent. For automatic playlists, adding these is fairly simple - the browsers and search box get turned into additional criteria that get added to the query. The issue with static playlists is that there currently isn't a seperation between the tracks in the playlist, and what is visible - which also causes bug 319278. What needs to happen is we combine the patch on that bug, with using a GtkTreeModelFilter so that HIDDEN tracks and those not matched by the browsers/search box are not visible, but don't affect the actual playlist definition. This will be easier to do once the playlist source separation happens.
I've just committed search-box support for automatic playlists. I'll take a look at refactoring the browser out and adding that to auto playlists soon, and normal playlist support after that.
Created attachment 56690 [details] [review] auto playlist browsers This adds browser support to automatic playlists. There is a reasonable amount of code copied from rb-library-source.c that should be factored out into it's own class/some helper functions, but I haven't done that yet. The only issue I've found so far is that the browser is hidden automatically when you changed sources.
Created attachment 57012 [details] [review] updated patch Browser code factored out into it's own class. It still needs a bit of tidying up - especially RBLibraryBrowserQueryType, which shouldn't have anything to do with searching.
Created attachment 57536 [details] [review] updated patch I've rewritten RBLibraryBrowser to a) have a sane API, b) make it easy to add browsers for different properties. All that sources need to do now is set the base query model, and listen for the "changed" signal. There is a known issue of rebuild_child_model not working after the initial run, which means that "child browsers" don't get updated when something is selected in the parent. I should fix this soon.
Is the file rb-library-browser.h missing from the latest patch? Because on applying patch and recompiling, I get this: [...] ed-label -Wunused-value -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wall -Werror -std=gnu89 -MT rb-library-source.lo -MD -MP -MF .deps/rb-library-source.Tpo -c rb-library-source.c -fPIC -DPIC -o .libs/rb-library-source.o rb-library-source.c:40:32: error: rb-library-browser.h: No such file or directory
Created attachment 57767 [details] [review] patch with missing files Oops, files are included in this one.
Well I can now see the browser for automatic playlists, but they are all empty (e.g. I see "0 genres" "0 artists" "0 albums" for every automatic playlist). Also, the first time I started rhythmbox after applying the patch, it overrode my library preference and reset it to browse "Artist" and "Album" only (I normally set it to Genre, Artist, Album).
Created attachment 58252 [details] [review] updated patch Fixes the browsers to auto playlists, and makes subsequent browsers update when selection changes (by fixing rb-property-model so that it handles the query-model being changed). For me this works well for the library and auto playlists, I haven't changed static playlists to use this yet.
This works nicely for me. The only issue now is that it doesn't remember the state (whether browser is on or off) when you switch from playlist to playlist. Even so, I'd say commit this now and work on that later.
Uh oh, here's a blocker. When I click a column in the library or in an autoplaylist to resort, I get a crash: RhythmDB-ERROR **: file rhythmdb-query-model.c: line 1868 (rhythmdb_query_model_base_index_to_child_index): assertion failed: (index == rhythmdb_query_model_child_index_to_base_index (model, pos)) aborting...
Confirming that backing out the patch removes this crash.
Here is the gdb backtrace for what it's worth:
+ Trace 65692
Created attachment 58307 [details] [review] fixed patch The problem was that my "base model resort" handling for the filter models was dodgy. It hadn't gotten tested properly before, because nothing using filter models was having the base model resorted. Basically the base->child and child->base index conversions don't work correctly during the middle of a resort.
Looks good, sorting works now. I'd say commit now, then worry about remembering if browser state is on/off later. This would be good to get in for 0.9.3.
(In reply to comment #25) > remembering if browser state is on/off later. Regarding this issue, couldn't the browser state just be written as an attribute to the open XML tag in playlists.xml? e.g. <playlist name="Recently Added" type="automatic" limit-count="0" limit-size="0" limit-time="0" sort-key="Quality" sort-direction="0" browser="1"> or would this constitute abuse of the playlists.xml? Storing it in a gconf key doesn't seem like it would work well because it would have to be synchronised with the playlists.xml file.
(In reply to comment #26) > <playlist name="Recently Added" type="automatic" limit-count="0" limit-size="0" > limit-time="0" sort-key="Quality" sort-direction="0" browser="1"> If the attribute "browser" was missing, then it would default to "0" and write out browser="0" to the tag the next time the XML file was written/updated. That would preserve backwards-compatibility.
(In reply to comment #25) > Looks good, sorting works now. I'd say commit now, then worry about > remembering if browser state is on/off later. This would be good to get in for > 0.9.3. > No, it wouldn't. This is not the sort of patch you commit two days before a release. It's not the end of the world if this gets pushed out to 0.9.4, but it'd suck if we had to release 0.9.3.1 in a couple of days to fix a crasher we hadn't spotted before releasing 0.9.3.
(In reply to comment #28) > No, it wouldn't. This is not the sort of patch you commit two days before a > release. It's not the end of the world if this gets pushed out to 0.9.4, but > it'd suck if we had to release 0.9.3.1 in a couple of days to fix a crasher we > hadn't spotted before releasing 0.9.3. Grumble. I guess you're right, it's just that it's such a nice feature that has been requested for 3 years. Lack of this feature is kind of thing that makes rhythmbox not look as polished as it should be (or has become). But you're right, it's probably better to have a stable feature than a broken one, so in this case, discretion is the better part of valour.
(In reply to comment #29) > Grumble. I guess you're right, it's just that it's such a nice feature that > has been requested for 3 years. Lack of this feature is kind of thing that > makes rhythmbox not look as polished as it should be (or has become). But > you're right, it's probably better to have a stable feature than a broken one, > so in this case, discretion is the better part of valour. My reason for lobbying for this is that I'm admittedly a bit Fedora-centric. If it was in 0.9.3 then there's a good chance that the Fedora people would include it in Rawhide and then in FC-5. If it waits until 0.9.4 (depending on when that is) then it would almost certainly not be in the first release of FC-5 and my experience is that you typically have to wait a good while before the Core maintainers will upgrade apps after the first release comes out. I like to be able to show friends who use Fedora that they have a real alternative to iTunes on Linux. So there's my bias on the table. ;-)
While having more features in FC5 or Dapper would be nice, putting unstable things in isn't a good plan. This patch includes a rewrite of the library browser code. It also uses bits of the filtering query model code that nothing else uses, and so hasn't been tested properly (see the resort problem above). This will want some proper testing before it gets into a release, best done as a few weeks in cvs (so it will be tested for 0.9.4). (In reply to comment #26) > Regarding this issue, couldn't the browser state just be written as an > attribute to the open XML tag in playlists.xml? > or would this constitute abuse of the playlists.xml? Storing it in a gconf key > doesn't seem like it would work well because it would have to be synchronised > with the playlists.xml file. If we want per-playlist browser visibility (and paned position), storing it in playlist.xml is about the only option currently available. Storing it anywhere else would require that playlists have some unique identifier, which they don't.
Holding this for 0.9.4 is prudent, and to demonstrate: I discovered an additional bug in the most recent patch. If you enable this patch then login to a DAAP server, the browser doesn't update. It shows the same Genre/Artist/Albums as the library. If you select an artist that happens to be in both your local library and the DAAP server, you see all the tracks by that artist on the DAAP server, and they are playable. Playlists also have the correct content. It appears simply that the browser doesn't update. (In reply to comment #31) > If we want per-playlist browser visibility (and paned position), storing it in > playlist.xml is about the only option currently available. Storing it anywhere > else would require that playlists have some unique identifier, which they > don't. Putting them in the playlists seems fine with me, they can be ignored by any other application that didn't use them.
I've also found an issues with RhythmDBPropertyModel - it doesn't store which tracks it has already counted, and so is confused by receiving multiple HIDDEN-changed signals from filtering models, which propagate ones from the base model as well as adding their own. This causes the track counts in the Artist browser to be doubled, and thsoe in the Album browser to be tripled. As a workaround, we could make filtering models not add their own. But the reason solution would be to make RhythmDBPropertyModel keep track of which entries it has included, so they don't get counted twice.
Created attachment 58697 [details] [review] updated patch This patch fixed the bugs from comment 32 and comment 33. Also daap sources have the same browser visibility and paned position as the library, because they derive from it. If this is okay, I think we could commit it and worry about browser vis/paned position later.
(In reply to comment #34) > Created an attachment (id=58697) [edit] > updated patch > > This patch fixed the bugs from comment 32 and comment 33. > > Also daap sources have the same browser visibility and paned position as the > library, because they derive from it. If this is okay, I think we could commit > it and worry about browser vis/paned position later. Works for me. DAAP server browser seems sane as do the browsers for the autoplaylists.
Recent CVS updates seem to have broken this patch. Even though it still applies against CVS HEAD, now it crashes whenever I click on either a Genre or an Artist in any of the browser windows. Here's the bt: crashes here: 0x080c600f in rhythmdb_property_model_entry_removed_cb (model=0x9937e28, entry=0x9f54878, propmodel=0xbfa6795c) at rhythmdb-property-model.c:478 478 if (g_hash_table_lookup (propmodel->priv->entries, entry) == NULL) (gdb) bt
+ Trace 65943
Created attachment 58852 [details] [review] updated patch update for cvs changes, and fix some issues. Hopefully this doesn't have any more problems, but it probably does.
(In reply to comment #37) > update for cvs changes, and fix some issues. > Hopefully this doesn't have any more problems, but it probably does. Tested, applies cleanly, works for me. Currently the patch does the right thing, it just has some user interface quirks, that you probably going to address after a commit to CVS in any case. One issue is that if I select an item (say album) in the browser it restricts the view (good), but move to another playlist and return, it remembers that view (good), but because the browser is hidden by default, you can't see that only a subset is selected (bad). This should be fixed when the visibility/paned state is fixed.
Patch committed to cvs, with the browsers preference fixed, so that it works correctly - this means we only have static playlists to go. Remembering browser visibility (between sessions, and across source selections) and pane position (between sessions) is something that needs to be fixed.
Created attachment 60296 [details] [review] add browsers to static playlists This is the penultimate piece, browsers for static playlists. The only thing left to do after this is store browser visibility and position.
(In reply to comment #40) > Created an attachment (id=60296) [edit] > add browsers to static playlists > > This is the penultimate piece, browsers for static playlists. Works for me.
This would add a browser to the play queue source, wouldn't it? I don't know why exactly, but that doesn't sound right to me. It could easily be hidden by setting impl_can_browse to rb_false_function in rb-play-queue-source.c. Otherwise, this looks sensible.
Committed, with the play-queue change. Once again I miss something because I don't use queue-as-source.
Created attachment 63863 [details] [review] Remember browser disclosure state for non-gconf managed sources (playlists etc.)
Created attachment 63867 [details] [review] Cleaned-up browser state patch Moved code duplication into a function.
Created attachment 63869 [details] [review] Fix mistake in previous patch
The patch looks good, so I've committed it to cvs. Do we want to remember browser state across sessions? Visibility and pane position for playlists could be stored in the playlists.xml file if you wanted to - however to me that file should really only have stuff that define what the playlist is.
(In reply to comment #47) > Do we want to remember browser state across sessions? Visibility and pane > position for playlists could be stored in the playlists.xml file if you wanted > to - however to me that file should really only have stuff that define what the > playlist is. Yes, I think that we should remember browser state between sessions, since Library, Podcast etc. source states are remembered it would be inconsistent for playlist browser state not to be remembered. I agree that playlists.xml is not an ideal place to store the state, but where else could we store it? DAAP tracks added to the queue are stored in the playlist.xml but they won't resolve upon a restart. Remembering browser state is no worse than that, IHMO.
*** Bug 339970 has been marked as a duplicate of this bug. ***
When playing a song of a playlist(static and dynamic ones), if I set a filter that makes the song disspear from the list, rhythmbox doesn't stop playing it, but it is a song from the library, if the song dissapears from the list rhythmbox stops. The different behaviour is intended, or there should be a default behaviour? I also think it should save browser state in betwenn sessions.
*** Bug 360756 has been marked as a duplicate of this bug. ***
Renaming to reflect remaining piece of work.
what about this bug ? I think that it's important to remember browser state between sessions.
Let's put this bug report to rest, it's way too old and lacks focus now. Go subscribe to bug #652892 to follow-up on the remaining "Browser visibility is forgotten on program restart" issue.