GNOME Bugzilla – Bug 359740
expanders in source list
Last modified: 2007-02-28 16:00:05 UTC
Might be nice to use expanders for each group in the source list. http://www.apple.com/itunes/jukebox/sourcelist.html It looks pretty slick. Might be able to use: http://blogs.gnome.org/view/mr/2006/09/23/0
Created attachment 74074 [details] [review] patch * Adds RBSourceGroup boxed type instead of using enum directly. This has a few advantages like adding display names, allowing more than one group of type FIXED, allowing plugins to add groups (say "Store" group for example). * Renames current library source to Music since the group is called Library and we may want to add a Movies source later anyway. * Disables reordering of playlists since we use sorting. I think keeping them sorted makes more sense anyway. * Fixes the playlist saving to use a foreach instead of only walking the toplevel. * Simplifies the code quite a bit.
Created attachment 74075 [details] screenshot
Created attachment 74137 [details] [review] patch 2 Patch made to not explode with the plugins I just committed to cvs. Using the expander-on-right mentioned in the blog might be okay, as-is things like DAAP and iPod playlists get indented a fair way. Although it would probably look odd with some expanders on the right (group) and others on the left (sources with children) We also should expose RB_SOURCE_GROUP_LIBRARY et al to Python, so sources implemented there (e.g. the MagnaTune one, which doesn't work with this applied). (In reply to comment #1) > * Renames current library source to Music since the group is called Library and > we may want to add a Movies source later anyway. Should we move "Missing Files" and "Import Errors" to being top-level Library items, rather than being under Library->Music? We also have some code that creates "child library sources" under the library (Music) if the user has multiple library locations set (requires gconf to set). Perhaps we should have the "Music" if the user only has one set, and if they have multiple have "All Music" and the location-specific ones? > * Disables reordering of playlists since we use sorting. I think keeping them > sorted makes more sense anyway. I find having my playlists manually sorted is handy - I have my rating-based playlists together, then my "criteria based" ones, then my "mood" ones and finally the others. AFAICT playlists aren't automatically sorted in the patch, you just can't change the ordering.
Created attachment 74162 [details] [review] updated patch * Add a Store group * Expose groups to plugins * Fix up a few random issues with plugins * Add copyright/license headers to magnatune files (we should require these for all new files) * Clean spurious whitespace I tried using the gossip expander in an earlier version. It didn't think it worked very well. One strange thing I noticed is that the __init__ method of the MagnatuneSource doesn't seem to get run.
The patch applied cleanly, but it segfaults on startup. (I am running uninstalled in a separate build directory). Does this depend on a newer version of gtk? i.e. GtkTreeView doesn't appear to have the particular property, or is this a result of an uninstalled build? $ ./build/rhythmbox/shell/rhythmbox (rhythmbox:6200): GLib-GObject-WARNING **: IA__g_object_set_valist: object class `GtkTreeView' has no property named `show-expanders' Rhythmbox: could not connect to socket Rhythmbox: No such file or directory Rhythmbox-ERROR **: file /home/alex/src/remote-cvs/gnome.org/rhythmbox/sources/rb-sourcelist.c: line 569 (rb_sourcelist_append): assertion failed: (group) aborting...
I installed the build, and got the same problem, so it must be an API change requiring a newer gtk. :-( The patch should then include an autoconf check for the minimal gtk version needed.
I see from http://www.gtk.org/gtk-2.10-announcement.html that it was added in 2.10. I guess there's no way to get that behaviour without upgrading to 2.10? Would it even be possible to put the patch into CVS if it depends on such a recent gtk2? I guess it could be conditionally enabled, but the patch seems to change the structure of the code around a fair bit, is there any way to have a backwards-compatible version for gtk2 < 2.10?
That g_object_set line can just be removed since those properties are the defaults. It is there because in a previous version I was experimenting with some of the non-defaults. It will be removed in the next patch.
Hmm, OK I commented-out the "show-expanders" line, but now I get a new error (running uninstalled): Rhythmbox-ERROR **: file /home/alex/src/remote-cvs/gnome.org/rhythmbox/sources/rb-sourcelist.c: line 569 (rb_sourcelist_append): assertion failed: (group) aborting...
Also happens if the build is installed.
remove the entire rb plugins installation directory (/usr/lib/rhythmbox/plugins or similar), reinstall, and see if it still occurs.
(In reply to comment #11) > remove the entire rb plugins installation directory (/usr/lib/rhythmbox/plugins > or similar), reinstall, and see if it still occurs. Yep, still occurs. (I did a complete "make uninstall" as well as removing the plugins directory manually as well).
Any ideas why this might not work?
It means that you have a source - possibly a plugin source that hasn't been updated to include the source-group. Can you post a backtrace?
When I get a chance to rebuild rhythmbox again with your patch, I'll do just that. The only plugin I'm using other than the standard one's in rhythmbox is my own "Artist/Album" browser (bug #331250) which doesn't create a source of it's own. Is it possible that it is responsible? (I'll disable it when I test your patch again to check).
Created attachment 75189 [details] [review] updated patch Updated for cvs, and adds group for last.fm streaming source
(In reply to comment #16) > Updated for cvs, and adds group for last.fm streaming source Excellent! Patch works for me now. I notice that the expander state (i.e. whether closed or open) isn't saved on restart. But, as noted in comment #3, not being able to reorder playlists within the expander list is an issue, it's very useful to be able to sort them in a particular way and I think a lot of other users will prefer the existing behaviour.
Created attachment 75201 [details] Normal source list, iTunes7 source list, modified source list Here is a mock up of the source list from the patch. Changes: * No header (like iTunes 7) * No triangles for unfolding This would make a beauiful alignment with the browser if bug #339143 (Search should be a toolbar) is fixed.
(In reply to comment #18) > Created an attachment (id=75201) [edit] > Normal source list, iTunes7 source list, modified source list > > Here is a mock up of the source list from the patch. > Changes: > > * No header (like iTunes 7) > * No triangles for unfolding > > This would make a beauiful alignment with the browser if bug #339143 (Search > should be a toolbar) is fixed. I don't understand the point you are making, the second and third panels in the graphic you attached are more or less the same as what we have now in CVS. Adding the disclosure widget (triangles) to be able to collapse sources that have a lot of elements (say many shares or many playlists) is the whole point of the patch.
> I don't understand the point you are making, the second and third panels in the > graphic you attached are more or less the same as what we have now in CVS. I guess I should have described it a little better. Sorry. The first is the unpached source list The second is the patched The third is my idea. I just included the first to for comparation. > Adding the disclosure widget (triangles) to be able to collapse sources that > have a lot of elements (say many shares or many playlists) is the whole point > of the patch. In that case, I withdraw 50% of my suggestion =) So I am still suggesting to have the header removed.
The reason for my somewhat cryptic comment that the expander renderer thing didn't work well is that I couldn't remember why. I suspect it had to do with the fact that if we use that kind of expander for the group and then use a regular expander for sub-sources (import errors and playlists on shares) things look strange. And also there were some subtle bugs in the custom widget. Maybe we should try again. As for reordering playlists, I think we may be able to use a field in the sourcelist model to represent the order and modify it when a drop occurs. There must be some prior art for this somewhere. I agree that things get a little wide with the current patch - part of that is due to space being reserved for the sub-source expanders. Also, it might be nice to remove the ability to collapse the Library group - like iTunes does. I don't see a valid case for being able to collapse it and it may be confusing if it collapsed by accident. One other thing we should change is to make groups that have the same category sort alphanumerically so the order is always the same. For example, now the Store group is sometimes above or below the Playlists.
(In reply to comment #21) > The reason for my somewhat cryptic comment that the expander renderer thing > didn't work well is that I couldn't remember why. I suspect it had to do with > the fact that if we use that kind of expander for the group and then use a > regular expander for sub-sources (import errors and playlists on shares) things > look strange. And also there were some subtle bugs in the custom widget. > Maybe we should try again. I don't think it's too bad to have two different kind of expanders so long as it's clear what they do. It would be useful to experiment with both. > As for reordering playlists, I think we may be able to use a field in the > sourcelist model to represent the order and modify it when a drop occurs. > There must be some prior art for this somewhere. The playlists are reorderable right now. Can't you just keep that code as-is? > I agree that things get a little wide with the current patch - part of that is > due to space being reserved for the sub-source expanders. Yes, I think that the gossip expander would help in that. > Also, it might be nice to remove the ability to collapse the Library group - > like iTunes does. I don't see a valid case for being able to collapse it and > it may be confusing if it collapsed by accident. I think that it would be useful to retain the ability to collapse the "Library" group, for example if you only wanted people to be able play from playlists (say at a party) and/or you had many playlists to display, then it would be helpful to be able use every bit of extra screen real estate. > One other thing we should change is to make groups that have the same category > sort alphanumerically so the order is always the same. For example, now the > Store group is sometimes above or below the Playlists. Yes, we used to have that problem before with shares and removable media, but now the order is fixed, how is that done now?
Patch needs to be updated to CVS, currently fails to apply cleanly because of changes to the lastfm, iradio and audio-cd plugin.
(In reply to comment #23) > Patch needs to be updated to CVS, currently fails to apply cleanly because of > changes to the lastfm, iradio and audio-cd plugin. Even more out of date now, now that DAAP has been turned into a plugin: bug #336150.
If someone wants to help update the patch that would be awesome (and very easy to do). Otherwise, just repeatedly pointing out that the patch hasn't been updated doesn't add any value.
Created attachment 77298 [details] [review] updated patch Just a resync with CVS. Will try other approach separately.
Created attachment 77307 [details] [review] gossip style groups OK I think I figured out one of the reasons why the gossip style didn't work for me before... it doesn't allow expanding rows deeper than the top level. I've just #if 0'd that code out. This seems to work pretty well now. Notes: * I think we should move current child sources of Music out into the Library group * This still doesn't do reordering of playlists (should be able to add as noted in previous comment) * Removed column header * Maybe use some color for background? * Should we add a bit of indentation? Use a column with a xpad * depth width. * Probably requires a recent GTK+
(In reply to comment #27) > gossip style groups Works for me very nicely, thanks! I think this is much nicer than the previous style. > * I think we should move current child sources of Music out into the Library > group I agree, but it would still be useful to have some kind of indication that they are subsets of the main "Music" library. > * This still doesn't do reordering of playlists (should be able to add as noted > in previous comment) Comment #21? > * Removed column header > * Maybe use some color for background? > * Should we add a bit of indentation? Use a column with a xpad * depth width. It would be useful to indent the child sources of "Music" otherwise it's difficult to tell that they are children of "Music". > * Probably requires a recent GTK+ Do you know what the minimum might be? It works fine for me on FC-6 which uses gtk2-2.10.4. Thanks again.
Created attachment 77321 [details] [review] updated - Hide group when empty - Don't use Music as a parent for import errors/missing files - Use icon from theme for Music source - Sort auto playlists before static ones and then collate
(In reply to comment #27) > I think we should move current child sources of Music out into the Library group This sounds good to me; having a slightly different icon (a small emblem composited onto the Music icon?) might be useful. > Should we add a bit of indentation? Use a column with a xpad * depth width. This would be very nice, since for some sources "A[BC]D", you can't currently tell which ones are inside A when it's expanded. A few more notes: The "Store" category should probably be "Stores", since the groups have either plural names or container names like Library. When you click on the Music source to view the contents, it always performs a expand/collapse, rather than just when you click the disclosure triangle. If you expand Store and then create a new playlist, the Playlists group moves up a bit and covers the Magnatune store row partially. I have no idea why.
Created attachment 77350 [details] [review] updated - Rename Store to Stores - Add two levels of indentation James: not sure why the store would move around - I'm not seeing it. However, I added a bit more logic to the compare rows function that may help. I think your idea about the icons is fine - but maybe import errors won't be just music one of these days... Also one of the next things I'd like to do (after resurrecting the friendly time) is to freshen all the icons. Any objections to getting this path in?
...sorry, treeviews on the mind. I meant "patch in".
In gtk 2.8, the 'show-expanders' and 'gtk-enable-animation' properties don't exist. Since show-expanders doesn't exist, it adds a lot of space to the left of the source icon, which sucks a bit. Also, rb-sourcelist.c:compare_rows calls g_object_unref on NULL pointers.
(In reply to comment #32) > ...sorry, treeviews on the mind. I meant "patch in". If this could be made to be backwards-compatible (or provide some kind of fallback) for gtk 2.8 (which still ships as the most recent gtk on many currently maintained distributions such Fedora Core 5) as per comment #33, I would be happy. I'm currently usin FC-6, but I a FC-5 box which I also use on rhythmbox and I suspect many users still compile from CVS on FC-5. If gtk of 2.8 becomes a hard requirement (currently the hard requirement is set at gtk 2.6), then the configure.ac should be updated accordingly.
Created attachment 77399 [details] [review] update for gtk+2.8 This makes it work correctly (as far as I can tell) with gtk+ 2.8 by adding a hidden treeview column if the show-expanders property does not exist. If gtk-enable-animations doesn't exist, it just disables animations. I haven't tried this with gtk+ 2.6. Also fixes the g_object_unref criticals mentioned above, and sets the source group for missing-files and import-errors.
I'm getting some non-fatal warnings with patch #77350: (rhythmbox:14311): Rhythmbox-WARNING **: source 0x8e7be20 has no group (rhythmbox:14311): Rhythmbox-WARNING **: source 0x8ea4058 has no group Rhythmbox: could not connect to socket Rhythmbox: No such file or directory /usr/local//lib/rhythmbox/plugins/magnatune/__init__.py:94: Warning: g_object_unref: assertion `G_IS_OBJECT (object)' failed shell.append_source(self.source, None) # Add the source to the list sys:1: Warning: g_object_unref: assertion `G_IS_OBJECT (object)' failed sys:1: Warning: g_date_set_julian: assertion `g_date_valid_julian (j)' failed sys:1: Warning: g_date_get_year: assertion `g_date_valid (d)' failed sys:1: Warning: g_date_set_dmy: assertion `g_date_valid_dmy (day, m, y)' failed sys:1: Warning: g_date_get_julian: assertion `g_date_valid (d)' failed sys:1: GtkWarning: gtk_tree_store_move: assertion `!GTK_TREE_STORE_IS_SORTED (tree_store)' failed /usr/local//lib/rhythmbox/plugins/magnatune/__init__.py:130: Warning: g_object_unref: assertion `G_IS_OBJECT (object)' failed self.source.delete_thyself() Also playlist reordering still doesn't work. I think it shouldn't go into CVS until that works because losing that feature would be a major regression, IMHO.
Patch #77399 makes the warnings go away, but it loses the indendation and "Store" -> "Stores" change from previous patch.
Created attachment 77406 [details] [review] merged patch Merges the changed together
With gtk+ 2.10, I get this warning on startup: (rhythmbox:26900): GLib-GObject-WARNING **: g_type_instance_get_private() requires a prior call to g_type_class_add_private() in rb_sourcelist_model_init(). Don't understand why at all. and with gtk+ 2.8, I get this when enabling the magnatune plugin after startup: sys:1: GtkWarning: file gtktreeview.c: line 4973 (validate_visible_area): assertion `has_next' failed. after which everything below the 'library' section disappears. It works properly if the magnatune plugin is already enabled at startup time. also, I misspelled 'persistent' when I first added the source list grouping concept.
Created attachment 77425 [details] [review] updated Thanks for the good feedback. - Includes Jonathan's fixes - Change persistant to persistent Strange, I don't get any warnings when running (gtk 2.10). Regarding the reordering... I guess my bias is that I prefer the software to manage the ordering for me in a way that is repeatable and predictable. So, if my playlist is named 'Xylophone Rock' I have a really good idea how far to scan down the list for it. As a data point it seems like this is how itunes does it too.
Created attachment 77427 [details] [review] updated - include the gtk-enable-animations fix too
It's probably just something weird in my jhbuild environment. No warning on my ubuntu install. I'll look into the gtk 2.8 problems later. I don't really have an opinion on playlist reordering. I currently use a specific non-alphabetical order, but it wouldn't bother me if I lost the ability to do that.
New patch works for me. It still has the problem that clicking on source groups expands/contracts the sources, not just clicking triangle as James points out in comment #30. Also it would be nice if the disclosure state was saved after closing and restarting. Regarding the playlist reordering, it would be nice if an option (maybe a gconf key) could be kept to preserve the old behaviour. I think it would be good to get wider feedback on this change on the mailing list.
The gtk 2.8 problem I mentioned seems to have disappeared. I probably screwed up merging the patches somehow.
Created attachment 78651 [details] [review] patch - Sync to CVS - Only activate when clicking on cell not entire row. This is needed for handling sources with children (eg. DAAP) Look good enough to commit?
I would prefer waiting until playlist reordering is included and/or getting feedback on the mailing list about that before committing. From my perspective loss of playlist reordering is a regression, I use that ability quite a lot to organise my playlists in groups that aren't necessarily alphabetical. At the very least make it an option available by gconf.
The more I think about it and talk to people the more I am convinced that simply ordering alphabetically is the right way to go. So, I have no plans to add reordering. Very simply, the software should be doing the managing. If the only thing you want to do it grouping then can't you just do something like the following? Funk - Playlist 1 Funk - Playlist 2 Rock - Playlist 1 Rock - Playlist 2 Even for your use case I think this is way better then having to manually order them. You always know where to look for them. For non-geeks the order that the playlists are in is incredibly uninteresting. What is interesting is: - being able to locate one simply - not having to spend time to manually keep them organized and arranged Automatic sorting makes it logically simple to understand where a new playlist will be located rather than having to drag to the correct spot. The logical extension of the ability to reorder is having tools to help reordering like options for "sort alphabetically" etc. That seems crazy to me.
Nevertheless, that's still your opinion and it removes an existing feature and works differently from what currently exists, which is exactly why it should be discussed more widely than just this bug. Canvassing on the mailing list will be a good way of shaking these issues out. I can post a thread with the link to the bug here if you would prefer not to post your own thread.
What if it was sorted alphabetically as default, but the order could be changed by DnD?
Source tree surgery broke this patch. I'll update it soon.
Created attachment 79831 [details] [review] update
Created attachment 83479 [details] [review] updated Fix for recent svn changes. Move Stores group to FIXED position (ie. above playlists).
I'd be happy for this to be committed to cvs, with a thread on the ML asking for people's opinions on it. Having people actively using it is more likely for them to figure out whether they actually like it, rather than just whether they think they will. Regarding manual sorting of playlists, do people actually want to sort them manually, or do they just want to group certain playlists together? If it's the latter, something like "playlist folders" might be what we want.
Great. Committed and email sent to list. Thanks!
Appearance of expander cell renderers should change on mouseover: bug 412852.
Manual ordering of playlists is a great feature. Personally, I don't have a hard set of rules for ordering, but I know what stuff I want at the top.