Re: [Muine] My first thoughts on the Muine code

[Jorn Baayen <jbaayen gnome org>]

Hi,

On Sun, 2004-12-26 at 01:57 -0800, Tamara wrote:
> So I had some extra time this weekend I began looking at the Muine
> source. I have a few questions:

Thanks! A second eye going over the code always yields a bunch of good
ideas. 

> StringUtils.StrCmp () wraps our beloved strcmp () but I don't see why.
> String.Compare () didn't work (I didn't look at it that close) but
> String.CompareOrdinal () appears to work just fine. One less jump to
> unmanaged code is good. I'm not sure why String.Compare () didn't work
> though.

This is because we are dealing with g_utf8_collate sort keys here, which
needs a strcmp which just deals with the very basic chars, not the
unicode values or something like that. CompareOrdinal seems indeed like
just what we need, I didn't know about it - fixed in CVS.

> Also I wrote a quick little static method to help with getting GConf keys:
> 
> public static object GetGConfValue (string key, object default_value)
>         {
>                 object gconf_value;
>                 
> 		try {
> 			gconf_value = GConfClient.Get (key);
> 		} catch {
> 			gconf_value = default_value;
> 		}
>                 
>                 return gconf_value;
>         } 
> 
> This enables us to replace the repetitive code:
> 
> int width;
> try {
>         width = (int) Muine.GConfClient.Get
> ("/apps/muine/playlist_window/width");
> } catch {
>         width = 500;
> }
> 
> with:
> 
> int width = (int) Muine.GetGConfValue
> ("/apps/muine/playlist_window/width", 500);
> 
> Which is much more concise and easy to understand what is going on.
> However, I'm not sure where to put it so I just dumped it in Muine.

Excellent idea. Muine seems like a fine place for it- would love the
patch.

> 
> I also redid the SignalDelegates and wrapped g_signal_connect_data and
> put it all into a new class called SignalUtils.
> 
> This allows us to replace the use of nested classes in Player
> 
> tick_cb = new IntSignalDelegate (TickCallback);
> eos_cb = new SignalDelegate (EosCallback);
> error_cb = new StringSignalDelegate (ErrorCallback);
> 
> ConnectInt.g_signal_connect_data (Raw, "tick", tick_cb,
>                                                           IntPtr.Zero,
> IntPtr.Zero, 0);
> Connect.g_signal_connect_data (Raw, "end_of_stream", eos_cb,
>                                                      IntPtr.Zero,
> IntPtr.Zero, 0);
> ConnectString.g_signal_connect_data (Raw, "error", error_cb,
>                                                      IntPtr.Zero,
> IntPtr.Zero, 0);
> 
> with something much cleaner:
> 
> tick_cb = new SignalUtils.SignalDelegateInt (TickCallback);
> eos_cb = new SignalUtils.SignalDelegate (EosCallback);
> error_cb = new SignalUtils.SignalDelegateStr (ErrorCallback);
> 
> SignalUtils.SignalConnect (Raw, "tick", tick_cb);
> SignalUtils.SignalConnect (Raw, "end_of_stream", eos_cb);
> SignalUtils.SignalConnect (Raw, "error", error_cb);

Again, excellent idea. 

> 
> If these things sound interesting to you, I'll post a patch up on
> Bugzilla. In some more experimental work I did, I factored out
> AddSongWindow and AddAlbumWindow to inherit from AddWindow and I'm
> working on splitting PlaylistWindow up into parts (currently I
> separated out the menu system).

If you could post a separate patch for the two issues you outlined
above, and the experimental work, that'd be great. The first two issues
are sure commits, the other stuff I'd first have to see. AddWindow
abstraction definetely sounds good ..

> Speaking of the menu system, why did Muine switch from Glade to
> forming the menu manually with Gtk.ActionEvent?

To be able to use GtkActions- now keeping the main menus and the tray
icon menu in sync is a lot easier, and it's gonna be very useful too
when we get plugin support one day. It also obsoletes a lot of hacks
that were needed to get the custom stock icons into the glade menus,
etc.

> My long term project is to rip libmuine out and use as much managed
> code as possible.

:)  I think ripping libmuine out at this point is impossible, due to the
gsequence stuff, tagging stuff, etc, but if you find any more
possibilities to reduce unmanaged code, I'd love to hear.


Thanks for your work!

Jorn