GNOME Bugzilla – Bug 322787
A search should limit the entries in the browser.
Last modified: 2006-06-07 20:09:08 UTC
Please describe the problem: I have always thought that performing a search should limit the entries in the browser. To see how I think it should work, have a look at quod libet [paned browser view]. Here is an example [contrived]. Search entered: "just". Songs found: Bob Dylan - Just Like Woman [Blonde On Blonde] Radiohead - Just [The Bends] Multiple songs by Justin Timberlake [I don't actually have any]. I believe that the artist browser pane should now be limited to: Dylan Radiohead Timberlake And the album pane should be limited to: Blonde On Blonde Radiohead Any albums by Timberlake. Steps to reproduce: 1. 2. 3. Actual results: Expected results: Does this happen every time? Other information:
*** This bug has been marked as a duplicate of 98725 ***
I was aware of this bug and I am pretty sure that the bug reports are talking about different things. From what I can understand of the other bug report (it isn't written very clearly), If I click on "The Cure" in the browser Pane", then there shouldn't be an Artist column in the song listing section because it is unnecessary. i.e. you know who the artist is. Either I'm reading the other bug incorrectly, or this isn't a duplicate.
Your right. I hadn't read that bug closely enough, and the several dupes on that bug aren't actually duplicates - because they suggest the same thing that you are.
*** Bug 140808 has been marked as a duplicate of this bug. ***
*** Bug 137255 has been marked as a duplicate of this bug. ***
iTunes has the same behaviour as the desired behaviour described here.
This would be relatively easy to implement with RB from cvs. Simply call rb_library_browser_set_model after each search box change, with a query model that takes the search box into account.
Created attachment 59637 [details] [review] Patch to limit browsers with search Attached is patch that partially implements this. Currently this patch will sometimes reset any selected elements in the browser. For example, if you have "Classical" genre selected and type "light" quickly, you will only see tracks in that genre that may have light in their artist, title or album. However, if you type it slowly, the browser genre list "resets" to all and searches on the entire library. Currently if any of the library items are selected "Genre", "Artist" or "Album" then the search only takes place within those selected items. Ideally with the extra browser limiting should only limit the items within those already-selected elements, but I'm not sure how to do this.
(In reply to comment #8) > Created an attachment (id=59637) [edit] > Patch to limit browsers with search > Currently if any of the library items are selected "Genre", "Artist" or "Album" > then the search only takes place within those selected items. Ideally with the > extra browser limiting should only limit the items within those already-selected elements, but I'm not sure how to do this. This is the way iTunes does it. iTunes will search within the already-selected items within the library browser. If the search box is blanked it will return to the previously-selected browser items.
(In reply to comment #7) > This would be relatively easy to implement with RB from cvs. Simply call > rb_library_browser_set_model after each search box change, with a query model > that takes the search box into account. The patch does this, but how do I keep the original selections active? It seems tht rb_library_browser_set_model() resets them.
(In reply to comment #10) > The patch does this, but how do I keep the original selections active? It > seems tht rb_library_browser_set_model() resets them. That's due to me not writing that bit of code yet. I originally hadn't done it because nothing needed it at the time, and I forgot about it. I'll whip up a patch.
(In reply to comment #11) > That's due to me not writing that bit of code yet. I originally hadn't done it > because nothing needed it at the time, and I forgot about it. I'll whip up a > patch. That might also fix bug #322787. I tried called rb_library_browser_get_selection before setting the query model and then restoring it with rb_library_browser_set_selection, but that didn't work, and probably isn't the right way to do it anyway.
(In reply to comment #11) > That's due to me not writing that bit of code yet. I originally hadn't done it > because nothing needed it at the time, and I forgot about it. I'll whip up a > patch. It's now done in cvs. If you have All Artists selected, select an album, and them select the atist for that album, it should keep the album selected.
(In reply to comment #13) > It's now done in cvs. > > If you have All Artists selected, select an album, and them select the atist > for that album, it should keep the album selected. Thanks, tested, seems to work. What about saving the criteria when redoing the query model? Do I need to call another function to save and then restore them?
Created attachment 60486 [details] [review] Updated patch should do the limiting Only works for library
Created attachment 60488 [details] [review] Should be a clean patch
Works for me. It can probably be committed if it was ported to the other sources.
Created attachment 60499 [details] [review] Updated with autoplay list support Added auto playlist support. Looks like static playlists are a bit tricker.
Created attachment 60728 [details] [review] Support for library, auto and static playlists This now does library, auto and static playlists. Podcast support coming, but since podcasts don't use the library browser widget it is a little more tricky. I'm using the rb_property_view_set_model to limit the feeds, but would appreciate pointers on how to construct the appropriate query (since feeds and posts are separate entries in rhythmdbx.xml).
(In reply to comment #17) > Works for me. It can probably be committed if it was ported to the other > sources. Could we commit for the library and playlist sources while podcast and iradio are in development? I think it would still be useful to have in CVS to test for a while, while porting the remaining sources continues.
Created attachment 60729 [details] [review] Partially working podcast feed limiting patch This partially works, but the query isn't quite right, it finds feeds that have the appropriate search terms, but don't always correspond to the actual episodes. We need to find all the feeds for a given set of episodes and using the query for the episodes doesn't work when applying back to the feed list.
Created attachment 60730 [details] [review] Limits iradio genres This limits the iradio genre lists by search. It works, but has the problem that the RBPropertyView does not have a way of holding on to the current selection when the model is updated.
Created attachment 60898 [details] [review] different approach for iradio Uses chained query models (search -> genre view) to limit the displayed genres. Probably fixes a query model leak too..
Created attachment 60901 [details] [review] better patch for iradio This removes the cached_all_query thing, which would only really make a difference if you had an unimaginably huge number of stations.
We can't really do this properly for podcasts. The podcast feed view is based on a TYPE equal PODCAST_FEED query, so it can't exclude feeds for which no episodes match the search string. Basing the feed view on a query for episodes grouped on feed URL has the problem that feeds with no episodes become invisible (see bug #327372).
(In reply to comment #24) > Created an attachment (id=60901) [edit] > better patch for iradio > > This removes the cached_all_query thing, which would only really make a > difference if you had an unimaginably huge number of stations. Works nicely for me.
(In reply to comment #25) > We can't really do this properly for podcasts. The podcast feed view is based > on a TYPE equal PODCAST_FEED query, so it can't exclude feeds for which no > episodes match the search string. Yes, I discovered that in trying to do my patch. > Basing the feed view on a query for episodes grouped on feed URL has the > problem that feeds with no episodes become invisible (see bug #327372). Is there no workaround, or should we just drop the idea of doing browser limiting for podcasts? Either way, lack of podcast support shouldn't block the other patches.
I think we should just drop the idea of doing browser limiting for podcasts. The items displayed in the podcast feed list aren't just groupings of entries from another view, they're entities themselves, so it doesn't really work the same way. I think the podcast view would work better as a tree view, with posts grouped by feed. We're (slowly) working towards being able to implement this.
(In reply to comment #28) > I think we should just drop the idea of doing browser limiting for podcasts. > The items displayed in the podcast feed list aren't just groupings of entries > from another view, they're entities themselves, so it doesn't really work the > same way. Fair enough I can see that for podcasts it may not make too much sense, so we should be good to go now with support for library, auto/static playlists and iradio. > I think the podcast view would work better as a tree view, with posts grouped > by feed. We're (slowly) working towards being able to implement this. Sounds good to me, like iTunes, right? Is there a bugzilla on this?
Using the two patches together looks fine to me. I'm not sure if the podcast grouping has a bug, but I think there are bits scattered across several bugs (for things needed to implement it).
I'm some fraction of the way through rewriting the library and playlist bits to use a single query chain (search->browser->entry view) which is simpler and should be more efficient too.
Created attachment 61117 [details] [review] reworked library and playlist patch - playlist- and library-derived sources use a single query chain - existing query models are reused when appending text to the search box; this makes incremental searches much faster. - browser only creates new chained query models when it has a non-empty selection - browser can preserve selections even when the query is asynchronous - no more query model leaks in the browser
Looks good from my quick testing and scan over the code.
With the commit of the search bar patch, this patch fails to apply cleanly against current CVS HEAD: $ patch -p1 < patches/active/search-limits-browser.diff patching file rhythmdb/rhythmdb-query-model.c patching file shell/rb-play-order.c patching file sources/rb-auto-playlist-source.c patching file sources/rb-iradio-source.c patching file sources/rb-library-source.c Hunk #1 succeeded at 74 (offset 1 line). Hunk #2 FAILED at 110. Hunk #3 succeeded at 121 with fuzz 2 (offset 1 line). Hunk #4 succeeded at 327 (offset 14 lines). Hunk #5 succeeded at 345 (offset 1 line). Hunk #6 succeeded at 454 (offset 66 lines). Hunk #7 succeeded at 480 (offset 1 line). Hunk #8 succeeded at 693 with fuzz 2 (offset 66 lines). Hunk #9 FAILED at 703. Hunk #10 FAILED at 753. Hunk #11 FAILED at 1162. 4 out of 11 hunks FAILED -- saving rejects to file sources/rb-library-source.c.rej patching file sources/rb-podcast-source.c Hunk #2 succeeded at 215 (offset -2 lines). Hunk #3 FAILED at 261. Hunk #4 succeeded at 445 (offset 10 lines). Hunk #5 succeeded at 735 (offset 44 lines). Hunk #6 succeeded at 789 (offset 10 lines). Hunk #7 succeeded at 1050 (offset 44 lines). Hunk #8 succeeded at 1029 (offset 10 lines). Hunk #9 succeeded at 1090 with fuzz 2 (offset 44 lines). Hunk #10 FAILED at 1122. Hunk #11 succeeded at 1107 (offset 21 lines). Hunk #12 succeeded at 1158 (offset 44 lines). Hunk #13 succeeded at 1143 (offset 21 lines). 2 out of 13 hunks FAILED -- saving rejects to file sources/rb-podcast-source.c.rej patching file sources/rb-static-playlist-source.c patching file widgets/rb-library-browser.c patching file widgets/rb-library-browser.h
Created attachment 61165 [details] [review] updated patch includes changes for iradio, library, and playlists.
Some issues: 1) The browser for podcasts feeds now seems to include the episodes instead. 2) There seems to be some UI "bounce" when updating the browsers which maybe because of async parsing: the list in each browser appears to empty, then re-populate. Not a big issue, but noticeable. 3) I got an odd segfault when searching in an auto-playlist, doesn't always happen, will back-out patch to see if it still happens: Program received signal SIGSEGV, Segmentation fault.
+ Trace 66900
Thread NaN (LWP 1224)
(In reply to comment #36) > 3) I got an odd segfault when searching in an auto-playlist, doesn't always > happen, will back-out patch to see if it still happens: Segfault goes away when patch is backed out.
Created attachment 61176 [details] [review] less broken patch drops the podcast changes and fixes the auto playlist search crash.
Definitely better, podcast issue gone and no autoplaylist search crash. (The browser list does repopulate when the search is updated, which isn't a big deal, but I noticed that it makes a difference if you select an item (say an artist), then do a search, then backspace out the search, the selected item can sometimes disappear from the center of the view, so it isn't clear that something is selected.) However, I would say commit this now and we can worry about the UI display/tweaking later.
This is really nice. Good work Jonathan!
Committed.
*** Bug 317540 has been marked as a duplicate of this bug. ***