summaryrefslogtreecommitdiffstats
path: root/kde/patch/gwenview/gwenview-17.04.1_dataloss.patch
blob: fa9342873a88679b223f127d99da3dea3424b99d (plain)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
From 6ce5d2f8d82f83c5a3d726ee5807ebaf7a6e3c6c Mon Sep 17 00:00:00 2001
From: Henrik Fehlauer <rkflx@lab12.net>
Date: Thu, 11 May 2017 22:40:15 +0200
Subject: Avoid data loss when importing pictures

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.

Reviewers: ltoscano, sandsmark, gateau

Reviewed By: ltoscano

Differential Revision: https://phabricator.kde.org/D5749
---
 importer/fileutils.cpp    | 5 ++++-
 importer/importdialog.cpp | 2 +-
 importer/importer.cpp     | 4 ++--
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/importer/fileutils.cpp b/importer/fileutils.cpp
index 5293d56..a51a18c 100644
--- a/importer/fileutils.cpp
+++ b/importer/fileutils.cpp
@@ -128,7 +128,10 @@ RenameResult rename(const QUrl& src, const QUrl& dst_, QWidget* authWindow)
         }
         result = RenamedUnderNewName;
 
-        dst.setPath(dst.path() + '/' + prefix + QString::number(count) + suffix);
+        dst.setPath(dst.adjusted(QUrl::RemoveFilename).path() + prefix + QString::number(count) + suffix);
+        statJob = KIO::stat(dst);
+        KJobWidgets::setWindow(statJob, authWindow);
+
         ++count;
     }
 
diff --git a/importer/importdialog.cpp b/importer/importdialog.cpp
index ee6f7cd..5e9e847 100644
--- a/importer/importdialog.cpp
+++ b/importer/importdialog.cpp
@@ -121,7 +121,7 @@ public:
         QList<QUrl> urls = importedUrls + skippedUrls;
         while (true) {
             KIO::Job* job = KIO::del(urls);
-            if (!job->exec()) {
+            if (job->exec()) {
                 break;
             }
             // Deleting failed
diff --git a/importer/importer.cpp b/importer/importer.cpp
index 51c4b96..a7e1d4e 100644
--- a/importer/importer.cpp
+++ b/importer/importer.cpp
@@ -98,7 +98,7 @@ struct ImporterPrivate
         }
         mCurrentUrl = mUrlList.takeFirst();
         QUrl dst = mTempImportDirUrl;
-        dst.setPath(dst.path() + '/' + mCurrentUrl.fileName());
+        dst.setPath(dst.path() + mCurrentUrl.fileName());
         KIO::Job* job = KIO::copy(mCurrentUrl, dst, KIO::HideProgressInfo);
         KJobWidgets::setWindow(job, mAuthWindow);
         QObject::connect(job, SIGNAL(result(KJob*)),
@@ -122,7 +122,7 @@ struct ImporterPrivate
         } else {
             fileName = src.fileName();
         }
-        dst.setPath(dst.path() + '/' + fileName);
+        dst.setPath(dst.path() + fileName);
 
         FileUtils::RenameResult result = FileUtils::rename(src, dst, mAuthWindow);
         switch (result) {
-- 
cgit v0.11.2