GNOME Bugzilla – Bug 548636
Unify last.fm cache dirs
Last modified: 2012-02-08 19:35:38 UTC
Currently there are two folders for last.fm cache files inside .cache: $HOME/.cache/banshee-1/extensions/last.fm $HOME/.cache/banshee-1/extensions/lastfm It would be nicer if there was only a single folder IMHO
Confirmed. I too have both these folders with banshee 1.4.2 on a fresh install.
Bulk changing the assignee to banshee-maint@gnome.bugs to make it easier for people to get updated on all banshee bugs by following that address. It's usually quite apparent who is working on a given bug by the comments and/or patches attached.
Ping. $HOME/.cache/banshee-1/extensions/lastfm only contains an (most of the time almost empty) queue xml file, not really worth having its own folder.
Created attachment 179294 [details] [review] Patch
Review of attachment 179294 [details] [review]: Thanks for your patch ! If the audioscrobbler-queue.xml file exists, it would be nice to move it over to the new folder, so that we don't lose any queued scrobbles. Deleting the old folder if it's empty after that would also be nice. Another thing : please configur git with a proper e-mail address for yourself, so that you are properly creditted. See : http://live.gnome.org/Git/Developers#Setting_up_Git
I'm using the latest daily ppa Banshee (git master branch) and I still see both of the folders. Has the patch been applied? What's going on with this bug?
Confirmed. it seems that patch hasn't been applied. what if audioscrobbler-queue.xml file also put into the lastfm folder without creating a last.fm folder
Created attachment 206781 [details] [review] A patch for Bug 548636 - Unify last.fm cache dirs Review this patch and let me know your comments.
Created attachment 206783 [details] [review] A change in patch for Bug 548636
Comment on attachment 206783 [details] [review] A change in patch for Bug 548636 Hello Udesh, thanks for your patches! It seems that your 2nd patch is not really a correction of the first, but a patch that sits on top of the first (so it couldn't be committed without committing the first patch). So can you revert your patches completely from your local copy and try to make a new patch that is just one commit please? While you're at it, please also read the HACKING file which states the coding guidelines we use in banshee (for example, 4 spaces of identation, spaces before and after the '=' symbol, a space before opening parenthesis, etc) here: http://git.gnome.org/browse/banshee/tree/HACKING Thanks
Created attachment 206804 [details] [review] Changed Patch Thanks Andrés for the guidelines. This is my first patch to GNOME and I'm new to git also. Now I'm getting the steps how to work with git. I tested the code with few test cases. Please review the patch and let me know. Thanks
Review of attachment 206804 [details] [review]: Looks good so far as I can tell. One improvement would be making the commit message more descriptive e.g. "Use one standard directory for last.fm caching (bgo#548636)" that will ensure that the git log lists improvement and bugs affected which is helpful for the release notes. Welcome onboard Udesh
Thanks David.. Hope we are done with this bug.
Review of attachment 206804 [details] [review]: (In reply to comment #11) > Created an attachment (id=206804) [details] [review] > Changed Patch > > Thanks Andrés for the guidelines. No problem, this looks much better. > This is my first patch to GNOME and I'm new to git also. Welcome! > Now I'm getting the steps how to work with git. > I tested the code with few test cases. Please review the patch and let me know. Cool, but I think it needs a 2nd review (because even if the patch works, it should have good readability and good practices). I'm using Splinter to do the review now, let's see how it goes. (Take also in account David's recommendation about the commit message.) ::: src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/Queue.cs @@ +143,3 @@ xml_path = Path.Combine (xmlfilepath, "audioscrobbler-queue.xml"); + string temp_xmlfilepath = Path.Combine (Hyena.Paths.ExtensionCacheRoot, "lastfm"); + string temp_xml_path = Path.Combine (temp_xmlfilepath, "audioscrobbler-queue.xml"); If "last.fm" is going to be the old directory, I would rather have that one in a variable that has the "temp" name instead of the other way around. Actually, instead of "temp" I would use the "old" prefix. @@ +149,3 @@ + xml_path = temp_xml_path; + } + else { Please put both braces and the else in the same line ( "} else {" ). @@ +150,3 @@ + } + else { + if(File.Exists(xml_path)) { The only coding guideline that you forgot this time is the spaces. As an example, this line should read: "if (File.Exists (xml_path)) {" @@ +151,3 @@ + else { + if(File.Exists(xml_path)) { + System.IO.File.Copy(xml_path,temp_xml_path); In Banshee we have higher level APIs for file-access operations, which then map to the IO backend chosen (one of those is System.IO). Can you use them instead? The classes to use are Banshee.IO.File and Banshee.IO.Directory. @@ +158,3 @@ + else { + System.IO.Directory.Delete(xmlfilepath); + System.IO.File.Delete(xml_path); Take in account that both the last 2 lines are inside the "if" and inside the "else", so you could actually place them outside the "if-else" block (but using the TRUE flag for the 2nd argument of the Delete() method to delete contents recursively).
Created attachment 206871 [details] [review] Use one standard directory for last.fm caching (bgo#548636) thanks.! changed the patch according to the guidelines and reviews.
Review of attachment 206871 [details] [review]: Thanks for the last patch Udesh! It's almost perfect, but still needs some last touches... First thing: it doesn't apply to my git tree, are you using git master to develop? If not, please use the latest master branch. Now onto the details: ::: src/Extensions/Banshee.Lastfm/Banshee.Lastfm.Audioscrobbler/Queue.cs @@ +138,3 @@ public event EventHandler TrackAdded; + public Queue () This change seems accidental because you're breaking identation here, right? @@ +145,3 @@ + string xml_file_path = Path.Combine (xml_dir_path, "audioscrobbler-queue.xml"); + Hyena.SafeUri old_file_safeuri = new Hyena.SafeUri (old_xml_file_path); + Hyena.SafeUri file_safeuri = new Hyena.SafeUri (xml_file_path); You can use a "using" statement at the top of the file ("using Hyena;") to allow you typing just "SafeUri" instead of "Hyena.SafeUri" all the time. Also, you can use the keyword "var" if you initialize the variable with a constructor in the same line, this way: var file = new SafeUri (xml_file_path); (And no need to specify the prefix "_safeuri" because there is no other "file" variable being used, just use "file"); @@ +150,3 @@ + if (Banshee.IO.Directory.Exists (old_xml_dir_path)) { + if (Banshee.IO.File.Exists (old_file_safeuri)) { + Banshee.IO.File.Copy (old_file_safeuri, file_safeuri, true); Have you noticed that in this line you forgot to add 4 extra spaces to respect identation? @@ +156,3 @@ } + xml_path = xml_file_path; Why not use xml_path simply instead of creating the new variable "xml_file_path"? I see that in the current code there's a "xmlfilepath" variable already, but it's actually a bad name because it refers to the dir, not the file, so you could rename it to xml_dir_path (the name you were already using for a variable in your patch).
Created attachment 207024 [details] [review] Use one standard directory for last.fm caching (bgo#548636) Thanks Andres..!!! I hope this should get apply..if not please let me know the error you are getting.
Review of attachment 207024 [details] [review]: With the exception of a missing space of one =, this looks to address every one of Andres' comments. Excellent work, thank you for your patience
Comment on attachment 179294 [details] [review] Patch This previous patch has been obsoleted by the new patch.
Comment on attachment 207024 [details] [review] Use one standard directory for last.fm caching (bgo#548636) Thanks for the patch and the revisions. Committed, after moving the migration into its own method : http://git.gnome.org/browse/banshee/commit/?id=76cbc5429
This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.
Thanks David,Andres and Bertrand