Cancel button in Tools > Preferences > Music Library doesn't work on folder edit

[Clementine-Issue-Importer]

From golden.cako on August 21, 2010 02:58:23

What steps will reproduce the problem? 1. Open Clementine, Tools > Preferences > Music Library

2. Add/Remove Folder
3. Click Cancel
4. Reopen Tools > Preferences > Music Library What is the expected output? What do you see instead? Since one has canceled the changes the folders shouldn't change, but they do. What version of the product are you using? On what operating system? 0.4 r1516 Ubuntu Linux 10.04 2.6.32-24-generic

Original issue: http://code.google.com/p/clementine-player/issues/detail?id=647

[Clementine-Issue-Importer]

From vasilakisfil on February 05, 2012 14:57:18

Hello! I managed to fix this issue! I have atttached the patch! Any suggestions in my code style etc are more than welcome since i intend to continue submitting patches :D

Attachment: fixFoldersCancel.patch

[Clementine-Issue-Importer]

From davidsansome on February 05, 2012 15:12:45

Thanks for the patch!

I see you're comparing the number of rows between the dialog opening and being closed. If I were doing this I'd probably work with the actual paths themselves, as strings - saving that list at the beginning and restoring it afterwards if the dialog is cancelled. Otherwise you might run into trouble if the user removes an existing path as well as adding a new one.

As for style, you might like to read the google c++ style guide which we try to follow in Clementine: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml In particular there are a couple of things you could change in your patch:

• non-const global variables are bad, use class members instead.
• use unix_hacker_style for variables instead of camelCase, and use a trailing underscore for member variables.
• put spaces around operators and function arguments ("foo = bar" instead of "foo=bar")
• put for loop bodies on a separate line and use curly brackets, even if they're really short
• group #includes from Qt together, separately from #includes from Clementine

[Clementine-Issue-Importer]

From vasilakisfil on February 05, 2012 15:28:02

Thanx for the feedback! I will study everything you said and i will come back with a better patch :D

[Clementine-Issue-Importer]

From vasilakisfil on February 06, 2012 05:41:04

Helloo again! I include a better implementation..actually i didn't exactly what you told me: if for any change the user made(removed one folder for example), i added all the directories from the beginning this would be very inefficient since some folders might be 100Gbytes big! So i made a little more geeky implementation in order to remove/add only these that truly need to, i.e the folders the user added or removed respectively. Again any suggestion is more than welcomed. For me the point is to make readable/efficient code not just working code :D As far as the code style the only thing that i (tried but eventually) didn't follow exactly is that one: Each line of text in your code should be at most 80 characters long. It really restricts me but i will give better try next time:P

The patch i made it with the command "git format-patch origin/master" which created 2 patched(including previous commit). I hope you know how to apply them(if you do) because personally i am not sure if you have to apply both of them or only the second.

Attachment: 0001-Fixed-issue-647.-Now-when-hitting-cancel-button-all-.patch 0002-Enchanced-the-implementation-of-issue-647.-Note-that.patch

[Clementine-Issue-Importer]

From vasilakisfil on February 06, 2012 05:42:45

Also i forgot to mention i fixed the duplicates entries when adding a new folder i.e. if this folder already exits clementine it just shows it to you with ui_->list->setCurrentIndex(index); command :D