Improve screenshot path handling (#1815)

* Make --path work correctly with relative paths

Relative paths are taken relative to the working directory of the calling
command, not relative to the daemon's working directory.

* Allow file paths in --path and refactor

* Remove some redundancy

These actions are already performed in the respective functions in
FlameshotDBusAdapter.

* Tweak --path error checker a bit more

* Rework FileNameHandler and update references

The class now has a much simpler interface.

- Screenshot paths are now universally determined by the function
  properScreenshotPath
- Some unreferenced methods have been removed
- The documentation of properScreenshotPath documents the changes well.

* Add crude tests for --path

Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>

* Fix failing build on Windows

Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>

* Add a test for invalid path

Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>

* Make tests clearer

Thanks to @mmahmoudian for his review and contribution.

Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>

* Fix bug in properScreenshotPath

Auto-numeration did not work when the screenshot was automatically
saved when copied to clipboard.

Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>

* Fall back to default pictures location

Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>

* Revert "Remove some redundancy"

This was not redundancy. I had actually introduced a bug with this.

This reverts commit 011ef737564892e494518443e6b80ccf3d286ae1.

* Change default path only on interactive save

Previously, the default save path was changed every time a screenshot
was saved. Now, that only happens when it gets saved from the GUI.

Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>

* Change --path help text

Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>

* Allow other image formats

Signed-off-by: Haris Gušić <harisgusic.dev@gmail.com>

src/config/filenameeditor.cpp

@@ -92,7 +92,7 @@ void FileNameEditor::initWidgets()
 void FileNameEditor::savePattern()
 {
     QString pattern = m_nameEditor->text();
-    m_nameHandler->setPattern(pattern);
+    ConfigHandler().setFilenamePattern(pattern);
 }
 
 void FileNameEditor::showParsedPattern(const QString& p)

src/core/capturerequest.cpp

@@ -73,7 +73,7 @@ void CaptureRequest::exportCapture(const QPixmap& p)
         if (m_path.isEmpty()) {
             ScreenshotSaver(m_id).saveToFilesystemGUI(p);
         } else {
-            ScreenshotSaver(m_id).saveToFilesystem(p, m_path, "");
+            ScreenshotSaver(m_id).saveToFilesystem(p, m_path);
         }
     }
 

src/core/flameshotdbusadapter.cpp

@@ -50,9 +50,7 @@ void FlameshotDBusAdapter::fullScreen(QString path,
     if (toClipboard) {
         req.addTask(CaptureRequest::CLIPBOARD_SAVE_TASK);
     }
-    if (!path.isEmpty()) {
     req.addTask(CaptureRequest::FILESYSTEM_SAVE_TASK);
-    }
     req.setStaticID(id);
     Controller::getInstance()->requestCapture(req);
 }
@@ -72,9 +70,7 @@ void FlameshotDBusAdapter::captureScreen(int number,
     if (toClipboard) {
         req.addTask(CaptureRequest::CLIPBOARD_SAVE_TASK);
     }
-    if (!path.isEmpty()) {
     req.addTask(CaptureRequest::FILESYSTEM_SAVE_TASK);
-    }
     req.setStaticID(id);
     Controller::getInstance()->requestCapture(req);
 }

src/main.cpp

@@ -148,7 +148,7 @@ int main(int argc, char* argv[])
     // Options
     CommandOption pathOption(
       { "p", "path" },
-      QObject::tr("Path where the capture will be saved"),
+      QObject::tr("Existing directory or new file to save to"),
       QStringLiteral("path"));
     CommandOption clipboardOption(
       { "c", "clipboard" }, QObject::tr("Save the capture to the clipboard"));
@@ -213,14 +213,17 @@ int main(int argc, char* argv[])
     };
 
     const QString pathErr =
-      QObject::tr("Invalid path, it must be a real path in the system");
+      QObject::tr("Invalid path, must be an existing directory or a new file "
+                  "in an existing directory");
     auto pathChecker = [pathErr](const QString& pathValue) -> bool {
-        bool res = QDir(pathValue).exists();
-        if (!res) {
+        QFileInfo fileInfo(pathValue);
+        if (fileInfo.isDir() || fileInfo.dir().exists()) {
+            return true;
+        } else {
             SystemNotification().sendMessage(
               QObject::tr(pathErr.toLatin1().data()));
+            return false;
         }
-        return res;
     };
 
     const QString booleanErr =
@@ -290,7 +293,7 @@ int main(int argc, char* argv[])
         }
         sessionBus.call(m);
     } else if (parser.isSet(guiArgument)) { // GUI
-        QString pathValue = parser.value(pathOption);
+        QString pathValue = QDir(parser.value(pathOption)).absolutePath();
         int delay = parser.value(delayOption).toInt();
         bool toClipboard = parser.isSet(clipboardOption);
         bool isRaw = parser.isSet(rawImageOption);
@@ -321,7 +324,7 @@ int main(int argc, char* argv[])
             return waitAfterConnecting(delay, app);
         }
     } else if (parser.isSet(fullArgument)) { // FULL
-        QString pathValue = parser.value(pathOption);
+        QString pathValue = QDir(parser.value(pathOption)).absolutePath();
         int delay = parser.value(delayOption).toInt();
         bool toClipboard = parser.isSet(clipboardOption);
         bool isRaw = parser.isSet(rawImageOption);
@@ -376,7 +379,7 @@ int main(int argc, char* argv[])
         QString numberStr = parser.value(screenNumberOption);
         int number =
           numberStr.startsWith(QLatin1String("-")) ? -1 : numberStr.toInt();
-        QString pathValue = parser.value(pathOption);
+        QString pathValue = QDir(parser.value(pathOption)).absolutePath();
         int delay = parser.value(delayOption).toInt();
         bool toClipboard = parser.isSet(clipboardOption);
         bool isRaw = parser.isSet(rawImageOption);

src/tools/launcher/applauncherwidget.cpp

@@ -88,7 +88,7 @@ void AppLauncherWidget::launch(const QModelIndex& index)
 {
     if (!QFileInfo(m_tempFile).isReadable()) {
         m_tempFile =
-          FileNameHandler().generateAbsolutePath(QDir::tempPath()) + ".png";
+          FileNameHandler().properScreenshotPath(QDir::tempPath(), "png");
         bool ok = m_pixmap.save(m_tempFile);
         if (!ok) {
             QMessageBox::about(

src/tools/launcher/openwithprogram.cpp

@@ -23,7 +23,7 @@ void showOpenWithMenu(const QPixmap& capture)
 {
 #if defined(Q_OS_WIN)
     QString tempFile =
-      FileNameHandler().generateAbsolutePath(QDir::tempPath()) + ".png";
+      FileNameHandler().properScreenshotPath(QDir::tempPath(), "png");
     bool ok = capture.save(tempFile);
     if (!ok) {
         QMessageBox::about(nullptr,

src/tools/save/savetool.cpp

@@ -67,7 +67,7 @@ void SaveTool::pressed(const CaptureContext& context)
         }
     } else {
         bool ok = ScreenshotSaver().saveToFilesystem(
-          context.selectedScreenshotArea(), context.savePath, "");
+          context.selectedScreenshotArea(), context.savePath);
         if (ok) {
             emit requestAction(REQ_CAPTURE_DONE_OK);
         }

src/utils/filenamehandler.cpp

@@ -5,7 +5,6 @@
 #include "src/utils/confighandler.h"
 #include "src/utils/strfparse.h"
 #include <QDir>
-#include <QStandardPaths>
 #include <ctime>
 #include <exception>
 #include <locale>
@@ -52,65 +51,75 @@ QString FileNameHandler::parseFilename(const QString& name)
     return res;
 }
 
-QString FileNameHandler::generateAbsolutePath(const QString& path)
+/**
+ * @brief Generate a valid destination path from the possibly incomplete `path`.
+ * The input `path` can be one of:
+ * - empty string
+ * - an existing directory
+ * - a file in an existing directory
+ * In each case, the output path will be an absolute path to a file with a
+ * suffix matching the specified `format`.
+ * @note
+ * - If `path` points to a directory, the file name will be generated from the
+ *   formatted file name from the user configuration
+ * - If `path` points to a file, its suffix will be changed to match `format`
+ * - If `format` is not given, the suffix will remain untouched, unless `path`
+ *   has no suffix, in which case it will be given the "png" suffix
+ * - If the path generated by the previous steps points to an existing file,
+ * 	 "_NUM" will be appended to its base name, where NUM is the first
+ * available number that produces a non-existent path (starting from 1).
+ * @param path Possibly incomplete file name to transform
+ * @param format Desired output file suffix (excluding an initial '.' character)
+ */
+QString FileNameHandler::properScreenshotPath(QString path,
+                                              const QString& format)
 {
-    QString directory = path;
-    QString filename = parsedPattern();
-    fixPath(directory, filename);
-    return directory + filename;
-}
-// path a images si no existe, add numeration
-void FileNameHandler::setPattern(const QString& pattern)
-{
-    ConfigHandler().setFilenamePattern(pattern);
+    QFileInfo info(path);
+    QString suffix = info.suffix();
+
+    if (info.isDir()) {
+        // path is a directory => generate filename from configured pattern
+        path = QDir(QDir(path).absolutePath() + "/" + parsedPattern()).path();
+    } else {
+        // path points to a file => strip it of its suffix for now
+        path = QDir(info.dir().absolutePath() + "/" + info.completeBaseName())
+                 .path();
     }
 
-QString FileNameHandler::absoluteSavePath(QString& directory, QString& filename)
-{
-    ConfigHandler config;
-    directory = config.savePath();
-    if (directory.isEmpty() || !QDir(directory).exists() ||
-        !QFileInfo(directory).isWritable()) {
-        directory =
-          QStandardPaths::writableLocation(QStandardPaths::PicturesLocation);
-    }
-    filename = parsedPattern();
-    fixPath(directory, filename);
-    return directory + filename;
+    if (!format.isEmpty()) {
+        // Override suffix to match format
+        path += "." + format;
+    } else if (!suffix.isEmpty()) {
+        // Leave the suffix as it was
+        path += "." + suffix;
+    } else {
+        path += ".png";
     }
 
-QString FileNameHandler::absoluteSavePath()
-{
-    QString dir, file;
-    return absoluteSavePath(dir, file);
+    if (!QFileInfo::exists(path)) {
+        return path;
+    } else {
+        return autoNumerateDuplicate(path);
+    }
 }
 
-QString FileNameHandler::charArrToQString(const char* c)
+QString FileNameHandler::autoNumerateDuplicate(QString path)
 {
-    return QString::fromLocal8Bit(c, MAX_CHARACTERS);
-}
-
-char* FileNameHandler::QStringToCharArr(const QString& s)
-{
-    QByteArray ba = s.toLocal8Bit();
-    return const_cast<char*>(strdup(ba.constData()));
-}
-
-void FileNameHandler::fixPath(QString& directory, QString& filename)
-{
-    // add '/' at the end of the directory
-    if (!directory.endsWith(QLatin1String("/"))) {
-        directory += QLatin1String("/");
-    }
     // add numeration in case of repeated filename in the directory
     // find unused name adding _n where n is a number
-    QFileInfo checkFile(directory + filename + ".png");
+    QFileInfo checkFile(path);
+    QString directory = checkFile.dir().absolutePath(),
+            filename = checkFile.completeBaseName(),
+            suffix = checkFile.suffix();
+    if (!suffix.isEmpty()) {
+        suffix = QStringLiteral(".") + suffix;
+    }
     if (checkFile.exists()) {
         filename += QLatin1String("_");
         int i = 1;
         while (true) {
-            checkFile.setFile(directory + filename + QString::number(i) +
-                              ".png");
+            checkFile.setFile(directory + "/" + filename + QString::number(i) +
+                              suffix);
             if (!checkFile.exists()) {
                 filename += QString::number(i);
                 break;
@@ -118,4 +127,5 @@ void FileNameHandler::fixPath(QString& directory, QString& filename)
             ++i;
         }
     }
+    return checkFile.filePath();
 }

src/utils/filenamehandler.h

@@ -13,19 +13,12 @@ public:
 
     QString parsedPattern();
     QString parseFilename(const QString& name);
-    QString generateAbsolutePath(const QString& path);
-    QString absoluteSavePath(QString& directory, QString& filename);
-    QString absoluteSavePath();
+
+    QString properScreenshotPath(QString filename,
+                                 const QString& format = QString());
 
     static const int MAX_CHARACTERS = 70;
 
-public slots:
-    void setPattern(const QString& pattern);
-
 private:
-    // using charArr = char[MAX_CHARACTERS];
-    QString charArrToQString(const char* c);
-    char* QStringToCharArr(const QString& s);
-
-    void fixPath(QString& directory, QString& filename);
+    QString autoNumerateDuplicate(QString path);
 };

src/utils/screengrabber.cpp

@@ -43,9 +43,8 @@ QPixmap ScreenGrabber::grabEntireDesktop(bool& ok)
         switch (m_info.windowManager()) {
             case DesktopInfo::GNOME: {
                 // https://github.com/GNOME/gnome-shell/blob/695bfb96160033be55cfb5ac41c121998f98c328/data/org.gnome.Shell.Screenshot.xml
-                QString path =
-                  FileNameHandler().generateAbsolutePath(QDir::tempPath()) +
-                  ".png";
+                QString path = FileNameHandler().properScreenshotPath(
+                  QDir::tempPath(), "png");
                 QDBusInterface gnomeInterface(
                   QStringLiteral("org.gnome.Shell"),
                   QStringLiteral("/org/gnome/Shell/Screenshot"),

src/utils/screenshotsaver.cpp

@@ -14,6 +14,7 @@
 #include <QImageWriter>
 #include <QMessageBox>
 #include <QMimeData>
+#include <QStandardPaths>
 #include <qimagewriter.h>
 #include <qmimedatabase.h>
 #if defined(Q_OS_MACOS)
@@ -87,8 +88,7 @@ bool ScreenshotSaver::saveToFilesystem(const QPixmap& capture,
                                        const QString& path,
                                        const QString& messagePrefix)
 {
-    QString completePath = FileNameHandler().generateAbsolutePath(path);
-    completePath += QLatin1String(".png");
+    QString completePath = FileNameHandler().properScreenshotPath(path);
     bool ok = capture.save(completePath);
     QString saveMessage = messagePrefix;
     QString notificationPath = completePath;
@@ -97,7 +97,6 @@ bool ScreenshotSaver::saveToFilesystem(const QPixmap& capture,
     }
 
     if (ok) {
-        ConfigHandler().setSavePath(path);
         saveMessage += QObject::tr("Capture saved as ") + completePath;
         Controller::getInstance()->sendCaptureSaved(
           m_id, QFileInfo(completePath).canonicalFilePath());
@@ -143,7 +142,13 @@ bool ScreenshotSaver::saveToFilesystemGUI(const QPixmap& capture)
 {
     bool ok = false;
     ConfigHandler config;
-    QString savePath = FileNameHandler().absoluteSavePath();
+    QString defaultSavePath = ConfigHandler().savePath();
+    if (defaultSavePath.isEmpty() || !QDir(defaultSavePath).exists() ||
+        !QFileInfo(defaultSavePath).isWritable()) {
+        defaultSavePath =
+          QStandardPaths::writableLocation(QStandardPaths::PicturesLocation);
+    }
+    QString savePath = FileNameHandler().properScreenshotPath(defaultSavePath);
 #if defined(Q_OS_MACOS)
     for (QWidget* widget : qApp->topLevelWidgets()) {
         QString className(widget->metaObject()->className());
@@ -158,10 +163,7 @@ bool ScreenshotSaver::saveToFilesystemGUI(const QPixmap& capture)
     if (!config.savePathFixed()) {
         // auto imageFormats = QImageWriter::supportedImageFormats();
         savePath =
-          ShowSaveFileDialog(nullptr,
-                             QObject::tr("Save screenshot"),
-                             FileNameHandler().absoluteSavePath() +
-                               ConfigHandler().getSaveAsFileExtension());
+          ShowSaveFileDialog(nullptr, QObject::tr("Save screenshot"), savePath);
     }
     if (savePath == "") {
         return ok;

src/utils/screenshotsaver.h

@@ -18,7 +18,7 @@ public:
     void saveToClipboardMime(const QPixmap& capture, const QString& imageType);
     bool saveToFilesystem(const QPixmap& capture,
                           const QString& path,
-                          const QString& messagePrefix);
+                          const QString& messagePrefix = "");
     bool saveToFilesystemGUI(const QPixmap& capture);
 
 private:

src/widgets/capture/capturewidget.cpp

@@ -1593,8 +1593,7 @@ void CaptureWidget::saveScreenshot()
     if (m_context.savePath.isEmpty()) {
         ScreenshotSaver(m_id).saveToFilesystemGUI(pixmap());
     } else {
-        ScreenshotSaver(m_id).saveToFilesystem(
-          pixmap(), m_context.savePath, "");
+        ScreenshotSaver(m_id).saveToFilesystem(pixmap(), m_context.savePath);
     }
     close();
 }

tests/path_option.sh

@@ -0,0 +1,52 @@
+#!/usr/bin/env sh
+
+# Before running the script make sure a flameshot daemon with a matching version
+# is running
+
+# The first argument to this script is a path to the flameshot executable
+[ -n "$1" ] && flameshot="$1" || flameshot='flameshot'
+
+# TODO Before proper stderr logging is implemented, you will have to look at the
+# system notifications
+
+rm -rf /tmp/flameshot_path_test 2>/dev/null
+mkdir -p /tmp/flameshot_path_test
+cd /tmp/flameshot_path_test
+
+echo ">> Nonexistent directory. This command should give an invalid path error."
+"$flameshot" screen -p blah/blah
+
+sleep 2
+echo ">> The output file is specified relative to PWD"
+"$flameshot" screen -p relative.png
+
+sleep 2
+echo ">> Absolute paths work too"
+"$flameshot" screen -p /tmp/flameshot_path_test/absolute.png
+
+sleep 2
+mkdir subdir
+echo ">> Redundancy in the path will be removed"
+"$flameshot" screen -p /tmp/flameshot_path_test/subdir/..///redundancy_removed.png
+
+sleep 2
+echo ">> If the destionation is a directory, the file name is generated from strf from the config"
+"$flameshot" screen -p ./
+
+sleep 2
+echo ">> If the output file has no suffix, it will be added (png)"
+"$flameshot" screen -p /tmp/flameshot_path_test/without_suffix
+
+sleep 2
+echo ">> Other suffixes are supported, and the image format will match it"
+"$flameshot" screen -p /tmp/flameshot_path_test/jpg_suffix.jpg
+
+sleep 2
+echo ">> If the destination path exists, it will have _NUM appended to the base name"
+"$flameshot" screen -p /tmp/flameshot_path_test/absolute.png
+
+sleep 2
+echo ">> Same thing again but without specifying a suffix"
+"$flameshot" screen -p /tmp/flameshot_path_test/absolute
+
+sleep 2