GNOME Bugzilla – Bug 620008
Replace the homegrown Log.cs by Hyena.Log
Last modified: 2010-06-01 14:35:49 UTC
This is an easy task: we currently have a homegrown Log.cs class in src/Utils/. I've just pulled in Hyena, which is a library of reusable routines from Banshee. We're slowly going to migrate anything we can reuse towards this. So the task is simple: remove Log.cs (git rm Log.cs and remove it from Makefile.am), then fix anything that breaks by invoking the right methods in Hyena.Log; Some pointers: * Please use "using Hyena;" and "Log.Debug (...)" instead of "Hyena.Log.Debug (...)". * Hyena also has message levels (debug / information / ...) too, be sure to translate accordingly.
I'll do it.
Awesome, let me know if you run into problems (you'll need to add a reference to Hyena.dll, Mike (tigger) has already tackled this problem, talk to him about it. I'll make sure that gets merged ASAP.
Created attachment 162271 [details] [review] Add build variables for Hyena To integrate Hyena in the build, you can use this patch. It introduces the variables to link hyena. So, you still have to edit src/Metadata.am and add $(LINK_HYENA) to the assemblies which should be compiled with reference to Hyena. e.g.: F_SPOT_ASSEMBLIES = \ $(LINK_HYENA) \ ... After doing this, you'll should get a lot of build errors with respect to Log, because this is now defined twice. So next step is to remove Utils/Log.cs and then go through the compiler errors. If any questions remain, just ask.
(In reply to comment #3) > So, you still have to edit src/Metadata.am That should be src/Makefile.am :-)
Thanks Ruben and Mike. I applied the patch and made the changes. I'm getting the expected build errors. So now I'm working my way through them :)
Created attachment 162285 [details] [review] Replaces FSpot.Utils.Log by Hyena.Log I'm almost done. However, I have some questions: 1. I'm getting the error: ./PicasaWebExport.cs(435,72): error CS0433: The imported type `System.Web.HttpUtility' is defined multiple times I saw that it is indeed defined in Hyena, but I don't know how to resolve this ambiguity. Any hints? 2. There is no function void DebugException(string message, Exception e) in Hyena.Log. It's only used once or twice. Should I change it to a simple Exception(...) so that it's always logged, or should I add this function to the Log class? 3. in main.cs there is a Log.Tracing = true; which doesn't exist in Hyena.Log. Should I delete the complete --trace command line option? Anyway, attached is a diff with current status. I probably can finish it tomorrow. Also, Maxxer told me to replace the Console.WriteLines. I will do that as a second step.
Created attachment 162327 [details] [review] Replaces FSpot.Utils.Log by Hyena.Log Hi Ruben, here's the patch. I've used git-format-patch the first time and not sure if everything's as expected. So let me know, if I should change something. Cheers, Peter
Good stuff! Seems complete and clean. Like I told you on IRC, this is more important than it seems, so it's highly appreciated. A very contribution, more like this!
Created attachment 162346 [details] [review] Replaces Console.WriteLines by Logging calls Thanks, Ruben, I'm really glad to hear that! Here's another patch. Maxxer told me to get rid of all the WriteLines in the code and replace them by logging calls. I tried my best to guess the right translations into Debug, Warning, Error etc. But I don't know if the guesses were good. So I think this patch definitely needs a serious review, if it's useful at all. Again, I'm open to suggestions. PS: I did not replace WriteLines that were commented out.
Review of attachment 162346 [details] [review]: All in all a good patch (with some comments) and it's very cool that you took the effort to do this. Don't bother with the ones that are commented. We should reconsider which messages we actually care about and which not. This patch cleanly shows that we have quite a few weirdo messages in there :-). Am not going to be tough about this, but could you explain the System.Log? Typo? ;-) If you want to update your patch, add the needed changes and add them to your current patch using git commit --amend (see e.g. "git from the ground up" [1] if this is new), refile the patch and mark the old one as obsolete. [1] http://ftp.newartisans.com/pub/git.from.bottom.up.pdf ::: extensions/Exporters/FacebookExport/FindNullableClashes.cs @@ +16,3 @@ if (xattr.IsNullable && ftype.IsValueType && field.DeclaringType == type && !is_nullable) { + Log.InformationFormat ("Possible clash for {0}/{1}", type.FullName, field.Name); + Log.InformationFormat (" Type: {0}", field.FieldType.FullName); Would go for Debug here, as this is stuff people will probably not care about. That being said, this usually doesn't matter much if it's Information or Debug, we don't have a hard policy on it (yet). ::: src/Histogram.cs @@ +187,3 @@ Gtk.Application.Init (); Gdk.Pixbuf pixbuf = new Gdk.Pixbuf (args [0]); + System.Log.Debug ("loaded {0}", args [0]); System.Log? ::: src/Imaging/JpegHeader.cs @@ +755,3 @@ if (value != null) { string xml = System.Text.Encoding.UTF8.GetString (value, 29, value.Length - 29); + System.Log.Debug (xml); Same here. And a couple of other places (which a git grep will point out).
Created attachment 162410 [details] [review] Replaces Console.WriteLines by appropriate logging calls Oops, I missed quite a few things in the last patch. Sorry, I should have taken a bit more time. I completely worked it over. So I hope I didn't forget anything this time. Btw. thanks for that git-manual link. Haven't seen this before and it clearifies some stuff in git that I haven't clearly understood before.
Comment on attachment 162410 [details] [review] Replaces Console.WriteLines by appropriate logging calls Merged! This makes the output of F-Spot so much nicer and less chatty. Thank you!