Comment 23 for bug 69479

Revision history for this message
Colin Watson (cjwatson) wrote :

The X-Katapult-ID change and the KURL setPath/addPath/cleanPath changes look fine for edgy-proposed.

Some qDebug statements are removed. Is this a good idea? It's not a minimal change, at any rate; if it's not necessary to remove them then please leave them be.

The sqlResult[2]=="-1" test was inverted, even though the body of each branch of that if seems to be logically the same. Was this intentional?

What is the purpose of this change?

- QString sqlQuery("SELECT a.name, t.title, t.deviceid, d.lastmountpoint, t.url, i.path, album.name FROM tags t, artist a, album LEFT JOIN statistics s ON t.url = s.url AND t.deviceid = s.deviceid LEFT JOIN images i ON (a.name = i.artist AND album.name = i.album) LEFT JOIN devices d ON t.deviceid = d.id WHERE t.album = album.id AND t.artist = a.id"); // AND
+ QString sqlQuery("SELECT a.name, t.title, t.deviceid, d.lastmountpoint, t.url, i.path, album.name FROM tags t LEFT JOIN statistics s ON t.url = s.url AND t.deviceid = s.deviceid LEFT JOIN artist a ON t.artist = a.id LEFT JOIN album ON t.album = album.id LEFT JOIN images i ON ( a.name = i.artist AND album.name = i.album) LEFT JOIN devices d ON t.deviceid = d.id WHERE 1");

It appears to be moving stuff from a WHERE into a LEFT JOIN. Is this necessary? Please explain. My SQL knowledge isn't adequate to approve this without an explanation.

The changelog is still not satisfactory for edgy-proposed. As Matt said, "Update of kubuntu_06_amarok_14.diff to working version" is not good enough. Please explain *what changes were made to that file*! For example, assuming that only the KURL changes are necessary, something like "Canonicalise dynamic collection URLs before passing them to amarok" would be more helpful (although please don't just copy my text as I'm sure you know the content of the change better than I do, and so should be able to describe it better). The reason why you need to do this is that this is the kind of level on which we need to be thinking when approving changes, and it's the sort of explanation that we ought to give to reasonably technically-minded users when explaining to them why we chose to issue a stable release update (particularly in the event that the update breaks).