Don't delete whole folder when deleting a track
ClosedPublic

Authored by wbauer on Sep 18 2019, 6:53 AM.

Details

Summary

SqlCollectionLocation::moodFile() tries to replace the filename's extension with ".mood" (and prepend a '.') to get the Url of the moodfile.
But it does that *after* the filename has already been removed from the Url, so QUrl::fileName() gives an empty string and the end result is actually "/path/to/folder/." which depicts the containing folder.

As a result, when Amarok tries to delete the corresponding moodfile after a track is deleted, the whole folder gets deleted instead.

To fix this, save the filename in a temporary variable before removing it, and use that for generating the new filename.

BUG: 411760

Test Plan

Delete a track from the local collection (right-click->"Delete tracks" or "Move to trash").
Only the selected track gets deleted now, previously it deleted all other files in the containing folder as well.

Debug output I added locally confirms that the generated path is now as expected, i.e. "/path/to/folder/.xxx.mood" instead of "/path/to/folder/.".

Diff Detail

Repository
R181 Amarok
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
wbauer created this revision.Sep 18 2019, 6:53 AM
Restricted Application removed a project: Amarok. · View Herald TranscriptSep 18 2019, 6:53 AM
Restricted Application added a subscriber: amarok-devel. · View Herald Transcript
wbauer requested review of this revision.Sep 18 2019, 6:53 AM
wbauer edited the test plan for this revision. (Show Details)Sep 18 2019, 7:04 AM
wbauer added a subscriber: Amarok.Sep 18 2019, 8:54 AM
schweingruber accepted this revision.Sep 19 2019, 7:32 AM
schweingruber added a subscriber: schweingruber.

Looks OK. What is the status of the unit tests, though?

This revision is now accepted and ready to land.Sep 19 2019, 7:32 AM

Looks OK. What is the status of the unit tests, though?

No idea.
But this isn't tested at all AFAICS.

Btw, this function is only used inside this source file, when deleting or moving tracks in the SqlCollection.
Otherwise, all mood file handling seems to be in src/moodbar/...

heikobecker accepted this revision.Sep 19 2019, 4:51 PM
heikobecker added a subscriber: heikobecker.

All those regressions from f1ab8128fdd6b2360bfea8b0bef9b5c46b8d5e68 :(

Looks OK. What is the status of the unit tests, though?

But this isn't tested at all AFAICS.

No, it isn't. It probably should, but judging from a cursory look it's not that quick and easy to test because it's not "near" the public interface. Anyway, I think it's more important to get this fix in, than delaying it for adding a new test.

This revision was automatically updated to reflect the committed changes.