ubuntu-touch-coreapps-reviewers team mailing list archive

[Stefano Verzegnassi <stefano92.100@xxxxxxxxx>]

Stefano Verzegnassi has proposed merging lp:~verzegnassi-stefano/ubuntu-docviewer-app/ch-imported-documents-name into lp:ubuntu-docviewer-app.

Commit message:
- Rename a file as 'filename (copy x).ext' (instead of 'filename.x.ext') if a file with the same name already exists.
- Fixed bad renaming of a file that already has "(copy x)" suffix in its name.

Requested reviews:
  Ubuntu Document Viewer Developers (ubuntu-docviewer-dev)
Related bugs:
  Bug #1432403 in Ubuntu Document Viewer App: "[content-hub] If a file with the same name of the imported document already exists, the new file is wrongly renamed"
  https://bugs.launchpad.net/ubuntu-docviewer-app/+bug/1432403
  Bug #1432408 in Ubuntu Document Viewer App: "[content-hub] Refactor format used for renaming files with the same name"
  https://bugs.launchpad.net/ubuntu-docviewer-app/+bug/1432408

For more details, see:
https://code.launchpad.net/~verzegnassi-stefano/ubuntu-docviewer-app/ch-imported-documents-name/+merge/256841

- Rename a file as 'filename (copy x).ext' (instead of 'filename.x.ext') if a file with the same name already exists.
The string is translatable, so that it follows user's locale (e.g. "filename (copia x).ext' in Italian).

- Fixed bad renaming of a file that already has "(copy x)" suffix in its name.
There was an issue in the previous revision, that caused a bad renaming of the file:
i.e. "filename (copy x) (copy 1).ext" instead of "filename (copy x+1).ext"

*** TESTS:
1. Using File Manager, browse the Documents folder
2. Open a PDF file with Document Viewer (e.g. "Manual_Aquaris_E4.5_ubuntu_EN.pdf")
EXPECTED RESULT:
A file called "Manual_Aquaris_E4.5_ubuntu_EN (copy 1).pdf" is imported

3. From File Manager, open "Manual_Aquaris_E4.5_ubuntu_EN (copy 1).pdf"
EXPECTED RESULT:
A file called "Manual_Aquaris_E4.5_ubuntu_EN (copy 2).pdf" is imported

4. From File Manager, open "Manual_Aquaris_E4.5_ubuntu_EN.pdf" again
EXPECTED RESULT:
A file called "Manual_Aquaris_E4.5_ubuntu_EN (copy 3).pdf" is imported

5. If you're feeling brave, repeat this a number of times. (so you'll get "copy 10").

*** NOTE:
On Nexus 5, I've always encountered some issue with Content Hub, that makes Unity Shell much unstable, freezing the whole phone.
This usually happens to me after a number of transfers.

For that reason I'm not able to say if this MP makes the app more unstable, so please test this code on a officially supported device.
Run multiple tests, and import a document about 5-8 times each test.

-- 
Your team Ubuntu Document Viewer Developers is requested to review the proposed merge of lp:~verzegnassi-stefano/ubuntu-docviewer-app/ch-imported-documents-name into lp:ubuntu-docviewer-app.

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2015-02-13 15:30:01 +0000
+++ CMakeLists.txt	2015-04-20 16:46:44 +0000
@@ -90,7 +90,7 @@
 
 file(GLOB_RECURSE I18N_SRC_FILES
      RELATIVE ${CMAKE_CURRENT_SOURCE_DIR}/po
-     *.qml *.js)
+     *.qml *.js *.cpp)
 list(APPEND I18N_SRC_FILES ${CMAKE_CURRENT_BINARY_DIR}/po/${DESKTOP_FILE}.in.in.h)
 list(SORT I18N_SRC_FILES)
 message("Found ${I18N_SRC_FILES}")

=== modified file 'debian/rules' (properties changed: +x to -x)
=== modified file 'po/CMakeLists.txt'
--- po/CMakeLists.txt	2015-01-24 00:03:13 +0000
+++ po/CMakeLists.txt	2015-04-20 16:46:44 +0000
@@ -27,7 +27,7 @@
   COMMAND ${GETTEXT_XGETTEXT_EXECUTABLE} -o ${POT_FILE}
           --from-code=UTF-8
           --c++ --qt --add-comments=TRANSLATORS
-          --keyword=tr --keyword=tr:1,2 --keyword=N_
+          --keyword=tr --keyword=tr:1,2 --keyword=N_ --keyword=_
           --package-name='${PROJECT}'
           --copyright-holder='Canonical Ltd.'
           --directory ${CMAKE_CURRENT_SOURCE_DIR} 

=== modified file 'po/com.ubuntu.docviewer.pot'
--- po/com.ubuntu.docviewer.pot	2015-04-14 15:37:06 +0000
+++ po/com.ubuntu.docviewer.pot	2015-04-20 16:46:44 +0000
@@ -8,7 +8,7 @@
 msgstr ""
 "Project-Id-Version: \n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2015-04-14 17:34+0200\n"
+"POT-Creation-Date: 2015-04-20 18:19+0200\n"
 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <LL@xxxxxx>\n"
@@ -18,6 +18,26 @@
 "Content-Transfer-Encoding: 8bit\n"
 "Plural-Forms: nplurals=INTEGER; plural=EXPRESSION;\n"
 
+#. TRANSLATORS: This string is used for renaming a copied file,
+#. when a file with the same name already exists in user's
+#. Documents folder.
+#.
+#. e.g. "Manual_Aquaris_E4.5_ubuntu_EN.pdf" will become
+#. "Manual_Aquaris_E4.5_ubuntu_EN (copy 2).pdf"
+#.
+#. where "2" is given by the argument "%1"
+#.
+#: ../src/app/content-communicator.cpp:101
+#, qt-format
+msgid "copy %1"
+msgstr ""
+
+#: ../src/app/docviewer-application.cpp:164
+#: ../src/app/qml/documentPage/DocumentPage.qml:25
+#: /home/stefano/Progetti/doc-viewer/branches/build-ch-imported-file-format-UbuntuSDK_for_armhf_GCC_ubuntu_sdk_14_10_utopic-Default/po/com.ubuntu.docviewer.desktop.in.in.h:1
+msgid "Document Viewer"
+msgstr ""
+
 #: ../src/app/qml/common/DetailsPage.qml:27
 #: ../src/app/qml/pdfView/PdfViewDefaultHeader.qml:97
 #: ../src/app/qml/textView/TextViewDefaultHeader.qml:83
@@ -214,11 +234,6 @@
 msgid "Even earlier..."
 msgstr ""
 
-#: ../src/app/qml/documentPage/DocumentPage.qml:25
-#: /home/stefano/Progetti/doc-viewer/build-landscape-support-Desktop-Default/po/com.ubuntu.docviewer.desktop.in.in.h:1
-msgid "Document Viewer"
-msgstr ""
-
 #: ../src/app/qml/documentPage/DocumentPageDefaultHeader.qml:28
 #: ../src/app/qml/documentPage/DocumentPagePickModeHeader.qml:35
 msgid "Switch to single column list"
@@ -304,6 +319,6 @@
 msgid "Open"
 msgstr ""
 
-#: /home/stefano/Progetti/doc-viewer/build-landscape-support-Desktop-Default/po/com.ubuntu.docviewer.desktop.in.in.h:2
+#: /home/stefano/Progetti/doc-viewer/branches/build-ch-imported-file-format-UbuntuSDK_for_armhf_GCC_ubuntu_sdk_14_10_utopic-Default/po/com.ubuntu.docviewer.desktop.in.in.h:2
 msgid "documents;viewer;pdf;reader;"
 msgstr ""

=== modified file 'run-pep8' (properties changed: +x to -x)
=== modified file 'src/app/CMakeLists.txt'
--- src/app/CMakeLists.txt	2015-02-13 15:30:01 +0000
+++ src/app/CMakeLists.txt	2015-04-20 16:46:44 +0000
@@ -3,6 +3,11 @@
 
 pkg_check_modules(CONTENTHUB REQUIRED libcontent-hub)
 
+add_definitions(
+  -DGETTEXT_PACKAGE=\"${PROJECT_NAME}\"
+  -DGETTEXT_LOCALEDIR=\"${CMAKE_INSTALL_LOCALEDIR}\"
+)
+
 set(docviewer_SRCS
     main.cpp
     content-communicator.cpp

=== modified file 'src/app/content-communicator.cpp'
--- src/app/content-communicator.cpp	2015-04-07 22:52:48 +0000
+++ src/app/content-communicator.cpp	2015-04-20 16:46:44 +0000
@@ -57,47 +57,68 @@
 {
     // FIXME: If a file is imported from $HOME/Documents, a new copy of the file is created.
     //   Could be use md5? http://doc.qt.io/qt-5/qml-qtqml-qt.html#md5-method
-    // FIXME: If there already a file called "filename.1.ext", the imported file won't be renamed as "filename.2.ext", but "filename.1.1.ext".
-    //  (This issue is in gallery-app too.)
-
     QVariantList importedDocuments;
     QVector<Item> transferedItems = transfer->collect();
     foreach (const Item &hubItem, transferedItems) {
         QFileInfo fi(hubItem.url().toLocalFile());
-        QString filename = fi.fileName();
+
         QString dir;
+        QString destination;
+        bool rejected = false;
+
         QMimeDatabase mdb;
         QMimeType mt = mdb.mimeTypeForFile(hubItem.url().toLocalFile());
-        QString destination;
-        bool rejected = false;
 
         // Check if the item is supported by Ubuntu Document Viewer
         if (isSupportedMimetype(mt.name())) {
             QString suffix = fi.completeSuffix();
-            // FIXME: Should we use fi.baseName()?
-            QString filenameWithoutSuffix = filename.left(filename.size() - suffix.size());
+
+            QString filenameWithoutSuffix = fi.baseName();
             if(suffix.isEmpty()) {
                 // If the filename doesn't have an extension add one from the
                 // detected mimetype
                 if(!mt.preferredSuffix().isEmpty()) {
                     suffix = mt.preferredSuffix();
-                    filenameWithoutSuffix += ".";
                 }
             }
+
             dir = QStandardPaths::writableLocation(QStandardPaths::DocumentsLocation) + QDir::separator();
+            destination = QString("%1.%2").arg(dir + filenameWithoutSuffix, suffix);
 
-            destination = QString("%1%2").arg(dir + filenameWithoutSuffix, suffix);
-            // If we already have a file of this name reformat to "filename.x.png"
+            // If we already have a file of this name reformat to "filename (copy x).png"
             // (where x is a number, incremented until we find an available filename)
             if(QFile::exists(destination)) {
+                /*
+                 TRANSLATORS: This string is used for renaming a copied file,
+                 when a file with the same name already exists in user's
+                 Documents folder.
+
+                 e.g. "Manual_Aquaris_E4.5_ubuntu_EN.pdf" will become
+                      "Manual_Aquaris_E4.5_ubuntu_EN (copy 2).pdf"
+
+                      where "2" is given by the argument "%1"
+                */
+                QString reformattedSuffix = QString(_("copy %1"));
+
+                QRegExp rx(" \\(" + reformattedSuffix.arg(QString("\\d+")) + "\\)");
+                int reformattedSuffixPos = filenameWithoutSuffix.lastIndexOf(rx);
+
+                // Check if the file has already a "copy" suffix
+                if(reformattedSuffixPos != -1) {
+                    // Remove the "copy" suffix. We will re-put it later.
+                    filenameWithoutSuffix.truncate(reformattedSuffixPos);
+                }
+
                 int append = 1;
                 do {
-                    destination = QString("%1%2.%3").arg(dir + filenameWithoutSuffix, QString::number(append), suffix);
+                    destination = QString("%1 (%2).%3").arg(dir + filenameWithoutSuffix,
+                                                            reformattedSuffix.arg(QString::number(append)),
+                                                            suffix);
                     append++;
                 } while(QFile::exists(destination));
             }
 
-            QFile::copy(hubItem.url().toLocalFile(), destination);           
+            QFile::copy(hubItem.url().toLocalFile(), destination);
         } else {
             rejected = true;
         }
@@ -106,7 +127,7 @@
         // emitted with the 'documentImported' signal.
         QVariantMap entry;
         if (rejected) {
-            entry["fileName"] = filename;
+            entry["fileName"] = fi.fileName();
         } else {
             entry["fileName"] = destination;
         }

=== modified file 'src/app/content-communicator.h'
--- src/app/content-communicator.h	2015-04-07 21:40:16 +0000
+++ src/app/content-communicator.h	2015-04-20 16:46:44 +0000
@@ -23,6 +23,9 @@
 
 #include <QUrl>
 #include <QVector>
+#include <libintl.h>
+
+#define _(value) dgettext(GETTEXT_PACKAGE, value)
 
 using namespace com::ubuntu;