GNOME Bugzilla – Bug 689123
Video imported with Music file organization options
Last modified: 2013-02-28 15:17:29 UTC
When I import videos with banshee, the video gets imported and organized in the folder hierarchy specified in the Music File Organization option. This is a bit annoying as it ends up with all my videos being placed in: Videos-> Unknown Artist-> Unknown Album -> ..... I do use the Music File Organization option and it works beautifully with Music. But, I don't want it to interfere with my video library. Shouldn't there be a similar option to specify how videos are organized in the library? It seems there is no way to set that up currently.
This would be just adding a few lines. Would also be a gnome-love bug. I would suggest the following folders: "%album_artist%%path_sep%%album%", "%genre%%path_sep%%album%", "%album%", No folder and the following FileNames "%track_artist% - %title%", "%track_artist% (%album%) - %title%", "%title%"
Samuel, I think this bug is not as easy as it seems. First, do the tags "Artist" and "Album" make sense at all for a video? Title does, but for a movie you may want "Director", "Producer", etc? And for TV shows a whole range of options are commented in bug 631772.
But this is a second step, Refactoring the Video extension as a whole. At the moment the Video Extension just overrides the Column Name of Track Author to "Produced by". In the current way, just offering some Naming Options would be consistent.
Sorry, you're right Samuel, I was not understanding the bug. Thing is, to fix this bug, we don't really need to implement what you mention comment#1 either. We just need to apply no folder organization at all for Video, as it is not implemented yet. Then, as a separate bug, we should implement folder organization for video. I'll put here the steps to reproduce that were missing from comment#0, which led me to misunderstand this bug: 1) Open banshee, go to menu Edit->Preferences->Source Specific, choose Video source in the combo box. 2) Set your video folder to be $HOME/Videos (if it's not set already like that). 3) Enable option "Copy files to media folder when importing" (notice there are no more options in this dialog, for Videos), close Preferences. 4) Go to menu Media -> Import Media..., choose a folder where you know you have videos, but which is not $HOME/Videos. For the sake of demonstrating the bug more easily, in my case I have some FLV videos there (note: we don't have FLV metadata parsing yet in Banshee AFAIK). Current results: The FLV videos get copied to $HOME/Videos/Unknown Artist/Unknown Album/. Expected results: The FLV videos get copied to $HOME/Videos/. Samuel, I don't see you lately on IRC, would be good to chat!
Hi Andres and Samuel, Thanks for the description. According the details you have provided I was able to create a patch for the bug.
Created attachment 237298 [details] [review] When importing videos they are copied to the Home/Videos folder.
Thanks for the Patch, but I think it would work without modifying the classes. If you check in Banshee.Library (Part of Banshee.Services), you'll find the file MusicFileNamePattern. Just create a similar class for VideoFileNamePattern and set the DefaultFolder to empty and the DefaultFile to something usable. This should work or?
Comment on attachment 237298 [details] [review] When importing videos they are copied to the Home/Videos folder. Yeah, can you adjust the patch addressing Samuel's concern Udesh? Also, you forgot a little detail with regards to our coding guidelines: use spaces abundantly when using operators. In this case, it should be "a = b", not "a=b". Thanks! Looking forward to the next version of the patch.
Created attachment 237367 [details] [review] Added a folderSchema for video file importing to Home/Videos
Thanks Samuel and Andres for the suggestions. I created a patch according to the suggestion by Samuel and I had to do some changes to other classes also. I'm not sure whether my patch is the exact way Samuel suggested. BTW it seems to be doing the work as suggested. but from my patch I later realized that some changes are happened to --> menu Edit->Preferences->Source Specific, choose Video source in the combo box. If you can review this and give your ideas it will be great. Thanks Andres for pointing out about the guidelines. This time I followed them. :)
Comment on attachment 237367 [details] [review] Added a folderSchema for video file importing to Home/Videos (In reply to comment #10) > Thanks Samuel and Andres for the suggestions. > > I created a patch according to the suggestion by Samuel and I had to do some > changes to other classes also. > > I'm not sure whether my patch is the exact way Samuel suggested. Good work Udesh. I think we still some iterations on this. First: Samuel, don't you think creating a VideoFileNamePattern class is useless if it's supposed to be not implemented yet (empty)? Also, the class that you have in your patch doesn't look empty to me, it has a lot of Music details such as album... Did you just copied it from MusicFileNamePattern? Maybe we just need a NullFileNamePattern that just makes the copy operation to be as is (no rename involved). Or maybe call it DefaultFileNamePattern? If we go ahead with this idea, maybe it's better to have all sources apply this pattern unless explicitly overriding it (like MusicLibrary would do, by using MusicFileNamePattern). Does that sound good? > BTW it seems to be doing the work as suggested. > but from my patch I later realized that some changes are happened to --> > menu Edit->Preferences->Source Specific, choose Video source in the combo box. > Exactly, the way you did it (just copying the MusicFileNamePattern and having a new LibrarySchema setting), the UI adapts itself to it. You have to make sure this doesn't happen. Maybe creating a DefaultFileNamePattern class that is completely empty achieves that. Just test it. > Thanks Andres for pointing out about the guidelines. This time I followed them. > :) Nope you didn't, I still see a lot of assignments like "a=b" instead of "a = b" and a lot of places where you forget a space before "(". Please read them again ;) -> http://git.gnome.org/browse/banshee/tree/HACKING
(In reply to comment #11) > (From update of attachment 237367 [details] [review]) > > Exactly, the way you did it (just copying the MusicFileNamePattern and having a > new LibrarySchema setting), the UI adapts itself to it. You have to make sure > this doesn't happen. Alright, I found out what the problem was and I committed a fix (the patch was much simpler than we thought: http://git.gnome.org/browse/banshee/commit/?id=2c82ffbeaf4f4ff9f996b4dfaf1bfc84064353f5 ). Udesh, I hope this doesn't deter you from contributing more patches, I think you're making good progress here. But take in account that reviews are not meant to be a process where the reviewer figures out why your patch doesn't work, but a process in which a patch that works is reviewed to check if it uses a good approach or a bad approach (and to check that coding guidelines are respected). Therefore, you should always submit a patch that meets the expectations of the bug (in regards to behaviour), and your patch didn't meet them in the sense that it created a pattern for video (in the Preferences UI) which was not acceptable (as this is a bug, not an enhancement). Also, please be always wary when you copy+paste code. In 99% of the cases when you do this, you are doing something wrong, because you're violating the DRY/OAOO principles (google around, useful stuff to know). Yes you can copy+paste as a proof of concept or to test something out, but most likely you will be able to abstract the common code somewhere in the end result. Maybe I should have waited a bit more, but given there were no more comments on the bug for 3 days, I thought you were kind of stuck and I wanted to move things forward, please don't be upset! Looking forward to your next pages wrt Audiobooks :) (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.)
Wow, that's elegant Andrés. Sorry, Udesh, seams my idea led into the wrong direction. Thanks for your effort.
(In reply to comment #12) Thanks and Good work Andres. :) > Udesh, I hope this doesn't deter you from contributing more patches, I think > you're making good progress here. But take in account that reviews are not > meant to be a process where the reviewer figures out why your patch doesn't > work, but a process in which a patch that works is reviewed to check if it uses > a good approach or a bad approach (and to check that coding guidelines are > respected). Therefore, you should always submit a patch that meets the > expectations of the bug (in regards to behaviour), and your patch didn't meet > them in the sense that it created a pattern for video (in the Preferences UI) > which was not acceptable (as this is a bug, not an enhancement). Anyway we should do some enhancement for the video pattern. As in music libraries. If so we can raise a new bug. > > Also, please be always wary when you copy+paste code. In 99% of the cases when > you do this, you are doing something wrong, because you're violating the > DRY/OAOO principles (google around, useful stuff to know). Yes you can > copy+paste as a proof of concept or to test something out, but most likely you > will be able to abstract the common code somewhere in the end result. Thanks. I used the existing code with understanding. I mentioned it in the comment even. anyway I agree , it doesn't meet with the expectations of the bug. > > Maybe I should have waited a bit more, but given there were no more comments on Oho I'm sorry. I was little bit busy. > the bug for 3 days, I thought you were kind of stuck and I wanted to move > things forward, please don't be upset! Looking forward to your next pages wrt > Audiobooks :) Yes I will work on them. Hope you would help with reviewing. > > (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.)