Created attachment 58111 [details]
mockup screenshot

There should also be some way to drag the query over to the sourcelist to create an automatic playlist.

Having a way of limit the search box would be good. In the mockup you removed the browser, which I'm not sure about though.

The "row" with the search box currently has three sections, we could use one for the enable browser disclosure, one for the search box, and one for the searh limit stuff. Alternately we could do what banshee does and have a drop-down menu - although that is less discoverable.

Created attachment 59876 [details] [review]
patch

I really like this.  Hope you do too.

I've removed the song position slider menu item and added a browse button to the toolbar which I think makes much more sense.

I've only implemented this for the library and podcast sources so far.

Hmm, there seems to be a problem where library source->priv->search_text is getting set to NULL after running the query.  So, when clicking on the other buttons only runs the is-all query.  I'm not sure why because I had this working a bit ago.

Created attachment 59894 [details] [review]
updated patch

OK, I figured it out.  The problem was that I was using a source as user-data to the action-group in the constructor.  Only one action-group is created per name which means per-class.  So the source user-data wouldn't be correct if there was more than one instance of the class.

It worked when there was only one Library but didn't work when there were other DAAP libraries.

I've fixed this so it should work now.

Great!  Works for me (I did notice that previous patch worked some of the time but I couldn't figure out when, but this one seems stable).  My only suggestion might be, since we would be changing the interface anyway, is to revisit bug #128109 and consider adding a clear button since it does seem to confuse many people (/me ducks).

This, together with a fix for bug #322787 to limit entries in the browser, will make rhythmbox a great music management tool.

Exactly, this change makes bug 322787 more obvious.

I don't want to bug this bug down with discussion of the clear button though.  We can do that after :)

One other thing that I noticed (even with current UI) is that we need to clear the browser when it is hidden.  If we don't then it is insanely confusing.

Not 100% sure about moving the browse button to the toolbar, though.  Perhaps the search box could be moved over to the right as James suggested in comment #3.

Also what about the "Tags" option from the mockup?  It would be nice to search on arbitrary tags so that you could find things embedded in the Comment field, for example (although I don't know if rb yet reads that piece of metadata).

(In reply to comment #8)
> Exactly, this change makes bug 322787 more obvious.
> 
> I don't want to bug this bug down with discussion of the clear button though. 
> We can do that after :)

Fair enough. ;-)
 
> One other thing that I noticed (even with current UI) is that we need to clear
> the browser when it is hidden.  If we don't then it is insanely confusing.

Yes, I was thinking that would be the case, because you don't know what's been limited on.  Incidentally what does iTunes do in that case?  (I don't have a box easily accessible right now).

I like having the browse button in the toolbar - just like in itunes.  Putting it in the search bar adds too much visual noise.  I think the search bar should only have search related stuff.  Perhaps we can find a better icon for the button though.

Looks fairly good to me (aside from the browser icon). Instead of giving the search box 1/3 of the width and having space at the right, it might be better to have the buttons over the right and give the search box the extra space.


Currently you've got sources handing the list of actions to the source-header. I'm not sure if that's the best way, or if it would be better to pass a list of RhythmDBPropertyTypes and have a "get query" method. The current way has the advantage that sources can do whatever they want with them, and they can easily have different button names for the same property (e.g. podcasts use the "album" field for the feed).

On the other hand, using the "get query" method means that the property->query code is in the source-header and not in every source. It would also make it easier for plugins to add search-box options, for example by giving the source-header a button and a string->query function. This would allow a plugin for adding the "complex" search box patch that has been floating around, which lets you enter things like 'artist = "foo" and playcount > 10)'.

Which way is best, I'm not sure.

The way the spacing works right now is that the search box is 1/3 and the buttons are 2/3.  Left aligning everything is useful since the width of the button toolbar will change and this avoids "bounce".  It would be possible to keep the search box left aligned and right align just the buttons but that puts too much space between the box and buttons.  The most common usage tighly couples the box and buttons - hit button then type or vice versa.  Giving the box 2/3 and the buttons 1/3 looks a little strange, doesn't allow us to add too many more buttons, and assumes maybe too much about how those terms are translated.  I think it should be possible to add "type" selections to the search bar as we see in iTunes: "Music" "Audio Books" even "Videos".  That will all require more space for the buttons.


I thought about using exporting properties for the buttons instead of actions but I thought it was too limiting and potentially breaks abstraction a bit.  I'm not sure that there will always be a simple one to one mapping from button action to property.  Don't know.

Actually I was thinking that it might be better for plugins (like a new source type) that pretty much all of the functionality is contained within the source.  However, I haven't looked over how you've designed the plugin interface.

I was thinking that even some fields have source specific meanings (ie PROP_TITLE doesn't mean the same thing for every source type).  Not to mention that the SearchAll query will be different for each source and doesn't map to a property at all for podcasts.

Hey, we can always change it, right?

So, any suggestions on a Browse icon?

Oh, and also being about to add content type selections to the search bar ("Music", "Audio books", "Podcasts", etc) like in iTunes I think suggests handling the actions in the source also since I'm not sure there is a simple mapping to property from any of these.

(In reply to comment #11)
> I like having the browse button in the toolbar - just like in itunes.  Putting
> it in the search bar adds too much visual noise.  I think the search bar should
> only have search related stuff.  Perhaps we can find a better icon for the
> button though.

I'm not too fussed about it.  I'm getting used to it now that I'm using the patch.  As for a better icon, where was the current icon taken from? 

I like this.

For plugins to be able to extend the set of search types, we'd need two things:
- register a function for each search type that returns a rhythmdb subquery to include in the source's active query
- somehow extend the list returned by rb_source_get_search_actions on a per-source basis

We'll have to do the second bit for rb_source_get_ui_actions too.

(In reply to comment #13)
> The way the spacing works right now is that the search box is 1/3 and the
> buttons are 2/3.  Left aligning everything is useful since the width of the
> button toolbar will change and this avoids "bounce".  It would be possible to
> keep the search box left aligned and right align just the buttons but that puts
> too much space between the box and buttons.  

What I was actually thinking, was right-aligning the buttons and giving the search box the rest of the space, not a fixed amount.



(In reply to comment #16)
> We'll have to do the second bit for rb_source_get_ui_actions too.

After thinking about it, I've implemented that in a fairly silly way. What that method should really do is return the name of a GtkUIManager item, which gets put into the toolbar. Then plugins could easily add stuff via GtkUIManager merging.

Created attachment 60271 [details]
SVG mockup

What about a Evince/Firefox type of search bar?

E.g. something like the attached mock up made in Inkscape.

That looks okay, although I'd probably re-order it to be "Search for [Artist/Album/Title/All] that [contains/starts with] [user-entry]"

I'm sorry but I don't like that mockup at all.  I think it misses the whole point of the search bar.

One of the reasons that Evince and Firefox have the search box at the bottom is the same reason why it isn't displayed all the time: the main interface isn't designed for finding things it is designed for reading (ie. search is secondary).  In Rhythmbox, search is primary.  The primary purpose of the track listing (and even the Browser) is to help you find tracks in your collection.  Separating the browser and search box doesn't make much sense to me either.

The Browser happens to be a somewhat cumbersome way to do it if you have more than say a few dozen artists.  When that happens we need to rely on searching.  Relying on the gtk treeview typeahead find just doesn't cut it for a number of reasons: too fiddly with respect to input focus, have to search each individual treeview separately, etc.

For similar reasons I don't think it makes much sense to be able to hide the search bar.  That would be like hiding the search box on google.com. 

I really think that the right way to go is:
  Search -> [Browse ->] Track selection

I think more relevant UI comparisons can be found in:

 * iTunes (obviously) - The search is at the top, search bar slightly below it, and the browser underneath that.

   This is somewhat flawed I think in that the search box is separate from the search bar.  It makes is slightly harder to use.  It also makes the search box global and not per source (good and bad I suppose)

 * Google Music Search (http://www.google.com/musicsearch?q=white+stripes&btnG=Search+Music)

   The search box is at the top and results are displayed underneath.  Artists and then Albums are displayed first in the list of results - this is equivalent to the filtering of the Browser.  And similar to using the search bar limit buttons.


Anyway, James and Jonathan I'm not really up to speed on the plugin stuff yet so if you can let me know exactly how you'd like this patch reworked I'd be happy to do so.  Thanks guys.

Okay, I see your point. If that's how iTunes/Rhythmbox is ought to be used, I would say the search entry should be its current level, but left aligned, so the user can use the UI in a flow like so:

|
v --> --> |
          v

Having it right aligned would make the flow:

----->
<-----
|
v --> --> |
          v

Does it make sense? =)


I have been thinking about the buttons that the patch introduces to the search bar...

If the search was carried out on the focused pane (Artist,Album,Song) the buttons wouldn't be needed.

So what if the default text inside the search entry had the focused pane name displayed?

Example: I have clicked in the album pane, so it have focus. The search bar now looks like this:

Search: [in albums     ]

Or perhaps the focused pane name is written outside the searchbox:

Search: [              ] in albums

(In reply to comment #20)
> Anyway, James and Jonathan I'm not really up to speed on the plugin stuff yet
> so if you can let me know exactly how you'd like this patch reworked I'd be
> happy to do so.  Thanks guys.

Basically, we don't know yet exactly how it would work. Plugins get handed the RBShell object, and from there can access other objects and do stuff.

Currently the processing of the search bar is handled inside the particular source. For plugins to be able to extend it, there would need to be some way of saying "add an option X, and this is how to process the result", I'm not entirely sure how it should work.

Don't worry too much about it, we can always add it later; there are many other things that currently aren't extensible via outside code either.

Created attachment 60602 [details] [review]
updated

This only syncs with HEAD and changes the Browser icon to be a little less silly - it now uses the stock_music-library icon until we can find something better.

Want to get this in?

How soon do we want to do 0.9.4? We could try to get it out soon, then land this, plugins and whatever else afterwards. Alternately we could land those things now, and do the release a few weeks later.

Good question.  I don't have a strong opinion but I lean toward getting in sooner so that we can move on to other things.  This is the of the UI changes that I had in mind.

I'd be happy to see this go in now.

+1 for going in now.

Looks fine to me. It might be clearer to have "Songs" called "Title", since that is the name of the column it will search in.

Committed with the songs->titles change.  Thanks.