Created attachment 107531 [details] [review]
adds RBLibraryChildSource

I don't think RBLibraryChildSource should be a subclass of RBAutoPlaylistSource - we currently use an auto playlist there just because it was easy to.  Instead, I think it should be a subclass of RBBrowserSource, and should construct the query model itself based on the entry type of the parent source and the path.

Regarding the gconf keys, I think it would be best to use separate keys for browser visibility (impl_get_browser_key), sort order (sort-key property on RBBrowserSource), and the browser position (impl_get_paned_key) for each child source.  Using gconf_escape_key(path) is probably a reasonable idea.  So, the browser key would be /apps/rhythmbox/state/library/escaped_path/show_browser, the sorting key would be /apps/rhythmbox/state/library/escaped_path/sorting, and the pane position key would be /apps/rhythmbox/state/library/escaped_path/paned_position.

Created attachment 112556 [details] [review]
RBLibraryChildSource, subclassing RBBrowserSource

Reimplementation of RBLibraryChildSource as Jonathan suggested above.

I changed rb_browser_source_do_query (rb-browser-source.c) to derive search queries from the RhythmDBQuery associated with the "base-query-model" property - does that make sense? Otherwise I'd have to reimplement the search related stuff in RBLibraryChildSource...

Works good so far, except for one thing I don't like yet: Child sources are populated with the whole unfiltered library initially, with songs not belonging to them being removed as the query thread kicks in. I haven't looked deeply into it yet, but I guess this is because RBBrowserSource uses an unfiltered RhythmDBQuery in its constructor, which is then overridden in RBLibraryChildSources constructor.

I think what we need to do there is to add a 'query' construction property on RBBrowserSource that would be used to construct the base query model for the source.  I haven't looked at how well that would work, it's just a though.

A couple of minor things:
- if the dispose method doesn't do anything, you don't need to have one
- g_free checks if the argument is NULL, and finalize can only ever be called once, so you can just g_free the strings
- I don't think there's much to be gained by lazily constructing the gconf keys; might as well just do it in the constructor, which would probably make the code a bit simpler

Thanks for reviewing, Jonathan.

(In reply to comment #4)
[...]
> - I don't think there's much to be gained by lazily constructing the gconf
> keys; might as well just do it in the constructor, which would probably make
> the code a bit simpler

The reason I did it this way is that both impl_get_browser_key and impl_get_paned_key have to be ready when the constructor is called, because they are used in the constructor of RBBrowserSource.

Is there a reason why these two methods are implemented as virtual methods and not as construction properties (like the "sorting-key" is)? At least in this case it would be easier if they were construction properties.

In that case, I guess you could construct the gconf keys when the 'path' property is set, which happens in the GObject constructor, before it returns to the subclass constructor methods.

Created attachment 112918 [details] [review]
RBLibraryChildSource, updated

Changes since previous patch:

* lib/eel-gconf-extensions.(c|h):
- added eel_gconf_recursive_unset (used by RBLibraryChildSource: impl_delete_thyself)

* sources/rb-library-child-source.(c|h):
- fixed dispose and finalize methods (comment #4)
- gconf keys are now constructed when the 'path' property is set (comment #6); fixed impl_get_browser_key and impl_get_paned_key accordingly
- removed now unneeded constructor
- added impl_delete_thyself method which can delete songs from the db and delete state keys from gconf

* sources/rb-library-source.c:
- fixed the rb_library_source_sync_child_sources logic: delete child sources (including removal of songs from the db and deletion of state keys from gconf) only if their location was removed; remove last child source (keeping songs and state keys) if there is only one location left.

* sources/rb-browser-source.c:
- added a 'query' construction property which is used when constructing the
'base-query-model' (comment #4). If the 'query' property is unset, a default query is used for backward compatibility.

I'm not sure that removing a library child source should remove songs from the database.  If we had an actual UI for managing them, we could ask the user, but without that it seems like a bit too much of an unintended consequence.

Apart from my concern about that, the patch looks fine now.

A use case:

Say we've got three library locations, /A /B and /C. We can access all songs in these three locations through the library source, and the songs in each location through their respective child sources, A, B and C.
Now we remove location /B from our library locations.

In this case, I expect child source B to be removed, and I don't expect to find songs from that location in the library source any longer - that's why I think the songs should be deleted from the database. If they were not deleted, we would find songs in our library source from a location we explicitly removed from our library locations.
State keys for child source B should be removed from gconf as well.

So, now there are two locations left, /A and /C.
Finally, we remove another one, say /A.

Now I believe child source A should be removed and its songs and state keys should be deleted - same as above.

But there is only one location left now: /C.

Both the library source and child source C now hold the same songs from location /C, the sources are redundant. So child source C should be removed as well, but, as it is the last location, without deleting songs from the database - we still want to be able to access them through the library source. And, we want to keep its state keys in gconf, because if another location is added, child source C will be added again.


The only case I can think of where deleting songs from the database might cause unexpected behaviour is if you added both a folder and its parent folder (e.g. /A and /A/B) to your library locations - but that seems to be useless anyway.

Jonathan,

I think I've got it now: You meant the entries should be hidden rather than deleted from the database, right? Please ignore my previous comment, then...

So, the right way to go would be to do it similar to missing files, i.e. hide the entries, set a new property RHYTHMDB_PROP_LOCATION_REMOVAL_TIME and hack RhythmDB to remove the entries from the database if location_removal_time + grace_time < now, right?

No, I don't think the entries should be hidden.  I don't think anything should happen to them.

I guess what I'm missing is a clear idea of why people want multiple library locations (I have my music scattered across a few different places, but I don't really want all of those to be 'library locations'), and what why they'd remove one of them.

Without that, it doesn't seem obvious to me that any files under the location that was removed should themselves be removed, and since it's trivial to do that manually (select all, right click, remove) it doesn't seem like we'd gain anything by doing it.

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

Created attachment 131359 [details] [review]
Updated patch for current svn

Same as last version, updated to apply against current svn. GnomeVFS usage replaced with GFile.

Created attachment 132420 [details] [review]
RBLibraryChildSource and new library location prefs UI

New library location preferences dialog:
* add/remove multiple library locations, asking if tracks and meta data should be kept if a location is removed
* select one of the locations as "default location", this is where tracks e.g. ripped from an audio cd are put
* per-location setting "watch for new files"

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

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

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

I haven't done a detailed review of this yet, but I hope to soon.

It looks like the code in rb-library-source.c for running the preferences widget is getting large and complex enough to be moved to a separate file.

I don't see why the file chooser used to select library locations has multiple selection mode enabled.  It complicates the code, and I can't really see the use of it.  

show_invalid_locations_error_dialog should know exactly why a given location is not valid, it shouldn't give a list of possible reasons.  If the problem is that an ancestor or descendent of the location is already configured as a library location, the error message should specify which one.  Unless there's some important use for adding multiple locations at once, it should only ever have a single location to talk about anyway.

The various places where you check for NULL pointers in various tree models, and check that child sources have non-NULL paths, should (as far as I can see) all be assertions.  We shouldn't silently ignore problems like that.

It's a bit confusing that sync_default_library_location_internal is less internal than sync_default_library_location.  I understand why it's named that way, but I think these functions need better names.  Alternatively, sync_default_library_location could take a parameter that specifies whether to remove and replace the notification handler.

If CONF_MONITOR_LIBRARY_LOCATIONS is not set, we should check CONF_MONITOR_LIBRARY and set CONF_MONITOR_LIBRARY_LOCATIONS based on that.  Otherwise, users upgrading from a previous version will have library monitoring unexpectedly disabled.

(In reply to comment #18)

I updated the patch according to your suggestions:

> It looks like the code in rb-library-source.c for running the preferences
> widget is getting large and complex enough to be moved to a separate file.

Done. I moved the functions for building file paths to a separate file, as well, because they are used both by RBLibrarySource and the preferences ui code.
 
> I don't see why the file chooser used to select library locations has multiple
> selection mode enabled.  It complicates the code, and I can't really see the
> use of it.

Users might want to add more than one location at a time. And whenever I'm in a situation where I want to do multiple things at once, but the app does not allow me to do it, forcing me to dully repeat the same actions several times to reach my goal, I get annoyed - even if it's just a few mouse clicks. That's why I'd like to keep multiple selection mode.
 
> show_invalid_locations_error_dialog should know exactly why a given location is
> not valid, it shouldn't give a list of possible reasons.  If the problem is
> that an ancestor or descendent of the location is already configured as a
> library location, the error message should specify which one.  Unless there's
> some important use for adding multiple locations at once, it should only ever
> have a single location to talk about anyway.

Agreed, the dialog now shows the exact reason for every failed location.

> The various places where you check for NULL pointers in various tree models,
> and check that child sources have non-NULL paths, should (as far as I can see)
> all be assertions.  We shouldn't silently ignore problems like that.

Fixed.

> It's a bit confusing that sync_default_library_location_internal is less
> internal than sync_default_library_location.  I understand why it's named that
> way, but I think these functions need better names.  Alternatively,
> sync_default_library_location could take a parameter that specifies whether to
> remove and replace the notification handler.

I took the extra parameter road, both for sync_default_library_location and sync_monitored_locations.
 
> If CONF_MONITOR_LIBRARY_LOCATIONS is not set, we should check
> CONF_MONITOR_LIBRARY and set CONF_MONITOR_LIBRARY_LOCATIONS based on that. 
> Otherwise, users upgrading from a previous version will have library monitoring
> unexpectedly disabled.

Done.

Created attachment 133509 [details] [review]
Updated version

(In reply to comment #19)
> > I don't see why the file chooser used to select library locations has multiple
> > selection mode enabled.  It complicates the code, and I can't really see the
> > use of it.
> 
> Users might want to add more than one location at a time. And whenever I'm in a
> situation where I want to do multiple things at once, but the app does not
> allow me to do it, forcing me to dully repeat the same actions several times to
> reach my goal, I get annoyed - even if it's just a few mouse clicks. That's why
> I'd like to keep multiple selection mode.

As far as I can see, this would only allow you to select multiple directories that are immediate children of the same directory.  That doesn't sound useful to me.

This is getting to the point where it's too hard to review it as a diff.. anyway, I think it's pretty close now.

(In reply to comment #21)
> (In reply to comment #19)
> > > I don't see why the file chooser used to select library locations has multiple
> > > selection mode enabled.  It complicates the code, and I can't really see the
> > > use of it.
> > 
> > Users might want to add more than one location at a time. And whenever I'm in a
> > situation where I want to do multiple things at once, but the app does not
> > allow me to do it, forcing me to dully repeat the same actions several times to
> > reach my goal, I get annoyed - even if it's just a few mouse clicks. That's why
> > I'd like to keep multiple selection mode.
> 
> As far as I can see, this would only allow you to select multiple directories
> that are immediate children of the same directory.  That doesn't sound useful
> to me.

True. But the code is there and working. I can switch it to use single selection mode, if you prefer simpler code over this admittedly rarely used feature.

> This is getting to the point where it's too hard to review it as a diff..
> anyway, I think it's pretty close now.

I created a repo at github.com:
git://github.com/christianbecke/rhythmbox.git
The branch I'm working on is called 'multiple-libraries'. Does this help?

Generally I prefer simpler code wherever possible.

Thanks for creating the git repo.  I was going to do that myself, but it's easier this way.

I just pushed an update to the repo at github. I consider the patch as feature-complete, now, so please review, and I'll do my best to iron out any issues you find.

Changes since last version:

* added drag'n'drop support to RBLibraryChildSource
* switched the library location file chooser to single selection mode and cleaned up rb-library-preferences.c accordingly
* added support for moving files from one RBLibraryChildSource to another

I think the source methods you've added for moving files are a bit too complicated for what they do.  I can't really see any other use for them, or other sources that would implement them.

I think it'd be better to have the library source manage the whole process, so you don't have the move completion callback method, which feels a bit too awkward to me.  There's nothing wrong with making the shell-clipboard object aware of different source types, and there's nothing wrong with adding new methods to the library source.

do_paste in rb-library-child-source.c is virtually identical to impl_paste in rb-library-source.c, so I think the child source should just pass that up to the library source, specifying the path to copy to.  Same with impl_receive_drag and probably impl_can_paste too.

in rb-library-preferences.c:

    dialog = rb_file_chooser_new (_("Choose Library Location"), 
                                  GTK_WINDOW (source->priv->shell_prefs),
                                  GTK_FILE_CHOOSER_ACTION_SELECT_FOLDER, FALSE);

I guess this should be 'choose new library location' to make it clear that adding a new library location will not remove any old ones.

The message displayed in show_remove_library_location_message_dialog is a bit confusing and probably too long.  Here's my attempt:

"Would you like to remove all songs stored at this location from your library?  If you click 'remove songs', the songs will not be deleted from disk, but will no longer appear in the library.  If you click 'keep songs', the songs will remain in your library."

.. which means I'd also prefer 'remove songs' and 'keep songs' buttons to the checkbox.

rb_library_child_source_set_keep_songs_and_state is a bit ugly.  Maybe it'd be better to move the delete-songs-and-state code to a new method and just call that when the user hits the 'remove songs' button.

retitling this since it's gone way beyond the original scope.

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

I pushed an update to github, now using GtkBuilder and following some of your suggestions:

(In reply to comment #25)
> I think the source methods you've added for moving files are a bit too
> complicated for what they do.  I can't really see any other use for them, or
> other sources that would implement them.
> 
> I think it'd be better to have the library source manage the whole process, so
> you don't have the move completion callback method, which feels a bit too
> awkward to me.  There's nothing wrong with making the shell-clipboard object
> aware of different source types, and there's nothing wrong with adding new
> methods to the library source.

There is at least one other source which could use the move files methods: the local files source from a plugin by Thomas Zander <riggs@rrr.de>. Unfortunately I can't find it anywhere on the web, I've got a local copy, though.

Anyway, I reimplemented moving files as you suggested.
 
> do_paste in rb-library-child-source.c is virtually identical to impl_paste in
> rb-library-source.c, so I think the child source should just pass that up to
> the library source, specifying the path to copy to.  Same with
> impl_receive_drag and probably impl_can_paste too.

What exactly do you mean by "pass that up to the library source"? Should I refactor impl_receive_drag, impl_can_paste and impl_paste in rb-library-source.c, and add rb_library_source_child_receive_drag, rb_library_source_child_can_paste and rb_library_source_child_paste as public methods, which are called from rb-library-child-source?

> in rb-library-preferences.c:
> 
>     dialog = rb_file_chooser_new (_("Choose Library Location"), 
>                                   GTK_WINDOW (source->priv->shell_prefs),
>                                   GTK_FILE_CHOOSER_ACTION_SELECT_FOLDER,
> FALSE);
> 
> I guess this should be 'choose new library location' to make it clear that
> adding a new library location will not remove any old ones.

Fixed.
 
> The message displayed in show_remove_library_location_message_dialog is a bit
> confusing and probably too long.  Here's my attempt:
> 
> "Would you like to remove all songs stored at this location from your library? 
> If you click 'remove songs', the songs will not be deleted from disk, but will
> no longer appear in the library.  If you click 'keep songs', the songs will
> remain in your library."
> 
> .. which means I'd also prefer 'remove songs' and 'keep songs' buttons to the
> checkbox.

Fixed.
 
> rb_library_child_source_set_keep_songs_and_state is a bit ugly.  Maybe it'd be
> better to move the delete-songs-and-state code to a new method and just call
> that when the user hits the 'remove songs' button.

Currently, all changes made using the library preferences dialog only change gconf values directly. The changes are then picked up by rhythmbox in response to gconf notifications. If we want to stick to this scheme for library locations, we have to track for which child sources songs should be deleted from rhythmdb. IMO, the best solution for this is some kind of flag stored in the child source.
Are you against sticking to this scheme, or is it just the method? What about storing the flag in a GObject property, so instead of calling rb_library_source_set_keep_songs_and_state (child_source, TRUE) we would use g_object_set (child_source, "keep-songs", TRUE, NULL)?

So, what's the status now?

I cleaned up the patch a bit, my branch can now be found at

git://github.com/christianbecke/rhythmbox-multiple-libraries.git

and should merge cleanly with current HEAD.

Jonathan, I'd appreciate if you found the time to review - because my patch moves a lot of code from rb-library-source.c to new files, keeping it up to date is quite laborious whenever there are changes to rb-library-source.c.

Apologies for the delay, things fall off my radar all too easily.

I think the basic library child source changes (commits f6dde885..5ab1f898 in your repo) are pretty much ready to commit, so I'd like to focus on getting that in.  The stuff about moving files between library locations raises bigger questions about how track transfer should really work.

f6dde8859d5d91a02cdaf53f1b7b738a081c3e8c and fc55bf6477ffc51abadf5b4b81b59682c2836d5a are fine as is (I'd probably like a bit more explanation of what the RBBrowserSource query property is for in the commit message though).

5ab1f8983adcc0144edbd40b90e21e14527b46b0:

:::data/rhythmbox.schemas

+        <short>Directory where Rhyhtmbox should put files</short>

Rhythmbox is misspelled here

:::sources/rb-library-child-source.*

Is the 'path' parameter/property/member actually a path, or is it really a URI?  It should be a URI, and it should be named accordingly.

(In reply to comment #28)
> > rb_library_child_source_set_keep_songs_and_state is a bit ugly.  Maybe it'd be
> > better to move the delete-songs-and-state code to a new method and just call
> > that when the user hits the 'remove songs' button.
> 
> Currently, all changes made using the library preferences dialog only change
> gconf values directly.

No, it also sets the keep_songs_and_state flag.

> The changes are then picked up by rhythmbox in response
> to gconf notifications. If we want to stick to this scheme for library
> locations, we have to track for which child sources songs should be deleted
> from rhythmdb. IMO, the best solution for this is some kind of flag stored in
> the child source.
> Are you against sticking to this scheme, or is it just the method? What about
> storing the flag in a GObject property, so instead of calling
> rb_library_source_set_keep_songs_and_state (child_source, TRUE) we would use
> g_object_set (child_source, "keep-songs", TRUE, NULL)?

You're not sticking to this scheme as it is.  You're already calling directly from the properties dialog code into the library child source, so why not just remove the songs directly rather than setting a flag to tell something else to do it later?  I don't see any meaningful difference other than that one is more complicated and harder to understand than the other.

Instead of rb_library_child_source_set_keep_songs_and_state(), I'd just have a function rb_library_child_source_remove_songs_and_state(), and call that when the user hits the 'remove' button.

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

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

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

Um, just wondering what the status is on this.

You guys seemed like you were doing great work and Christian's patch was ready 7 months ago.  Was the effort dropped for some reason? 

Thanks for any update you can give.

Sorry for the delay, I've been busy with other things the last months.

Jim: My patches needed some final touches (see comment 31).

Jonathan: I pushed updated patches to http://wiki.github.com/christianbecke/rhythmbox-multiple-libraries
I hope it is all right now. It still contains the track transfer stuff as a separate commit, because I use it. Just ignore it for now.

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

Given that a huge portion of users probably encounter the "Multiple locations set" textbox when trying to configure the app, it'd be nice to see this changed to a user-friendly list that the user can edit, without using gconf-editor.

Something so core that the average user will want to change, should be handled within the app itself, not in an external application. As it stands I don't even have an "/apps/rhythmbox" config entry (Ubuntu 12.04), which nicely represents the fragility of such a solution to the problem.

The reason why on Ubuntu it's more evident than other distribution is because by default we already preset multiple locations: ~/Music & Ubuntu One music store purchases folder.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/rhythmbox/issues/532.