Re: [Rhythmbox-devel] Rhythmbox archive

[Jonathan Matthew <jonathan kaolin hn org>]

On Sun, Jun 05, 2005 at 09:24:40PM +0200, Christophe Fergeau wrote:
> Hi,
> 
> Le vendredi 03 juin 2005 à 09:13 +1000, Jonathan Matthew a écrit :
> > > Fwiw, I grabbed jonathan's --bugs-- branch as separate patches and
> > > started to review them individually, I have selected 60/70% of those
> > > which can go into mainline as is, and I still have to look more
> > > carefully at the other ones. 
> > 
> > Thanks!  I wasn't looking forward to going through all of that.
> > Please let me know if there's anything I can do to help here.  I know my
> > patch summaries aren't very descriptive, so I might need to clarify some
> > things.
> 
> I put the patches that look fine to me at
> http://cfergeau.free.fr/rb-patches/
> 
> http://cfergeau.free.fr/rb-patches/remaining/ contains the patches I
> still need to look at (unsurprisingly, those are the biggest ones ;)

I'll try to explain what I was thinking when I made each of these.

patch-5:  
	effectively undone by patch-6, as far as I can tell.

patch-6: 
	The playlist hashtable was getting out of sync with the query
	model when entries were dropped into the playlist, since that
	didn't go through rb_playlist_source_add_location.  By updating
	the hashtable in signal handlers for the query model row
	insert/delete signals, it's always in sync.

patch-9:
	obsolete, fixed (better) in CVS.
	
patch-12:
	Rather than indirectly linking librb.la (and some others)
	multiple times, just get the link order right.  This makes
	partial rebuilds work properly (I was having some weird problems
	before this), and also eliminates a lot of link warnings on some
	platforms.

patch-13:
	Without this, rhythmbox ignores a lot of shoutcast streams
	because it can't stat the stream - the gnome_vfs_get_file_info
	call fails with GNOME_VFS_ERROR_GENERIC.  I don't like it, but
	at least it works.

patch-15:
	Allows iradio streams to be added via drag and drop from
	browsers (_NETSCAPE_URL) and elsewhere.  Doesn't use rb-tree-dnd
	because you shouldn't be able to reorder the stations, it's just
	there so you can drag things in from outside.
	(bug #158717)

patch-21:
	Extracts the list of genres from the current iradio stations to
	populate the genre field in the new iradio station dialog.

	Also changes the new station dialog to only require the stream
	location, not the genre or title, because those can be read from
	the stream itself.

patch-22:
	Just an experiment.  Uses natural sort order for album titles,
	so if you have albums numbered 'fnord vol. 1' through 'fnord vol. 20', 
	they get sorted into ascending order as if you'd zero-filled the
	numbers.  Doesn't really make much difference.
	(bug #158599)

patch-25:
	Currently, the menu popup shown by the sourcelist when you
	right-click on it is the popup for the selected source, not the
	source the pointer is over.  This is inconsistent with the rest
	of the universe, where right clicking on a non-selected item
	selects the item and then shows the popup.

	Adds popups for the iradio source (to add a new station) and the
	blank space in the source list (to create new playlists).

	I'm not sure what the change to rb-shell-player.c is doing
	there.  It should be removed.

	(bug #131340)

patch-31:
	Currently, clicking on a playlist name allows you to rename it.
	It's easy to do this accidentally, and it's not really a common
	thing to do.  This change removes this behaviour and adds an
	item to the menu popup to do renames.  This makes playlist
	renaming work like file renaming in nautilus.
	(bug #133845)

patch-35:
	Only update the rhythmdb entry for an iradio station if it's
	actually been edited.  I'm not sure if updating the db entry was
	causing any problems, but there's no point doing it anyway if
	there are no changes.

patch-36:
	Show popups in the entry view when shift-f10 or menu key is
	pressed, for consistency with nautilus etc.
	(bug #152650 (well, half of it, at least))

patch-37, patch-41:
	If there are multiple screens (not Xinerama), the tray icon is
	only shown on the one rhythmbox is running on.  This was an
	attempt at fixing that, but I don't think it was entirely
	successful.  This should probably be left out until I get it
	right.

patch-38:
	Changes the way signals are emitted from
	rhythmdb_query_model_entry_changed_cb so when an entry is
	changed such that it no longer matches the query, signal
	handlers can determine what actually happened.  I'm not 100%
	sure, but I think this fixes the "crash on editing iradio
	properties" bugs that keep getting reported (#160358).

	The query model now emits the entry-prop-changed signal, then
	a new entry-removed signal (which is different from row-deleted
	as it indicates that the entry has only been filtered out by the
	query, not deleted).

	Prior to this, the property model could not update its
	value reference counts correctly when a changed entry no longer
	matched the query, because it never got the 'old' property
	value.  This usually caused crashes.
	
patch-40:
	Adds a playback error display to the iradio station properties
	dialog, to match the playback error display in the song
	properties dialog.
	
patch-43:
	Attempts to improve the tray icon's tracking of the main window
	state, so it can remember whether the main window was maximized
	before it was hidden. (#145268)
	
> The only patch I didn't like so far is patch-10, see below for the
> notes I took at the time :
> 
> --- orig/configure.ac +++ mod/configure.ac @@ -38,6 +38,8 @@
>  
>  PKG_PROG_PKG_CONFIG
>  
> +PKG_PROG_PKG_CONFIG +
> 
> Unneeded

Probably just the result of a botched merge, since I (stupidly) first
made these changes in another branch and merged them in.

> +if USE_CD_BURNER
> +librbplayer_la_SOURCES += 	\
> +	rb-recorder.h		\
> +	rb-recorder-gst.c
> +
> +INCLUDES += $(LIBNAUTILUS_BURN_CFLAGS)
> +
> 
> This breaks the xine backend, which I removed from my playbin branch but
> still seems to be present in the bug branch, so it's not really a big
> issue, but still ;)

Not breaking the xine backend wasn't particularly high on my list of
priorities..

thanks,
-jonathan.