Avoid data loss when importing pictures
ClosedPublic

Authored by rkflx on May 7 2017, 8:26 PM.

Details

Summary

Fix porting regressions, which left users of Gwenview Importer with:

  • failed import (import destination still empty)
  • additionally (when choosing "Delete" instead of "Keep" after import): pictures also removed from import source, with no way to recover

Correct additional problems remaining after fixing the import failure:

  • hang on duplicate filenames
  • identically named files with different content are never imported
  • error dialog when deleting pictures from import source

BUG: 379615

In detail:

1st problem (introduced in 017b4fe5dc7f4b6e876cfd7b108dcebbf609ae94):

Initially, pictures are copied to a temporary folder
(e.g. "foo/.gwenview_importer-IcQqvo/"), before being moved/renamed
to the final destination (e.g. "foo/"), which is determined by
calling "cd .." on the temporary folder.

However, mistakenly this path contains a superfluous '/'
(e.g. "foo/.gwenview_importer-IcQqvo//"), resulting in the final
destination reading "foo/.gwenview_importer-IcQqvo/" instead of
"foo/". On cleanup, the temporary folder is removed, simultaneously
deleting all new pictures.

Fix: Omit '/' where appropriate.

2nd problem (broken by a3262e9b197ee97323ef8bf3a9dca1e13f61a74c):

When trying to find a unique filename, the while loop "stat"ing the
file repeats forever.

Fix: Call "KIO::stat" and "KJobWidgets::setWindow"
     also inside the loop.

3rd problem (introduced in 017b4fe5dc7f4b6e876cfd7b108dcebbf609ae94):

If the destination directory already contains an identically named
file, we try to find a different name by appending a number. For
this, we need to strip the old filename from the full path.
Unfortunately, only calling "path()" is incorrect, giving
"foo/pict0001.jpg/pict0001_1.jpg", rather than "foo/pict0001_1.jpg".

Fix: Use "adjusted(QUrl::RemoveFilename)".

4th problem (broken by a3262e9b197ee97323ef8bf3a9dca1e13f61a74c):

Although deletion works, we show a warning dialog stating failure.

Fix: Invert the logic of "job->exec()", as the error handling should
     be skipped by "break"ing out of the while loop.
Test Plan

Steps to reproduce (see https://bugs.kde.org/show_bug.cgi?id=379615) no longer result in data loss.

Autotests for importer (separate review request in D5750) pass. Hopefully, this helps catching any future porting regressions.

Diff Detail

Repository
R260 Gwenview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rkflx created this revision.May 7 2017, 8:26 PM
rkflx edited the test plan for this revision. (Show Details)
rkflx edited the test plan for this revision. (Show Details)May 7 2017, 8:37 PM
ltoscano accepted this revision.May 10 2017, 9:37 PM

I confirm the fixes, and this should go to Applications/17.04. Do you have commit access? If not, can you please send me your data for the commit message? (the patch was not produced with arcanist, so I can't get them). I see your name and email from bugzilla, but I guess the surname should be better too.

This revision is now accepted and ready to land.May 10 2017, 9:37 PM
rkflx added a comment.May 11 2017, 7:37 PM

@ltoscano: Thanks for caring for Gwenview and having taken the time to review this.

Do you have commit access? If not, can you please send me your data for the commit message?

Nope, first patch to the KDE project. Please commit on my behalf (Henrik Fehlauer <rkflx@lab12.net>).

BTW, I used "git format-patch" to retain authorship information, but i guess T5242 is still waiting for resolution by upstream Phabricator.

rkflx added a comment.May 11 2017, 7:51 PM

@ltoscano: Just seen the thread on the release-team-ML. Next time, feel free to CC me if you're hoping for an immediate reply. No worries though, I fully understand reviewing took some time since sunday. I would have liked to submit earlier before the freeze on monday, it just wasn't possible this time.

Once you've commited the patch, should we link the commit on the release-team- and/or kde-distro-packagers mailinglist so distributions could pick it up?

@gateau: I tried commenting on your blog [1], but unfortunately comments are now closed over there. Maybe you could add a note to the blog entry?

Anyway, as the bug is only present in 17.04.0 and 17.04.1, I reckon the number of affected users should be fairly small (although the issue might be quite unpleasant for anyone clicking "Delete").

Thanks again!

[1] http://agateau.com/2016/gwenview-importer-is-back/

This revision was automatically updated to reflect the committed changes.
In D5749#108866, @rkflx wrote:

@ltoscano: Thanks for caring for Gwenview and having taken the time to review this.

Do you have commit access? If not, can you please send me your data for the commit message?

Nope, first patch to the KDE project. Please commit on my behalf (Henrik Fehlauer <rkflx@lab12.net>).

Done. First of a long series, right? :)

BTW, I used "git format-patch" to retain authorship information, but i guess T5242 is still waiting for resolution by upstream Phabricator.

Probably not, or it would have been announced (and the task closed).

In D5749#108875, @rkflx wrote:

@ltoscano: Just seen the thread on the release-team-ML. Next time, feel free to CC me if you're hoping for an immediate reply. No worries though, I fully understand reviewing took some time since sunday. I would have liked to submit earlier before the freeze on monday, it just wasn't possible this time.

Of course, no problem, you did a lot already.

Once you've commited the patch, should we link the commit on the release-team- and/or kde-distro-packagers mailinglist so distributions could pick it up?

I'm going to send an email to kde-distro-packagers.

rkflx added a comment.May 11 2017, 9:46 PM

@ltoscano: Thanks again for your help with this patch series.

Done. First of a long series, right? :)

So many ideas, but too little time… Let's see how this pans out :)

gateau edited edge metadata.May 14 2017, 8:32 PM

Thanks a lot for fixing the mess I created. I just added a note about this to the article on my blog.

rkflx added a comment.May 14 2017, 8:43 PM

Thanks a lot for fixing the mess I created. I just added a note about

> this to the article on my blog.

Thanks, Aurélien!

Without your porting efforts, there would be no KF5 based importer today
(You just missed to port your own unit tests :P). Also, for me this was
a really good motivation to get involved with KDE.